On Mon, Sep 08, 2025 at 10:27:23AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 08, 2025 at 12:10:44PM +0100, Lorenzo Stoakes wrote:
> > We thread the state through the mmap_context, allowing for both PFN map and
> > mixed mapped pre-population.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> > ---
> > fs/cramfs/inode.c | 134 +++++++++++++++++++++++++++++++---------------
> > 1 file changed, 92 insertions(+), 42 deletions(-)
> >
> > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > index b002e9b734f9..11a11213304d 100644
> > --- a/fs/cramfs/inode.c
> > +++ b/fs/cramfs/inode.c
> > @@ -59,6 +59,12 @@ static const struct address_space_operations cramfs_aops;
> >
> > static DEFINE_MUTEX(read_mutex);
> >
> > +/* How should the mapping be completed? */
> > +enum cramfs_mmap_state {
> > + NO_PREPOPULATE,
> > + PREPOPULATE_PFNMAP,
> > + PREPOPULATE_MIXEDMAP,
> > +};
> >
> > /* These macros may change in future, to provide better st_ino semantics. */
> > #define OFFSET(x) ((x)->i_ino)
> > @@ -342,34 +348,89 @@ static bool cramfs_last_page_is_shared(struct inode *inode)
> > return memchr_inv(tail_data, 0, PAGE_SIZE - partial) ? true : false;
> > }
> >
> > -static int cramfs_physmem_mmap(struct file *file, struct vm_area_struct *vma)
> > +static int cramfs_physmem_mmap_complete(struct file *file, struct vm_area_struct *vma,
> > + const void *context)
> > {
> > struct inode *inode = file_inode(file);
> > struct cramfs_sb_info *sbi = CRAMFS_SB(inode->i_sb);
> > - unsigned int pages, max_pages, offset;
> > unsigned long address, pgoff = vma->vm_pgoff;
> > - char *bailout_reason;
> > - int ret;
> > + unsigned int pages, offset;
> > + enum cramfs_mmap_state mmap_state = (enum cramfs_mmap_state)context;
> > + int ret = 0;
> >
> > - ret = generic_file_readonly_mmap(file, vma);
> > - if (ret)
> > - return ret;
> > + if (mmap_state == NO_PREPOPULATE)
> > + return 0;
>
> It would be nicer to have different ops than this, the normal op could
> just call the generic helper and then there is only the mixed map op.
Right, but I can't stop to refactor everything I change, or this effort will
take even longer.
I do have to compromise a _little_ on that as there's ~250 odd callsites to
go...
>
> Makes me wonder if putting the op in the fops was right, a
> mixed/non-mixed vm_ops would do this nicely.
I added a reviewers note just for you in 00/16 :) I guess you missed it:
REVIEWER NOTE:
~~~~~~~~~~~~~~
I considered putting the complete, abort callbacks in vm_ops,
however this won't work because then we would be unable to adjust
helpers like ngeneric_file_mmap_prepare() (which provides vm_ops)
to provide the correct complete, abort callbacks.
Conceptually it also makes more sense to have these in f_op as they
are one-off operations performed at mmap time to establish the VMA,
rather than a property of the VMA itself.
Basically, existing generic code sets vm_ops to something already, now we'd
need to somehow also vary it on this as well or nest vm_ops? I don't think
it's workable.
I found this out because I started working on this series with the complete
callback as part of vm_ops then hit this stumbling block as a result.
>
> Jason
Cheers, Lorenzo
