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

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

On 5/20/26 9:48 AM, Darrick J. Wong wrote:
> On Wed, May 20, 2026 at 08:09:29AM -0700, Dai Ngo wrote:
>> On 5/20/26 1:24 AM, Christoph Hellwig wrote:
>>> [adding Sergey, who wrote the code to allow multiple mappings]
>>>
>>> On Tue, May 19, 2026 at 07:59:49AM -0700, Darrick J. Wong wrote:
>>>> 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?
>>> Looks like it.
>> I already described the scenario in which the file mapping can change
>> between successive calls from nfsd—triggered by a single LAYOUTGET
>> request—to xfs_fs_map_blocks().
>>
>> I also don't see how page faults are relevant in the context of pNFS
>> layouts. The server is not servicing actual data accesses through page
>> faults when constructing a LAYOUTGET response
> Some other process on the nfs server mmaps the file and generates
> faults.

This would cause a layout conflict and this process would be delayed
until the nfs server finishes recalling the layout from the first
client.

>
>>>> 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?
>>> I guess we need to do that, or revert the code to provide multiple maps
>>> for now.
>> I think we should address the immediate problem first by replacing XFS_BMAPI_ENTIRE
>> with 0, and handle support for returning multiple map entries from a single
>> call to xfs_fs_map_blocks() in follow-up patches. That work is more involved,
>> as it requires coordinated changes in both the nfsd and XFS code.
> Agreed.
>
>>>> 3) Do the reflink and realtime inode checks need to be re-assessed after
>>>> grabbing the ilock since they can change?
>>> Yes.
>> I can move the check for reflink and realtime inode inside the ilock in v2
>> of this patch series.
> Yeah, that probably needs to be made too. :)

Thanks! I'll include this change in v2.

-Dai

>
> --D
>
>> -Dai
>>
>>