Re: Weird code with change "mm/gup: clean up follow_pfn_pte() slightly"

[Jason Gunthorpe]

On Thu, Feb 03, 2022 at 12:44:57PM -0800, John Hubbard wrote:
> On 2/3/22 05:01, Jason Gunthorpe wrote:
> ...
> > > > In the new branch if (pages), you set page = ERR_PTR(-EFAULT) and goto
> > > > out. However, at the label out, the value of page is not used, but the
> > > > return uses the variables i and ret.
> > > 
> > > Yes, I think that the complaint is accurate. The intent of this code is
> > > to return either number of pages so far (i) or ret (which should be zero
> > > in this case), because we are just stopping early, rather than calling
> > > this an actual error.
> > 
> > IIRC GUP shouldn't return 0, it should return an error code, not zero.
> > 
> > Jason
> 
> Errors work for single pages, but GUP is a multi-page API call. If it
> returned an error part way through the list of pages, then callers would
> have no way of knowing how many pages to release.

Yes, but that is returning a positive error code, I said it should not
return zero.

When it hits an error with pages already loaded it returns that number
and the caller will then do gup once more with the VA pointing at the
problematic page. Then GUP can return the error code because it has 0
pages on the next iteration.

It should not return 0 here when it got an error.

>  * Returns either number of pages pinned (which may be less than the
>  * number requested), or an error. Details about the return value:
>  *
>  * -- If nr_pages is 0, returns 0.
>  * -- If nr_pages is >0, but no pages were pinned, returns -errno.
>  * -- If nr_pages is >0, and some pages were pinned, returns the number of
>  *    pages pinned. Again, this may be less than nr_pages.
>  * -- 0 return value is possible when the fault would need to be retried.

I actually don't know of any place that handles the 0 return code, or
what 'fault would need to be retried' is supposed to mean for the
caller ...

Jason