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

[Kasireddy, Vivek]

Hi Andrew,

> 
> > Hi Andrew and SJ,
> >
> > >
> > > >
> > > > 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 at intel.com>
> 
> Cool, thanks.
> 
> We could do this to propagate the try_grab_folio() return value:
> 
> --- a/mm/gup.c~mm-gup-introduce-memfd_pin_folios-for-pinning-memfd-
> folios-fix-fix
> +++ a/mm/gup.c
> @@ -3848,6 +3848,8 @@ long memfd_pin_folios(struct file *memfd
> 
>  			next_idx = 0;
>  			for (i = 0; i < nr_found; i++) {
> +				int ret2;
Is there any reason why you introduced a new variable instead of just reusing ret?

> +
>  				/*
>  				 * As there can be multiple entries for a
>  				 * given folio in the batch returned by
> @@ -3860,10 +3862,10 @@ long memfd_pin_folios(struct file *memfd
>  					continue;
> 
>  				folio = page_folio(&fbatch.folios[i]->page);
> -
> -				if (try_grab_folio(folio, 1, FOLL_PIN)) {
> +				ret2 = try_grab_folio(folio, 1, FOLL_PIN);
> +				if (ret2) {
>  					folio_batch_release(&fbatch);
> -					ret = -EINVAL;
> +					ret = ret2;
>  					goto err;
>  				}
> 
> 
> But try_grab_folio can return that weird -EREMOTEIO.  The
> try_grab_folio() kerneldoc doesn't even mention that - it incorrectly
> claims that only -ENOMEM can be returned. (needs fixing!).
> 
> And if memfd_pin_folios() returns -EREMOTEIO then I expect
> udmabuf_ioctl() will return -EREMOTEIO to userspace.  And userspace
It is not clear to me if userspace needs to do anything in particular to handle
this specific error. At the moment, Qemu (userspace user of udmabuf_ioctl)
does not do anything special for any error codes, and just treats them all as
failures though.

> will wonder "what the hell is that".  If there's a udmabuf_ioctl
> manpage then will that explain this errno?  And similar concerns for
> future callers of memfd_pin_folios().
Sure, we can definitely add documentation explaining this situation once we
agree how the userspace needs to handle this specific error code.

Thanks,
Vivek