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

[Dai Ngo <dai.ngo@oracle.com>]

On 5/19/26 7:59 AM, Darrick J. Wong wrote:
> On Tue, May 19, 2026 at 06:44:18AM -0700, Dai Ngo wrote:
>> On 5/18/26 11:30 PM, Christoph Hellwig wrote:
>>> On Mon, May 18, 2026 at 12:55:17PM -0700, Dai Ngo wrote:
>>>> As shown, the file map changes. Entry# 7 and 8 before the NFSD calls
>>>> merged into entry#7 after the calls. So there must be some activities
>>>> that cause the map to change. I don't know whether the activities were
>>>> triggered by NFS or something in XFS or the block device layer.
>>> Hmm.  We only insert layouts (and search for conflicts) after calling
>>> ->proc_layoutget.  So this might be racy against unwritten extent
>>> conversion, or other writers, which is a bit of an issue.  I guess
>>> we need to fix nfsd4_layoutget to insert an in-progress layout before
>>> calling into ->layouget.
>> I can't quite see the race condition regarding layoutget here. Could you
>> please explain?
> /me has no idea about nfs layouts but has thoughts anyway :)
>
> 1) xfs_fs_map_blocks only takes i_rwsem and XFS_ILOCK; it doesn't take
> the mmap invalidation lock.  Does that mean that pagefaults could wander
> in and mess with the layout?
>
> (I think the race that Christoph is referring to here is that any other
> thread can wander in and change the mapping after a ->map_blocks call
> returns, because nfs isn't holding any kind of lock on the inode)

I think this is what happened.

Assuming the current map of the file as follow:

   0000-4095: IOMAP_UNWRITTEN - block allocated
   4096-8191: IOMAP_HOLE - no block allocated
   8192-12287: IOMAP_UNWRITTEN - block allocated

NOTE:
    . the bmapi_flags used by xfs_bmapi_read is set to XFS_BMAPI_ENTIRE.
    . the nimaps count is set to 1. xfs_bmapi_read can return only 1 extent.

1st call from nfsd to xfs_fs_map_blocks(), specifying offset 0, length 12288 and
write is true.

Extent Returned:

   - offset: 0
   - length: 4096 bytes (one filesystem block)
   - type: IOMAP_UNWRITTEN (existing preallocated extent)
   - addr: physical block number of the first unwritten extent.

2nd call from nfsd to xfs_fs_map_blocks(), specifying offset 4096, length 8192
and write is true.

Returned Extent:

   - offset: 4096
   - length: 4096 bytes (one filesystem block)
   - type: IOMAP_UNWRITTEN (freshly allocated unwritten extent)
   - addr: physical block number assigned when filling the hole at 4096

3rd call from nfsd to xfs_fs_map_blocks(), specifying offset 8192, length
4096 and write is true.

Returned Extent

   - offset: 0
   - length: 12288 bytes (three contiguous blocks)
   - type: IOMAP_UNWRITTEN
   - addr: physical address of the merged unwritten allocation

After the second call, filling the hole merged the three unwritten segments
into a single extent (br_startoff = 0, br_blockcount = 3). When the third call
looks up the range starting at 8192, xfs_bmapi_read() finds that single extent.
Because XFS_BMAPI_ENTIRE is set, xfs_bmapi_trim_map() bypasses trimming and
returns the full extent to xfs_bmbt_to_iomap().

-Dai

>
> 2) Now that NFS apparently can serve up multiple mappings at once,
> should ->map_blocks pass in an array element count so that we can do
> multiple iomaps in a single lock cycle?
>
> 3) Do the reflink and realtime inode checks need to be re-assessed after
> grabbing the ilock since they can change?
>
> --D
>
>>>> However, based on this data I think it's better to change the bmapi_flags
>>>> from XFS_BMAPI_ENTIRE to '0' to address the overlap issue.
>>> We absolutely should be doing that as I said from my first reply.
>>> Still trying to understand what is going on here at a higher level,
>>> though.
>> I'll resubmit the patch series with both fixes combined: the uninitialized
>> imap handling in the error path and the replacement of XFS_BMAPI_ENTIRE
>> with 0.
>>
>> Thanks,
>> -Dai
>>
>>