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. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
