Re: [PATCH 3/6] xfs: xfs_itruncate_extents has no extent count limitation

[Chandan Babu R <chandanrlinux@gmail.com>]

On 31 May 2021 at 18:25, Chandan Babu R wrote:
> On 27 May 2021 at 10:21, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> Ever since we moved to freeing of extents by deferred operations,
>> we've already freed extents via individual transactions. Hence the
>> only limitation of how many extents we can mark for freeing in a
>> single xfs_bunmapi() call bound only by how many deferrals we want
>> to queue.
>>
>> That is xfs_bunmapi() doesn't actually do any AG based extent
>> freeing, so there's no actually transaction reservation used up by
>> calling bunmapi with a large count of extents to be freed. RT
>> extents have always been freed directly by bunmapi, but that doesn't
>> require modification of large number of blocks as there are no
>> btrees to split.
>>
>> Some callers of xfs_bunmapi assume that the extent count being freed
>> is bound by geometry (e.g. directories) and these can ask bunmapi to
>> free up to 64 extents in a single call. These functions just work as
>> tehy stand, so there's no reason for truncate to have a limit of
>> just two extents per bunmapi call anymore.
>>
>> Increase XFS_ITRUNC_MAX_EXTENTS to 64 to match the number of extents
>> that can be deferred in a single loop to match what the directory
>> code already uses.
>>
>> For realtime inodes, where xfs_bunmapi() directly frees extents,
>> leave the limit at 2 extents per loop as this is all the space that
>> the transaction reservation will cover.
>>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> ---
>>  fs/xfs/xfs_inode.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 0369eb22c1bb..db220eaa34b8 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -40,9 +40,18 @@ kmem_zone_t *xfs_inode_zone;
>>
>>  /*
>>   * Used in xfs_itruncate_extents().  This is the maximum number of extents
>> - * freed from a file in a single transaction.
>> + * we will unmap and defer for freeing in a single call to xfs_bunmapi().
>> + * Realtime inodes directly free extents in xfs_bunmapi(), so are bound
>> + * by transaction reservation size to 2 extents.
>>   */
>> -#define	XFS_ITRUNC_MAX_EXTENTS	2
>> +static inline int
>> +xfs_itrunc_max_extents(
>> +	struct xfs_inode	*ip)
>> +{
>> +	if (XFS_IS_REALTIME_INODE(ip))
>> +		return 2;
>> +	return 64;
>> +}
>>
>>  STATIC int xfs_iunlink(struct xfs_trans *, struct xfs_inode *);
>>  STATIC int xfs_iunlink_remove(struct xfs_trans *, struct xfs_inode *);
>> @@ -1402,7 +1411,7 @@ xfs_itruncate_extents_flags(
>>  	while (unmap_len > 0) {
>>  		ASSERT(tp->t_firstblock == NULLFSBLOCK);
>>  		error = __xfs_bunmapi(tp, ip, first_unmap_block, &unmap_len,
>> -				flags, XFS_ITRUNC_MAX_EXTENTS);
>> +				flags, xfs_itrunc_max_extents(ip));
>>  		if (error)
>>  			goto out;
>
> The list of free extent items at xfs_defer_pending->dfp_work could
> now contain XFS_EFI_MAX_FAST_EXTENTS (i.e. 16) entries in the worst case.
>
> For a single transaction, xfs_calc_itruncate_reservation() reserves space for
> logging only 4 extents (i.e. 4 exts * 2 trees * (2 * max depth - 1) * block
> size).

... Sorry, I meant to say "xfs_calc_itruncate_reservation() reserves log space
required for freeing 4 extents ..."

> But with the above change, a single transaction can now free upto 16
> extents. Wouldn't this overflow the reserved log space?


-- 
chandan