Re: [PATCH] mm: get_user_pages() stores ERR_PTR() in pages[i] on failure

[Hugh Dickins <hugh.dickins@tiscali.co.uk>]

On Tue, 4 Aug 2009, Brice Goglin wrote:
> 
> >> I wonder if we should change get_user_pages to store ERR_PTR(ret)
> >> in page[i] when it fails to get page #i.
> >>     
> > Yes, I would see that as an improvement in finding out why rc <
> > nr_pages, in case rc > 0.
> >
> > Also I think it does not break existing users.
> >   
> 
> Only compile-tested (and not in the nommu case).
> 
> 
> 
> 
> When get_user_pages() fails to get a non-first page, it returns
> the number of successfully gotten pages. The caller has to call
> get_user_pages() again on the failed page to figure out the
> error code.
> 
> Store the error code with ERR_PTR() in pages[i] when we fail
> to get page #i.
> 
> The arch specific get_user_pages_fast() do not need to be changed
> since they revert to the main get_user_pages() on failure.
> 
> Signed-off-by: Brice Goglin <Brice.Goglin@inria.fr>

This is a nice idea, and it looks like a good patch to implement
it, and I wish get_user_pages() had done that from the start, but...

... some callers of get_user_pages() might expect it not to touch
entries in the page array beyond the fail point: for example, they
might start off with a zeroed array, then when they come to put_page
on the array afterwards, would skip any NULL entries found - without
having to have noted the returned number of pages.  I don't think
that way of working is supported by any guarantee in documentation,
but it's not unreasonable to expect it to behave in that way.

If there were nothing intree which worked that way, then I'd
say let's go ahead with your nice patch.  But I soon reached
drivers/gpu/drm/ttm/ttm_tt.c then drivers/gpu/drm/via/via_dmablit.c
which both appear to work in that way.  I've not looked beyond them,
but I suspect those two cases amount to a NAK on your patch, sadly.
Of course we _could_ fix individual callers to work with it, but
for me they sound a warning that it's too late to change now.

Hugh