On 07 Apr 2022 at 07:16, Darrick J. Wong wrote:
> On Wed, Apr 06, 2022 at 11:49:00AM +0530, Chandan Babu R wrote:
>> This commit enables upgrading existing inodes to use large extent counters
>> provided that underlying filesystem's superblock has large extent counter
>> feature enabled.
>>
>> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 10 ++++++++++
>> fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
>> fs/xfs/libxfs/xfs_format.h | 8 ++++++++
>> fs/xfs/libxfs/xfs_inode_fork.c | 19 +++++++++++++++++++
>> fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
>> fs/xfs/xfs_bmap_item.c | 2 ++
>> fs/xfs/xfs_bmap_util.c | 13 +++++++++++++
>> fs/xfs/xfs_dquot.c | 3 +++
>> fs/xfs/xfs_iomap.c | 5 +++++
>> fs/xfs/xfs_reflink.c | 5 +++++
>> fs/xfs/xfs_rtalloc.c | 3 +++
>> 11 files changed, 74 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 23523b802539..66c4fc55c9d7 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -776,8 +776,18 @@ xfs_attr_set(
>> if (args->value || xfs_inode_hasattr(dp)) {
>> error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
>> XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
>> + if (error == -EFBIG)
>> + error = xfs_iext_count_upgrade(args->trans, dp,
>> + XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
>> if (error)
>> goto out_trans_cancel;
>> +
>> + if (error == -EFBIG) {
>> + error = xfs_iext_count_upgrade(args->trans, dp,
>> + XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
>> + if (error)
>> + goto out_trans_cancel;
>> + }
>> }
>>
>> error = xfs_attr_lookup(args);
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 4fab0c92ab70..82d5467ddf2c 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -4524,14 +4524,16 @@ xfs_bmapi_convert_delalloc(
>> return error;
>>
>> xfs_ilock(ip, XFS_ILOCK_EXCL);
>> + xfs_trans_ijoin(tp, ip, 0);
>>
>> error = xfs_iext_count_may_overflow(ip, whichfork,
>> XFS_IEXT_ADD_NOSPLIT_CNT);
>> + if (error == -EFBIG)
>> + error = xfs_iext_count_upgrade(tp, ip,
>> + XFS_IEXT_ADD_NOSPLIT_CNT);
>> if (error)
>> goto out_trans_cancel;
>>
>> - xfs_trans_ijoin(tp, ip, 0);
>> -
>> if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &bma.icur, &bma.got) ||
>> bma.got.br_startoff > offset_fsb) {
>> /*
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 43de892d0305..bb327ea43ca1 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -934,6 +934,14 @@ enum xfs_dinode_fmt {
>> #define XFS_MAX_EXTCNT_DATA_FORK_SMALL ((xfs_extnum_t)((1ULL << 31) - 1))
>> #define XFS_MAX_EXTCNT_ATTR_FORK_SMALL ((xfs_extnum_t)((1ULL << 15) - 1))
>>
>> +/*
>> + * This macro represents the maximum value by which a filesystem operation can
>> + * increase the value of an inode's data/attr fork extent count.
>> + */
>> +#define XFS_MAX_EXTCNT_UPGRADE_NR \
>> + min(XFS_MAX_EXTCNT_ATTR_FORK_LARGE - XFS_MAX_EXTCNT_ATTR_FORK_SMALL, \
>> + XFS_MAX_EXTCNT_DATA_FORK_LARGE - XFS_MAX_EXTCNT_DATA_FORK_SMALL)
>> +
>> /*
>> * Inode minimum and maximum sizes.
>> */
>> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
>> index bb5d841aac58..1245e9f1ca81 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.c
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
>> @@ -756,3 +756,22 @@ xfs_iext_count_may_overflow(
>>
>> return 0;
>> }
>> +
>> +int
>> +xfs_iext_count_upgrade(
>
> Hmm. I think the @nr_to_add parameter is supposed to be the one
> that caused xfs_iext_count_may_overflow to return -EFBIG, right?
>
> I was about to comment that it would be really helpful to have a comment
> above this function dropping a hint that this is the case:
>
> /*
> * Upgrade this inode's extent counter fields to be able to handle a
> * potential increase in the extent count by this number. Normally
> * this is the same quantity that caused xfs_iext_count_may_overflow to
> * return -EFBIG.
> */
> int
> xfs_iext_count_upgrade(...
>
> ...though I worry that this will cause fatal warnings about the
> otherwise unused parameter on non-debug kernels?
> I'm not sure why it
> matters that nr_to_add is constrained to a small value? Is it just to
> prevent obviously huge values? AFAICT all the current callers pass in
> small #defined integer values.
>
> That said, if the assert here is something Dave asked for in a previous
> review, then I won't stand in the way:
It was me who added the call to ASSERT() in V7 of the patchset. This was done
to catch any unintentional programming errors.
Also, I found out today that Linux kernel build does not pass either
-Wunused-parameter or -Wextra options to the compiler
(https://www.spinics.net/lists/newbies/msg63816.html). Passing either of those
compiler options causes several "unused parameter" warnings across the kernel
source code. So we should never see warnings about nr_to_add being unused on
non-debug kernels.
>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>
Thanks for the review! I will include the comment for xfs_iext_count_upgrade()
that you have suggested above and I will also replace the open code that you
have pointed out (in the your next mail) with a call to
xfs_inode_has_large_extent_counts().
--
chandan
