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@xxxxxxxxxx> >> >> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx> > ..... >> >> + 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
