Hi-

On Jun 28, 2021, at 2:17 PM, Matthew Wilcox [off-list ref] wrote:

On Mon, Jun 28, 2021 at 02:12:59PM -0400, Chuck Lever wrote:

quoted

The SUNRPC consumer of __alloc_bulk_pages() legitimately calls it
with a full array sometimes. In that case, the correct return code,
according to the API contract, is to return the number of pages
already in the array/list.

Let's clean up the return logic to make it clear that the returned
value is always the total number of pages in the array/list, not the
number of pages that were allocated during this call.

This is more complicated than either v1 or the version that Mel sent
earlier today.  Is it worth it?

Yes.

My v2 addresses the reason the bug was introduced in the first place: The
code currently does not reflect the API contract described in the documenting
comment.

A human reading the function as it is currently written might easily expect
that a zero return code is proper if something failed. However, the API
contract does not list zero as a unique return value with a special meaning.
The contract merely states: "Returns the number of pages on the list or
array." Therefore zero is a plausible return value only if @nr_pages is
zero or less.

Note that the value returned if prepare_alloc_pages() fails is also incorrect,
by my reading, and my v2 addresses that.

The only other call site is __page_pool_alloc_pages_slow(), and that looks
incorrect to me -- it does not agree with either the API contract or the
SUNRPC call site. I did not fix that, but I think it should be looked into
by someone familiar with that code.

I haven't seen the patch Mel sent earlier today. I was not cc'd on that one
or on b3b64ebd3822.

--
Chuck Lever