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

[Andrew Morton <akpm@linux-foundation.org>]

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?