Re: [PATCH 13/16] mm: update cramfs to use mmap_prepare, mmap_complete

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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@oracle.com>
> > ---
> >  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