Hi Andrew and SJ,
>
> On Fri, 5 Jul 2024 13:48:25 -0700 SeongJae Park <sj@xxxxxxxxxx> 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@xxxxxxxxx>
Thanks,
Vivek
