Re: need SUNRPC TCP to receive into aligned pages [was: Re: [PATCH 1/6] NFSD: add the ability to enable use of RWF_DONTCACHE for all IO]

Benjamin Coddington
2025-06-12T14:17:35Z

On 12 Jun 2025, at 9:28, Chuck Lever wrote:

> On 6/12/25 6:28 AM, Jeff Layton wrote:
>> On Wed, 2025-06-11 at 17:36 -0400, Mike Snitzer wrote:
>>> On Wed, Jun 11, 2025 at 04:29:58PM -0400, Jeff Layton wrote:
>>>> On Wed, 2025-06-11 at 15:18 -0400, Mike Snitzer wrote:
>>>>> On Wed, Jun 11, 2025 at 10:31:20AM -0400, Chuck Lever wrote:
>>>>>> On 6/10/25 4:57 PM, Mike Snitzer wrote:
>>>>>>> Add 'enable-dontcache' to NFSD's debugfs interface so that: Any data
>>>>>>> read or written by NFSD will either not be cached (thanks to O_DIRECT)
>>>>>>> or will be removed from the page cache upon completion (DONTCACHE).
>>>>>>
>>>>>> I thought we were going to do two switches: One for reads and one for
>>>>>> writes? I could be misremembering.
>>>>>
>>>>> We did discuss the possibility of doing that. Still can-do if that's
>>>>> what you'd prefer.
>>>>>
>>>>
>>>> Having them as separate controls in debugfs is fine for
>>>> experimentation's sake, but I imagine we'll need to be all-in one way
>>>> or the other with a real interface.
>>>>
>>>> I think if we can crack the problem of receiving WRITE payloads into an
>>>> already-aligned buffer, then that becomes much more feasible. I think
>>>> that's a solveable problem.
>>>
>>> You'd immediately be my hero! Let's get into it:
>>>
>>> In a previously reply to this thread you aptly detailed what I found
>>> out the hard way (with too much xdr_buf code review and tracing):
>>>
>>> On Wed, Jun 11, 2025 at 08:55:20AM -0400, Jeff Layton wrote:
>>>>>
>>>>> NFSD will also set RWF_DIRECT if a WRITE's IO is aligned relative to
>>>>> DIO alignment (both page and disk alignment). This works quite well
>>>>> for aligned WRITE IO with SUNRPC's RDMA transport as-is, because it
>>>>> maps the WRITE payload into aligned pages. But more work is needed to
>>>>> be able to leverage O_DIRECT when SUNRPC's regular TCP transport is
>>>>> used. I spent quite a bit of time analyzing the existing xdr_buf code
>>>>> and NFSD's use of it. Unfortunately, the WRITE payload gets stored in
>>>>> misaligned pages such that O_DIRECT isn't possible without a copy
>>>>> (completely defeating the point). I'll reply to this cover letter to
>>>>> start a subthread to discuss how best to deal with misaligned write
>>>>> IO (by association with Hammerspace, I'm most interested in NFS v3).
>>>>>
>>>>
>>>> Tricky problem. svc_tcp_recvfrom() just slurps the whole RPC into the
>>>> rq_pages array. To get alignment right, you'd probably have to do the
>>>> receive in a much more piecemeal way.
>>>>
>>>> Basically, you'd need to decode as you receive chunks of the message,
>>>> and look out for WRITEs, and then set it up so that their payloads are
>>>> received with proper alignment.
>>>
>>> 1)
>>> Yes, and while I arrived at the same exact conclusion I was left with
>>> dread about the potential for "breaking too many eggs to make that
>>> tasty omelette".
>>>
>>> If you (or others) see a way forward to have SUNRPC TCP's XDR receive
>>> "inline" decode (rather than have the 2 stage process you covered
>>> above) that'd be fantastic. Seems like really old tech-debt in SUNRPC
>>> from a time when such care about alignment of WRITE payload pages was
>>> completely off engineers' collective radar (owed to NFSD only using
>>> buffered IO I assume?).
>>>
>>> 2)
>>> One hack that I verified to work for READ and WRITE IO on my
>>> particular TCP testbed was to front-pad the first "head" page of the
>>> xdr_buf such that the WRITE payload started at the 2nd page of
>>> rq_pages. So that looked like this hack for my usage:
>>>
>>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>>> index 8fc5b2b2d806..cf082a265261 100644
>>> --- a/net/sunrpc/svc_xprt.c
>>> +++ b/net/sunrpc/svc_xprt.c
>>> @@ -676,7 +676,9 @@ static bool svc_alloc_arg(struct svc_rqst *rqstp)
>>>
>>> /* Make arg->head point to first page and arg->pages point to rest */
>>> arg->head[0].iov_base = page_address(rqstp->rq_pages[0]);
>>> - arg->head[0].iov_len = PAGE_SIZE;
>>> + // FIXME: front-pad optimized to align TCP's WRITE payload
>>> + // but may not be enough for other operations?
>>> + arg->head[0].iov_len = 148;
>>> arg->pages = rqstp->rq_pages + 1;
>>> arg->page_base = 0;
>>> /* save at least one page for response */
>>>
>>> That gut "but may not be enough for other operations?" comment proved
>>> to be prophetic.
>>>
>>> Sadly it went on to fail spectacularly for other ops (specifically
>>> READDIR and READDIRPLUS, probably others would too) because
>>> xdr_inline_decode() _really_ doesn't like going beyond the end of the
>>> xdr_buf's inline "head" page. It could be that even if
>>> xdr_inline_decode() et al was "fixed" (which isn't for the faint of
>>> heart given xdr_buf's more complex nature) there will likely be other
>>> mole(s) that pop up. And in addition, we'd be wasting space in the
>>> xdr_buf's head page (PAGE_SIZE-frontpad). So I moved on from trying
>>> to see this frontpad hack through to completion.
>>>
>>> 3)
>>> Lastly, for completeness, I also mentioned briefly in a previous
>>> recent reply:
>>>
>>> On Wed, Jun 11, 2025 at 04:51:03PM -0400, Mike Snitzer wrote:
>>>> On Wed, Jun 11, 2025 at 11:44:29AM -0400, Jeff Layton wrote:
>>>>
>>>>> In any case, for now at least, unless you're using RDMA, it's going to
>>>>> end up falling back to buffered writes everywhere. The data is almost
>>>>> never going to be properly aligned coming in off the wire. That might
>>>>> be fixable though.
>>>>
>>>> Ben Coddington mentioned to me that soft-iwarp would allow use of RDMA
>>>> over TCP to workaround SUNRPC TCP's XDR handling always storing the
>>>> write payload in misaligned IO. But that's purely a stop-gap
>>>> workaround, which needs testing (to see if soft-iwap negates the win
>>>> of using O_DIRECT, etc).
>>>
>>> (Ab)using soft-iwarp as the basis for easily getting page aligned TCP
>>> WRITE payloads seems pretty gross given we are chasing utmost
>>> performance, etc.
>>>
>>> All said, I welcome your sage advice and help on this effort to
>>> DIO-align SUNRPC TCP's WRITE payload pages.
>>>
>>> Thanks,
>>> Mike
>>
>> (Sent this to Mike only by accident yesterday -- resending to the full
>> list now)
>>
>> I've been looking over the code today. Basically, I think we need to
>> have svc_tcp_recvfrom() receive in phases. At a high level:
>>
>> 1/ receive the record marker (just like it does today)
>>
>> 2/ receive enough for the RPC header and then decode it.
>>
>> 3/ Use the rpc program and version from the decoded header to look up
>> the svc_program. Add an optional pg_tcp_recvfrom callback to that
>> structure that will receive the rest of the data into the buffer. If
>> pg_tcp_recvfrom isn't set, then just call svc_tcp_read_msg() like we do
>> today.
>
> The layering violations here are mind-blowing.

What's already been mentioned elsewhere, but not yet here:

The transmitter could always just tell the receiver where the data is, we'd
need an NFS v3.1 and an extension for v4.2?

Pot Stirred,
Ben