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]

[Mike Snitzer <snitzer@kernel.org>]

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.

Thanks,
Mike