Re: [RFC PATCH v1 0/8] nfs: PCI P2PDMA for O_DIRECT over RDMA

[Pranjal Shrivastava <praan@google.com>]

On Wed, Oct 07, 2026 at 05:20:50PM -0400, Chuck Lever wrote:
> On 10/6/26 7:32 PM, Pranjal Shrivastava wrote:
> > Enable NFS O_DIRECT to and from PCI peer-to-peer DMA (P2PDMA) memory
> > on RDMA mounts. This replaces the initial RFC [1] and builds on the
> > Direct I/O modernization series [2].
>
> Hi Praan,
>
> Good to see this moving.

Hi Chuck,

Thanks a lot for the detailed review!

> > 6. pNFS: excluded for now. Route P2PDMA I/O through the MDS, or let
> >    each layout driver opt in?
> >
> > Known issue: the pNFS check happens at extraction. If a mount gains
> > pNFS afterwards (e.g. after migration), a rescheduled WRITE could
> > reach a layout driver. I plan to decide once per O_DIRECT call and
> > force the MDS path when P2PDMA pages are allowed.
>
> I think we need to have a path to supporting DS I/O with P2PDMA.
>
> The deployment sites that want P2PDMA O_DIRECT are the same sites
> running flexfiles, and flexfiles with a co-located DS is where
> LOCALIO actually offers quite a benefit. So for question 6 I'd say:
> layout driver opt-in, flexfiles first, and the flexfiles LOCALIO
> path is part of that.

Makes sense. Layout driver opt-in it is, with flexfiles first.

> Although it is fine for a prototype, forcing the MDS path will
> always be the slow path. Can v1 be designed so the follow-on is
> additive? As posted it might not be. The P2PDMA refusals sit in
> the MDS-only pgio path and at page extraction, and the decision is
> made once and never revisited where the RPC is built.
>
>  - The krb5i/krb5p refusal is in nfs_pgio_prepare(). Flexfiles and
>    filelayout install their own rpc_call_prepare, so a DS RPC never
>    sees it and gets as far as call_encode, where gss_wrap() encrypts
>    device memory in place before xprtrdma has a chance to refuse.
>
>  - ff_layout_read_pagelist() and ff_layout_write_pagelist() open an
>    nfsd_file through ff_local_open_fh() without going through
>    nfs_local_p2pdma_check().
>
> If the auth refusal lives in the RPC client (any XDRBUF_P2PDMA
> buffer under an RPCAUTH_AUTH_DATATOUCH flavor) and the rest lives
> in the transport, then every RPC gets checked no matter who built
> it, and enabling flexfiles later is a small series on top instead
> of reworking already-merged code.

Agreed, the checks shouldn't depend on who builds the RPC. For v2,
I plan to:

 - Move the krb5i/krb5p refusal into rpcauth_wrap_req_encode(), right
   after the encoder tags the buffer and before gss_wrap_req_integ()
   and gss_wrap_req_priv() touch the pages. Both of them currently
   turn an encoder failure into -EMSGSIZE / -EIO, so they'd pass the
   error up instead. The check in nfs_pgio_prepare() goes away.

 - Make the LOCALIO decision in nfs_initiate_pgio(), which is where
   both the generic pgio path and flexfiles hand a header over to
   nfs_local_doio(). That covers ff_local_open_fh() as well.

 - Add a layout driver flag for P2PDMA, with no driver setting it in
   this series, and decide once per O_DIRECT call. A layout driver
   that hasn't opted in gets the MDS path, which also closes the
   migration race. Flexfiles, including its LOCALIO path, would then
   be a small series on top.

> > 1. Layering: "RDMA transport, !pNFS" is only a hint, and each RPC
> >    re-checks its transport. Should the transport advertise P2PDMA
> >    capability up front instead? That would also allow LOCALIO on
> >    TCP mounts, which this series refuses.
>
> Yes, please. LOCALIO never goes near the NIC, so a TCP mount has
> no reason to be refused. Advertise the capability and keep the
> per-RPC check as the backstop; that covers both cases.

Ack. I'll add a transport capability that xprtrdma advertises. NFS
uses it as the extraction hint, and xprt_request_transmit() refuses
a tagged request on a transport without it, which also replaces the
three xs_* guards you pointed out below. The per-RPC device check in
xprtrdma stays as the backstop.

With LOCALIO out of v2 (below), a TCP mount still refuses P2PDMA
pages for now, but the capability leaves room for LOCALIO to opt in
later.

> > 5. LOCALIO: the check uses sb->s_bdev, which misses multi-device
> >    filesystems. Is there a better way to ask whether a file can do
> >    P2PDMA? Should misaligned I/O fall back to an RPC instead of
> >    failing with -EINVAL?
>
> s_bdev turns out to be the smaller part of this. I went looking
> for ways LOCALIO could still CPU-touch the payload after the
> checks pass, and found these:

[...]

> I don't see a clean fix for 1 or 2. The VFS does not have a "DIO
> or fail" knob, and a filesystem's data path isn't visible from the
> block queue. Perhaps for v1, drop LOCALIO for P2PDMA pages and
> bring it back under a filesystem allowlist when the flexfiles
> work needs it.

Thanks for digging into this. I missed the -ENOTBLK fallback and the
btrfs checksums, and you're right that the cover letter overclaims
there.

Agreed, v2 won't use LOCALIO for P2PDMA pages; they go out as a
regular RPC instead. FWIW, the XFS dma-buf direct I/O RFC [1] hits
the same fallbacks and refuses them explicitly for dma-buf iterators,
so that may turn out to be a cleaner way back for LOCALIO than an
allowlist for P2PDMA pages.

> On alignment: yes, fall back to an RPC. Decide inside
> nfs_local_p2pdma_check() from offset, count and
> bdev_logical_block_size(), and return NULL when it doesn't line up.
> The RDMA path has no alignment requirement, so returning -EINVAL
> here makes LOCALIO strictly worse than not having it. That change
> also lets you delete the two duplicated error exits.

Makes sense. I'll do that when LOCALIO comes back, along with the
short write restart you pointed out below.

> Nits:
>
> 8/8 doesn't build with CONFIG_NFS_V4=n. nfs_direct_extract_pages()
> calls pnfs_enabled_sb(), which is only defined inside the
> IS_ENABLED(CONFIG_NFS_V4) block in fs/nfs/pnfs.h, and the #else
> branch has no stub.

Oops, thanks. Will fix.

> In rpcrdma_marshal_req(), a P2PDMA WRITE is forced to
> rpcrdma_readch without checking that head and tail fit the inline
> send threshold. The READ side checks rpcrdma_nonpayload_inline();
> the WRITE side should too, or fall back to a Position Zero Read
> chunk, or return -EREMOTEIO. From code audit.

Agreed. I'll check head and tail against the inline send threshold
and return -EREMOTEIO when they don't fit, same as the READ side.
The non-payload part of an NFS WRITE is small, so a Position Zero
Read chunk doesn't seem worth it for now.

> On a server returning READ data inline despite the Write chunk:
> you're right that RFC 8166 section 4.3.2.2 requires the responder
> to use the offered chunk, so failing the RPC is correct. It does
> surface as a per-RPC "RPC call returned error 121" printk from
> call_status(), though. A tracepoint and a quiet error path would
> be better.

Ack. I'll add a tracepoint in rpcrdma_decode_msg() and keep
-EREMOTEIO quiet in call_status().

> A short LOCALIO write restarts via nfs_local_pgio_restart() with
> offset and count advanced, and the residual can then fail the
> alignment check with -EINVAL after most of the bytes have landed.

Good catch. This goes away with LOCALIO in v2, and I'll keep it in
mind for when it comes back.

> The same three-line P2PDMA guard appears in xs_local, xs_udp and
> xs_tcp send_request. One check in xprt_request_transmit() keyed on
> a transport flag would cover the socket transports and anything
> that comes later.

Ack, that's covered by the capability above.

Thanks,
Praan

[1] https://lore.kernel.org/all/20261016091919.971295-1-anuj1072538@gmail.com/