Hi- > On Jun 28, 2021, at 2:17 PM, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Mon, Jun 28, 2021 at 02:12:59PM -0400, Chuck Lever wrote: >> 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
