Re: [PATCH 1/1] NFSv4.2: fix LISTXATTR buffer receive size

[Chuck Lever <chuck.lever@oracle.com>]

> On Nov 23, 2020, at 12:38 PM, Frank van der Linden <fllinden@amazon.com> wrote:
> 
> On Mon, Nov 23, 2020 at 11:42:46AM -0500, Olga Kornievskaia wrote:
>> Hi Frank, Chuck,
>> 
>> I would like your option on how LISTXATTR is supposed to work over
>> RDMA. Here's my current understanding of why the listxattr is not
>> working over the RDMA.
>> 
>> This happens when the listxattr is called with a very small buffer
>> size which RDMA wants to send an inline request. I really dont
>> understand why, Chuck, you are not seeing any problems with hardware
>> as far as I can tell it would have the same problem because the inline
>> threshold size would still make this size inline.
>> rcprdma_inline_fixup() is trying to write to pages that don't exist.
>> 
>> When LISTXATTR sets this flag XDRBUF_SPARSE_PAGES there is code that
>> will allocate pages in xs_alloc_sparse_pages() but this is ONLY for
>> TCP. RDMA doesn't have anything like that.
>> 
>> Question: Should there be code added to RDMA that will do something
>> similar when it sees that flag set? Or, should LISTXATTR be re-written
>> to be like READDIR which allocates pages before calling the code.
> 
> Hm.. so if the flag never worked for RDMA, was NFS_V3_ACL ever tested
> over RDMA? That's the only other user.
> 
> If the flag simply doesn't work, I agree that it should either be fixed
> or just removed.

Shirley fixed a crasher here years ago by making SPARSE_PAGES work
for RPC/RDMA. She confirmed the fix was effective.


> It wouldn't be the end of the world to change LISTXATTRS (and GETXATTR)
> to use preallocated pages. But, I didn't do that because I didn't want to
> waste the max size (64k) every time, even though you usually just get
> a few hundred bytes at most. So it seems like fixing XDRBUF_SPARSE_PAGES
> is cleaner.

These are infrequent operations. We've added extra conditional branches
in the hot path of the transports to handle this rare, non-performance
sensitive case.

I also wonder how well SPARSE_PAGES will work with xdr->bvecs, since
the bio_vec array is constructed before the transport can allocate
those pages.

To me, the whole SPARSE_PAGES concept is prototype-quality code that
needs to be replaced with a robust permanent solution.


--
Chuck Lever