RE: [PATCH v16 3/9] mm/gup: Introduce memfd_pin_folios() for pinning memfd folios

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

Hi Andrew and SJ, 

> 
> On Fri,  5 Jul 2024 13:48:25 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > > + * memfd_pin_folios() - pin folios associated with a memfd
> > [...]
> > > +			for (i = 0; i < nr_found; i++) {
> > > +				/*
> > > +				 * As there can be multiple entries for a
> > > +				 * given folio in the batch returned by
> > > +				 * filemap_get_folios_contig(), the below
> > > +				 * check is to ensure that we pin and return a
> > > +				 * unique set of folios between start and end.
> > > +				 */
> > > +				if (next_idx &&
> > > +				    next_idx != folio_index(fbatch.folios[i]))
> > > +					continue;
> > > +
> > > +				folio = try_grab_folio(&fbatch.folios[i]->page,
> > > +						       1, FOLL_PIN);
> > > +				if (!folio) {
> > > +					folio_batch_release(&fbatch);
> > > +					ret = -EINVAL;
> > > +					goto err;
> > > +				}
> >
> > I found this patch is applied on mm-unstable as commit 7618d1ff59ef
> ("mm/gup:
> > introduce memfd_pin_folios() for pinning memfd folios").  Somehow,
> however, the
> > commit has changd the above try_grab_folio() call to try_grab_folio_fast()
> > call.
> >
> > As a result, building kernel without CONFIG_MMU fais as below:
> >
> > ...
> >
> > Maybe the change has made to fix conflict with another mm-unstable
> commit
> > 02a2d55767d1 ("mm: gup: stop abusing try_grab_folio"), but forgot the
> > CONFIG_MMU unset case?
> 
> Yes.  That patch didn't add a CONFIG_MMU=n version of
> try_grab_folio_fast().  Maybe it should have?
> 
> > I confirmed the failure disappears after further cleanup like below:
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 46a266ed84f7..9f4902425070 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -3859,9 +3859,9 @@ long memfd_pin_folios(struct file *memfd, loff_t
> start, loff_t end,
> >                                     next_idx != folio_index(fbatch.folios[i]))
> >                                         continue;
> >
> > -                               folio = try_grab_folio_fast(&fbatch.folios[i]->page,
> > -                                                      1, FOLL_PIN);
> > -                               if (!folio) {
> > +                               folio = page_folio(&fbatch.folios[i]->page);
> > +
> > +                               if (try_grab_folio(folio, 1, FOLL_PIN)) {
> >                                         folio_batch_release(&fbatch);
> >                                         ret = -EINVAL;
> >                                         goto err;
> >
> > I didn't look deep into the patch, so unsure if that's a valid fix, though.
> > May I ask your thoughts?
> 
> Perhaps we should propagate the errno which was returned by
> try_grab_folio()?
> 
> I'll do it this way.  Vivek, please check and let us know?
Yeah, memfd_pin_folios() doesn't need the fast version, so replacing with
the slow version (try_grab_folio) should be fine. And, as you suggest,
propagating the errno returned by try_grab_folio() would be the right thing
to do instead of explicitly setting errno to -EINVAL. Either way, this change is
Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

Thanks,
Vivek