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]

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-06-13 at 05:23 -0400, Mike Snitzer wrote:
> On Thu, Jun 12, 2025 at 10:46:01PM -0700, Christoph Hellwig wrote:
> > On Thu, Jun 12, 2025 at 12:22:42PM -0400, Jeff Layton wrote:
> > > If you're against the idea, I won't waste my time.
> > > 
> > > It would require some fairly hefty rejiggering of the receive code. The
> > > v4 part would be pretty nightmarish to work out too since you'd have to
> > > decode the compound as you receive to tell where the next op starts.
> > > 
> > > The potential for corruption with unaligned writes is also pretty
> > > nasty.
> > 
> > Maybe I'm missing an improvement to the receive buffer handling in modern
> > network hardware, but AFAIK this still would only help you to align the
> > sunrpc data buffer to page boundaries, but avoid the data copy from the
> > hardware receive buffer to the sunrpc data buffer as you still don't have
> > hardware header splitting.
> 
> Correct, everything that Jeff detailed is about ensuring the WRITE
> payload is received into page aligned boundary.
> 
> Which in practice has proven a hard requirement for O_DIRECT in my
> testing -- but I could be hitting some bizarre driver bug in my TCP
> testbed (which sadly sits ontop of older VMware guests/drivers).
> 
> But if you looking at patch 5 in this series:
> https://lore.kernel.org/linux-nfs/20250610205737.63343-6-snitzer@kernel.org/
> 
> I added fs/nfsd/vfs.c:is_dio_aligned(), which is basically a tweaked
> ditto of fs/btrfs/direct-io.c:check_direct_IO():
> 
> static bool is_dio_aligned(const struct iov_iter *iter, loff_t offset,
>                            const u32 blocksize)
> {
>         u32 blocksize_mask;
> 
>         if (!blocksize)
>                 return false;
> 
>         blocksize_mask = blocksize - 1;
>         if ((offset & blocksize_mask) ||
>             (iov_iter_alignment(iter) & blocksize_mask))
>                 return false;
> 
>         return true;
> }
> 
> And fs/nfsd/vfs.c:nfsd_vfs_write() has (after my patch 5):
> 
>         nvecs = xdr_buf_to_bvec(rqstp->rq_bvec, rqstp->rq_maxpages, payload);
>         iov_iter_bvec(&iter, ITER_SOURCE, rqstp->rq_bvec, nvecs, *cnt);
> 
>         if (nfsd_enable_dontcache) {
>                 if (is_dio_aligned(&iter, offset, nf->nf_dio_offset_align))
>                         flags |= RWF_DIRECT;
> 
> What I found is that unless SUNRPC TPC stored the WRITE payload in a
> page-aligned boundary then iov_iter_alignment() would fail.
> 
> The @payload arg above, with my SUNRPC TCP testing, was always offset
> 148 bytes into the first page of the pages allocated for xdr_buf's
> use, which is rqstp->rq_pages, which is allocated by
> net/sunrpc/svc_xprt.c:svc_alloc_arg().
> 
> > And I don't even know what this is supposed to buy the nfs server.
> > Direct I/O writes need to have the proper file offset alignment, but as
> > far as Linux is concerned we don't require any memory alignment.  Most
> > storage hardware has requirements for the memory alignment that we pass
> > on, but typically that's just a dword (4-byte) alignment, which matches
> > the alignment sunrpc wants for most XDR data structures anyway.  So what
> > additional alignment is actually needed for support direct I/O writes
> > assuming that is the goal?  (I might also simply misunderstand the
> > problem).
>
> THIS... this is the very precise question/detail I discussed with
> Hammerspace's CEO David Flynn when discussing Linux's O_DIRECT
> support.  David shares your understanding and confusion.  And all I
> could tell him is that in practice I always page-aligned my data
> buffers used to issue O_DIRECT.  And that in this instance if I don't
> then O_DIRECT doesn't work (if I commented out the iov_iter_alignment
> check in is_dio_aligned above).
>
> But is that simply due to xdr_buf_to_bvec()'s use of bvec_set_virt()
> for xdr_buf "head" page (first page of rqstp->rg_pages)?  Whereas you
> can see xdr_buf_to_bvec() uses bvec_set_page() to add each of the
> other pages that immediately follow the first "head" page.
> 
> All said, if Linux can/should happily allow non-page-aligned DIO (and
> we only need to worry about the on-disk DIO alignment requirements)
> that'd be wonderful.
> 
> Then its just a matter of finding where that is broken...
> 
> Happy to dig into this further if you might nudge me in the right
> direction.
> 

This is an excellent point. If the memory alignment doesn't matter,
then maybe it's enough to just receive the same way we do today and
just pad out to the correct blocksize in the bvec array if the data is
unaligned vs. the blocksize.

We still have the problem of how to do a proper RMW though to deal with
unaligned writes. A couple of possibilities come to mind:

1. nfsd could just return nfserr_inval when a write is unaligned and
the export is set up for DIO writes. IOW, just project the requirement
about alignment to the client. This might be the safest option, at
least initially. Unaligned writes are pretty uncommon. Most clients
will probably never hit the error.

2. What if we added a new "rmw_iter" operation to file_operations that
could be used for unaligned writes? XFS (for instance) could take the
i_rwsem exclusive, do DIO reads of the end blocks into bounce pages,
copy in the unaligned bits at the ends of the iter, do a DIO write and
release the lock. It'll be slow as hell, but it wouldn't be racy.

Mike, would you be amenable to option #1, at least initially? If we can
come up with a way to do unaligned writes safely, we could relax the
restriction later.

I'm only half serious about rmw_iter, but it does seem like that could
work.
-- 
Jeff Layton <jlayton@kernel.org>