On 12 Apr 2022 at 03:37, Darrick J. Wong wrote:
> On Sat, Apr 09, 2022 at 07:17:21PM +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. XFS_DIR2_SPACE_SIZE * 3 = ~96GB.
>>
>> Since a directory's inode can never overflow its data fork extent counter,
>> this commit removes all the overflow checks associated with
>> it. xfs_dinode_verify() now performs a rough check to verify if a diretory's
>> data fork is larger than 96GB.
>>
>> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
>> ---
>> fs/xfs/libxfs/xfs_bmap.c | 20 -------------
>> fs/xfs/libxfs/xfs_da_btree.h | 1 +
>> fs/xfs/libxfs/xfs_da_format.h | 1 +
>> fs/xfs/libxfs/xfs_dir2.c | 2 ++
>> fs/xfs/libxfs/xfs_format.h | 13 ++++++++
>> fs/xfs/libxfs/xfs_inode_buf.c | 3 ++
>> fs/xfs/libxfs/xfs_inode_fork.h | 13 --------
>> fs/xfs/xfs_inode.c | 55 ++--------------------------------
>> fs/xfs/xfs_symlink.c | 5 ----
>> 9 files changed, 22 insertions(+), 91 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 1254d4d4821e..4fab0c92ab70 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -5147,26 +5147,6 @@ xfs_bmap_del_extent_real(
>> * Deleting the middle of the extent.
>> */
>>
>> - /*
>> - * For directories, -ENOSPC is returned since a directory entry
>> - * remove operation must not fail due to low extent count
>> - * availability. -ENOSPC will be handled by higher layers of XFS
>> - * by letting the corresponding empty Data/Free blocks to linger
>> - * until a future remove operation. Dabtree blocks would be
>> - * swapped with the last block in the leaf space and then the
>> - * new last block will be unmapped.
>> - *
>> - * The above logic also applies to the source directory entry of
>> - * a rename operation.
>> - */
>> - error = xfs_iext_count_may_overflow(ip, whichfork, 1);
>> - if (error) {
>> - ASSERT(S_ISDIR(VFS_I(ip)->i_mode) &&
>> - whichfork == XFS_DATA_FORK);
>> - error = -ENOSPC;
>> - goto done;
>> - }
>> -
>> old = got;
>>
>> got.br_blockcount = del->br_startoff - got.br_startoff;
>> diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
>> index 0faf7d9ac241..7f08f6de48bf 100644
>> --- a/fs/xfs/libxfs/xfs_da_btree.h
>> +++ b/fs/xfs/libxfs/xfs_da_btree.h
>> @@ -30,6 +30,7 @@ struct xfs_da_geometry {
>> unsigned int free_hdr_size; /* dir2 free header size */
>> unsigned int free_max_bests; /* # of bests entries in dir2 free */
>> xfs_dablk_t freeblk; /* blockno of free data v2 */
>> + xfs_extnum_t max_extents; /* Max. extents in corresponding fork */
>>
>> xfs_dir2_data_aoff_t data_first_offset;
>> size_t data_entry_offset;
>> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
>> index 5a49caa5c9df..95354b7ab7f5 100644
>> --- a/fs/xfs/libxfs/xfs_da_format.h
>> +++ b/fs/xfs/libxfs/xfs_da_format.h
>> @@ -277,6 +277,7 @@ xfs_dir2_sf_firstentry(struct xfs_dir2_sf_hdr *hdr)
>> * Directory address space divided into sections,
>> * spaces separated by 32GB.
>> */
>> +#define XFS_DIR2_MAX_SPACES 3
>> #define XFS_DIR2_SPACE_SIZE (1ULL << (32 + XFS_DIR2_DATA_ALIGN_LOG))
>> #define XFS_DIR2_DATA_SPACE 0
>> #define XFS_DIR2_DATA_OFFSET (XFS_DIR2_DATA_SPACE * XFS_DIR2_SPACE_SIZE)
>> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
>> index 5f1e4799e8fa..52c764ecc015 100644
>> --- a/fs/xfs/libxfs/xfs_dir2.c
>> +++ b/fs/xfs/libxfs/xfs_dir2.c
>> @@ -150,6 +150,8 @@ xfs_da_mount(
>> dageo->freeblk = xfs_dir2_byte_to_da(dageo, XFS_DIR2_FREE_OFFSET);
>> dageo->node_ents = (dageo->blksize - dageo->node_hdr_size) /
>> (uint)sizeof(xfs_da_node_entry_t);
>> + dageo->max_extents = (XFS_DIR2_MAX_SPACES * XFS_DIR2_SPACE_SIZE) >>
>> + mp->m_sb.sb_blocklog;
>> dageo->magicpct = (dageo->blksize * 37) / 100;
>>
>> /* set up attribute geometry - single fsb only */
>
> Shouldn't we set up mp->m_attr_geo.max_extents too? Even if all we do
> is set it to XFS_MAX_EXTCNT_ATTR_FORK_{SMALL,LARGE}? I get that nothing
> will use it anywhere, but we shouldn't leave uninitialized geometry
> structure variables around.
>
I had left it to be initialized to the value of zero as an indicator that the
field has an invalid value. But I think your suggestion is indeed correct
since we can assign the field with either XFS_MAX_EXTCNT_ATTR_FORK_SMALL or
XFS_MAX_EXTCNT_ATTR_FORK_LARGE. I will post a v9.2 patch soon.
--
chandan
