Re: [PATCH V9.1] xfs: Directory's data fork extent counter can never overflow

[Chandan Babu R <chandan.babu@oracle.com>]

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@oracle.com>
>> ---
>>  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