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

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

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?

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://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

or I guess the xfs scrub bitmap support code does when you set a range:
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

But as I said before, I don't know if "two mappings retrieved in rapid
succession that overlap" is actually an NFS error.

--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.
> > > 
> > 
>