Re: [PATCH 1/1] xfs: fix overlapping extents returned for pNFS LAYOUTGET

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, May 14, 2026 at 10:19:14AM -0700, Dai Ngo wrote:
> 
> On 5/13/26 5:25 PM, Darrick J. Wong wrote:
> > On Wed, May 13, 2026 at 10:28:31AM -0700, Dai Ngo wrote:
> > > Hi Christoph,
> > > 
> > > On 5/13/26 8:50 AM, Dai Ngo wrote:
> > > > On 5/13/26 12:01 AM, Christoph Hellwig wrote:
> > > > > On Tue, May 12, 2026 at 10:21:53AM -0700, Dai Ngo wrote:
> > > > > > A single LAYOUTGET request from the client can cause the server to
> > > > > > issue multiple calls to xfs_fs_map_blocks() for different offsets
> > > > > > within the same extent. Because the use of XFS_BMAPI_ENTIRE flag,
> > > > > > these calls can produce overlapping mappings.
> > > > > > 
> > > > > > As a result, the LAYOUTGET reply sent to the NFS client may contain
> > > > > > overlapping extents. This creates ambiguity in extent selection for a
> > > > > > given file range, which can lead to incorrect device selection,
> > > > > > inconsistent handling of datastate, and ultimately data corruption or
> > > > > > protocol violations on the client side.
> > > > > Please also add a check to the client that catches this and doesn't
> > > > > use the layout that has extents outside the requested range. And maybe
> > > > > warn about it as well.
> > > > The returned extents cover exactly the range requested in the LAYOUTGET
> > > > op. However these extents are overlapping. For example, here is the
> > > > on-the-wire capture of the LAYOUTGET operation and reply showing the
> > > > overlapping extents:
> > > > 
> > > >      Network File System, Ops(3): SEQUENCE, PUTFH, LAYOUTGET
> > > >          [Program Version: 4]
> > > >          [V4 Procedure: COMPOUND (1)]
> > > >          Tag: <EMPTY>
> > > >          minorversion: 2
> > > >          Operations (count: 3): SEQUENCE, PUTFH, LAYOUTGET
> > > >              Opcode: SEQUENCE (53)
> > > >              Opcode: PUTFH (22)
> > > >              Opcode: LAYOUTGET (50)
> > > >                  layout available?: No
> > > >                  layout type: LAYOUT4_SCSI (5)
> > > >                  IO mode: IOMODE_RW (2)
> > > >                  offset: 122880
> > > >                  length: 65536
> > > >                  min length: 4096
> > > >                  StateID
> > > >                  maxcount: 4096
> > > >          [Main Opcode: LAYOUTGET (50)]
> > > >          Network File System, Ops(3): SEQUENCE PUTFH LAYOUTGET
> > > >          [Program Version: 4]
> > > >          [V4 Procedure: COMPOUND (1)]
> > > >          Status: NFS4_OK (0)
> > > >          Tag: <EMPTY>
> > > >          Operations (count: 3)
> > > >              Opcode: SEQUENCE (53)
> > > >              Opcode: PUTFH (22)
> > > >              Opcode: LAYOUTGET (50)
> > > >                  Status: NFS4_OK (0)
> > > >                  return on close?: Yes
> > > >                  StateID
> > > >                  Layout Segment (count: 1)
> > > >                      offset: 122880
> > > >                      length: 77824
> > > >                      IO mode: IOMODE_RW (2)
> > > >                      layout type: LAYOUT4_SCSI (5)
> > > >                      SCSI Extents (count: 2)
> > > >                          extent 0
> > > >                              device ID: 01000000000000000000000000000000
> > > >                              file offset: 122880
> > > >                              length: 53248
> > > >                              volume offset: 339460096
> > > >                              extent state: INVALID_DATA (2)
> > > >                          extent 1
> > > >                              device ID: 01000000000000000000000000000000
> > > >                              file offset: 122880
> > > >                              length: 77824
> > > >                              volume offset: 339460096
> > > >                              extent state: INVALID_DATA (2)
> > > >          [Main Opcode: LAYOUTGET (50)]
> > > After reviewing ext_tree_insert(), with assist from Codex, I think this
> > > function handles overlapping extents properly. The only issue I see in
> > > ext_tree_insert() is the accuracy of the return error code, EINVAL instead
> > > of ENOMEM, when kmemdup() fails.
> > > 
> > > Since ext_tree_insert seems to handle overlapping extents fine, do you
> > > think it's worth it to fix xfs_fs_map_blocks() to avoid returning overlap
> > > extents?
> > > 
> > > IMHO, I think we still should fix xfs_fs_map_blocks() to avoid any overhead
> > > and complication in ext_tree_insert having to handle overlapping extents.
> > I don't know enough about the nfs blocklayout code to say for sure, but
> > it seems like you want to upsert the mapping returned by
> > xfs_fs_map_blocks into the "ext_tree" right?
> 
> This is currently done on the NFS client side by ext_tree_insert(). The
> question I have is should we enhance the server side by passing '0' to
> xfs_fs_map_blocks() so the client does not have to do the work of
> handling the overlap extents.

Oh, ok.

Not passing XFS_BMAPI_ENTIRE into bmapi_read sounds fine to me.

> > 
> > And by "upsert" I mean "clear out any mappings for the (offset, length)
> > range, then insert the new mapping", sort of like what the fuse iomap
> > cache does:
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/tree/fs/fuse/fuse_iomap_cache.c?h=fuse-iomap-cache_2026-05-07*n1682__;Iw!!ACWV5N9M2RV99hQ!LP7Lgbj10myml6rtWPCyBcfoKlvvpS39fLQ4Dy8puJ9c8ZQbxV6ToyYupyVa8TrICy--mS-sUwGxrA$
> > 
> > or I guess the xfs scrub bitmap support code does when you set a range:
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/tree/fs/xfs/scrub/bitmap.c?h=fuse-iomap-cache_2026-05-07*n395__;Iw!!ACWV5N9M2RV99hQ!LP7Lgbj10myml6rtWPCyBcfoKlvvpS39fLQ4Dy8puJ9c8ZQbxV6ToyYupyVa8TrICy--mS9n7W3nLw$
> > 
> > But as I said before, I don't know if "two mappings retrieved in rapid
> > succession that overlap" is actually an NFS error.
> 
> As far as I can tell,|nfsd4_block_proc_layoutget()| correctly advances the
> offset and decrements the length after each call to|nfsd4_block_map_extent()|,
> which passes the arguments through verbatim to|xfs_fs_map_blocks()|.

<nod>

--D

> -Dai
> 
> > 
> > --D
> > 
> > > -Dai
> > > 
> > > > -Dai
> > > > 
> > > > > > Also drop the check for (!error) since it was checked after call to
> > > > > > xfs_bmapi_read().
> > > > > > 
> > > > > > Fixes: cc6c40e09d7b1 ("NFSD/blocklayout: Support multiple
> > > > > > extents per LAYOUTGET").
> > > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > > > > ---
> > > > > >    fs/xfs/xfs_pnfs.c | 6 +++---
> > > > > >    1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > - This patch is based on top of the patch:
> > > > > >     xfs: fix use of uninitialized imap in xfs_fs_map_blocks error path
> > > > > The error changes should go into that patch, so please resend it with
> > > > > that fixes.  Maybe as a series together with this patch to keep them
> > > > > together.
> > > > > 
> > > > > > @@ -172,6 +172,7 @@ xfs_fs_map_blocks(
> > > > > >        offset_fsb = XFS_B_TO_FSBT(mp, offset);
> > > > > >          lock_flags = xfs_ilock_data_map_shared(ip);
> > > > > > +    bmapi_flags = 0;    /* return map for requested range only */
> > > > > Just remove the variable and hard code the 0 in the xfs_bmapi_read call.
> > > > > 
>