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?
