Re: [PATCH V7 14/17] xfs: Conditionally upgrade existing inodes to use 64-bit extent counters

Chandan Babu R
2022-03-07T10:57:35Z

On 07 Mar 2022 at 10:32, Dave Chinner wrote:
> On Sat, Mar 05, 2022 at 06:15:15PM +0530, Chandan Babu R wrote:
>> On 04 Mar 2022 at 13:21, Dave Chinner wrote:
>> > On Tue, Mar 01, 2022 at 04:09:35PM +0530, Chandan Babu R wrote:
>> >> This commit upgrades inodes to use 64-bit extent counters when they are read
>> >> from disk. Inodes are upgraded only when the filesystem instance has
>> >> XFS_SB_FEAT_INCOMPAT_NREXT64 incompat flag set.
>> >>
>> >> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> .....
>> >> + return xfs_iext_count_may_overflow(ip, whichfork, nr_to_add);
>> >
>> > If the answer is so we don't cancel a dirty transaction here, then
>> > I think this check needs to be more explicit - don't even try to do
>> > the upgrade if the number of extents we are adding will cause an
>> > overflow anyway.
>> >
>> > As it is, wouldn't adding 2^47 - 2^31 extents in a single hit be
>> > indicative of a bug? We can only modify the extent count by a
>> > handful of extents (10, maybe 20?) at most in a single transaction,
>> > so why do we even need this check?
>>
>> Yes, the above call to xfs_iext_count_may_overflow() is not correct. The value
>> of nr_to_add has to be larger than 2^17 (2^32 - 2^15 for attr fork and 2^48 -
>> 2^31 for data fork) for extent count to overflow. Hence, I will remove this
>> call to xfs_iext_count_may_overflow().
>
> Would it be worth putting an assert somewhere with this logic in it?
> That way we at least capture such bugs in debug settings and protect
> ourselves from unintentional mistakes.
>

Sure. I will add an ASSERT() call to check if we ever add more than 2^17
extents in a single modification of an inode's data/attr fork extent count.

--
chandan