On 25 Mar 2022 at 11:32, Chandan Babu R wrote: > On 25 Mar 2022 at 03:44, Dave Chinner wrote: >> On Mon, Mar 21, 2022 at 10:47:46AM +0530, Chandan Babu R wrote: >>> The maximum file size that can be represented by the data fork extent counter >>> in the worst case occurs when all extents are 1 block in length and each block >>> is 1KB in size. >>> >>> With XFS_MAX_EXTCNT_DATA_FORK_SMALL representing maximum extent count and with >>> 1KB sized blocks, a file can reach upto, >>> (2^31) * 1KB = 2TB >>> >>> This is much larger than the theoretical maximum size of a directory >>> i.e. 32GB * 3 = 96GB. >>> >>> Since a directory's inode can never overflow its data fork extent counter, >>> this commit replaces checking the return value of >>> xfs_iext_count_may_overflow() with calls to ASSERT(error == 0). >> >> I'd really prefer that we don't add noise like this to a bunch of >> call sites. If directories can't overflow the extent count in >> normal operation, then why are we even calling >> xfs_iext_count_may_overflow() in these paths? i.e. an overflow would >> be a sign of an inode corruption, and we should have flagged that >> long before we do an operation that might overflow the extent count. >> >> So, really, I think you should document the directory size >> constraints at the site where we define all the large extent count >> values in xfs_format.h, remove the xfs_iext_count_may_overflow() >> checks from the directory code and replace them with a simple inode >> verifier check that we haven't got more than 100GB worth of >> individual extents in the data fork for directory inodes.... >> >> Then all this directory specific "can't possibly overflow" overflow >> checks can go away completely. The best code is no code :) > > I had retained the directory extent count overflow checks for the sake of > completeness i.e. The code checks for overflow before every fs operation that > could cause extent count to increase. However, I think your suggestion makes > more sense. I will include this change in the next version of the patchset. Also, Removing directory extent counter overflow checks would also mean that the test xfs/533 (Verify that XFS does not cause inode fork's extent count to overflow when adding/removing directory entries) has to be removed as well. I will post a patch to do the same if no objections are raised. -- chandan
