Re: [PATCH] xfs: Allow scrub to detect inodes with non-maximal sized extents

[Chandan Babu R <chandanrlinux@gmail.com>]

On 24 Feb 2021 at 02:00, Dave Chinner wrote:
> On Tue, Feb 23, 2021 at 01:56:29PM +0530, Chandan Babu R wrote:
>> This commit now makes it possible for scrub to check if an inode's extents are
>> maximally sized i.e. it checks if an inode's extent is contiguous (in terms of
>> both file offset and disk offset) with neighbouring extents and the total
>> length of both the extents is less than the maximum allowed extent
>> length (i.e. MAXEXTLEN).
>
> It took me a while to understand that what this is actually doing
> (had to read the code because I couldn't work out what this meant).
> Essentially, it is determining if two extents that are physically
> and logically adjacent were not merged together into a single extent
> when the combined size of the two extents would fit into a single
> extent record.
>
> I'm not sure this is an issue - it most definitely isn't corruption
> as nothing will have any problems looking up either extent, nor
> modifying or removing either extent. It's not ideal, but it isn't
> corruption.
>
> I can see how it would come about, too, because extent removal
> doesn't merge remaining partial extents.
>
> That is, create a long written extent in a file, then use fallocate
> to allocate an adjacent extent that puts the two extents over
> MAXEXTLEN. Now we have two phsyically and logically adjacent extents
> that only differ by state. Now do a single write that converts the
> entire unwritten extent to written so no merging occurs during the
> state conversion.  Now punch out the far end of the second extent.
>
> This ends up in xfs_bmap_del_extent_real(), which simply removes the
> end of the second extent. It does not look up the previous extent
> and attempt to merge the remaining part of the second extent into
> the previous adjacent extent.
>
> Hence, at this point, we have two logically and physically adjacent
> extents whose combined length is less than MAXLEN. This patch will
> now signal that as corruption, which is wrong.

Ah ok. Thanks for explaining the scenario which could lead to a false positive
report because of the code changes in this patch.

--
chandan