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

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

On Tue, May 12, 2026 at 10:21:53AM -0700, Dai Ngo wrote:
> xfs_fs_map_blocks() currently passes XFS_BMAPI_ENTIRE to xfs_bmapi_read(),
> which causes the bmap code to expand the mapping to cover the entire
> extent rather than the requested range.

Nitpicking: _ENTIRE causes bmapi_read to return the whole extent instead
of trimming it down to the requested range.

> 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.
> 
> Problem discovered with xfstest generic/075 test using NFSv4.2 mount
> with SCSI layout.

Might be helpful to provide an example of the request vs. the
overlapping layouts.  IIRC the client asks for a layout for the first
32 fsblocks of the file.  On the first call to xfs_fs_map_blocks, block
0 is a single unwritten mapping, so that gets returned.

Meanwhile, another thread fallocates block 2 and gets lucky in that an
adjacent block is free, so the first mapping in the file is now 2
unwritten fsblocks starting at 0.  This can happen because we don't hold
i_rwsem (or the ILOCK) between calls to ->map_blocks.

Returning to the first thread, it calls xfs_fs_map_blocks again to map
block 1.  However, the mapping's been changed, so we now return the
entire 2-fsblock  mapping.  What gets sent to the client is

{.offset = 0, .length = 4096, .addr = X, .dev = Y},
{.offset = 0, .length = 8192, .addr = X, .dev = Y},

and the client rejects that as overlapping.  Right?

> Fix this by replacing the XFS_BMAPI_ENTIRE flag with '0' so that
> xfs_bmapi_read() returns only the mapping for the requested range.
> 
> Also drop the check for (!error) since it was checked after call to
> xfs_bmapi_read().

Cc: <stable@vger.kernel.org> # v6.19

> 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
> 
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index f7c6dba3d21e..697bf3e4ad7e 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -118,7 +118,7 @@ xfs_fs_map_blocks(
>  	struct xfs_bmbt_irec	imap;
>  	xfs_fileoff_t		offset_fsb, end_fsb;
>  	loff_t			limit;
> -	int			bmapi_flags = XFS_BMAPI_ENTIRE;
> +	int			bmapi_flags;

Why not just replace the argument to xfs_bmapi_read with a constant
zero?

--D

>  	int			nimaps = 1;
>  	uint			lock_flags;
>  	int			error = 0;
> @@ -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 */
>  	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
>  				&imap, &nimaps, bmapi_flags);
>  	if (error) {
> @@ -182,8 +183,7 @@ xfs_fs_map_blocks(
>  
>  	ASSERT(!nimaps || imap.br_startblock != DELAYSTARTBLOCK);
>  
> -	if (!error && write &&
> -	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
> +	if (write && (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
>  		if (offset + length > XFS_ISIZE(ip))
>  			end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
>  		else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
> -- 
> 2.47.3
> 
>