On 25 Mar 2022 at 03:58, Dave Chinner wrote:
> On Mon, Mar 21, 2022 at 10:47:47AM +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 | 9 ++++++-
>> fs/xfs/libxfs/xfs_bmap.c | 10 ++++++--
>> fs/xfs/libxfs/xfs_inode_fork.c | 27 +++++++++++++++++++++
>> fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
>> fs/xfs/xfs_bmap_item.c | 8 ++++++-
>> fs/xfs/xfs_bmap_util.c | 43 ++++++++++++++++++++++++++++++----
>> fs/xfs/xfs_dquot.c | 9 ++++++-
>> fs/xfs/xfs_iomap.c | 17 ++++++++++++--
>> fs/xfs/xfs_reflink.c | 17 ++++++++++++--
>> fs/xfs/xfs_rtalloc.c | 9 ++++++-
>> 10 files changed, 136 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 23523b802539..6e56aa17fd82 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -776,8 +776,15 @@ 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)
>> + if (error && error != -EFBIG)
>> 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;
>> + }
>
> Neater and more compact to do this by checking explicitly for
> -EFBIG:
>
> 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;
> }
I agree.
>>
>> error = xfs_attr_lookup(args);
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 5a089674c666..0cb915bf8285 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -4524,13 +4524,19 @@ 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)
>> + if (error && error != -EFBIG)
>> goto out_trans_cancel;
>>
>> - xfs_trans_ijoin(tp, ip, 0);
>> + if (error == -EFBIG) {
>> + error = xfs_iext_count_upgrade(tp, ip,
>> + XFS_IEXT_ADD_NOSPLIT_CNT);
>> + if (error)
>> + goto out_trans_cancel;
>> + }
>>
>> 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_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
>> index bb5d841aac58..aff9242db829 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.c
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
>> @@ -756,3 +756,30 @@ xfs_iext_count_may_overflow(
>>
>> return 0;
>> }
>> +
>> +int
>> +xfs_iext_count_upgrade(
>> + struct xfs_trans *tp,
>> + struct xfs_inode *ip,
>> + int nr_to_add)
>
> nr_to_add can only be positive, so should be unsigned.
>
I will change this.
>> +{
>> + if (!xfs_has_large_extent_counts(ip->i_mount) ||
>> + (ip->i_diflags2 & XFS_DIFLAG2_NREXT64) ||
>> + XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS))
>> + return -EFBIG;
>> +
>> + ip->i_diflags2 |= XFS_DIFLAG2_NREXT64;
>> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>> +
>> + /*
>> + * The value of nr_to_add cannot be larger than 2^17
>> + *
>> + * - XFS_MAX_EXTCNT_ATTR_FORK_LARGE - XFS_MAX_EXTCNT_ATTR_FORK_SMALL
>> + * i.e. 2^32 - 2^15
>> + * - XFS_MAX_EXTCNT_DATA_FORK_LARGE - XFS_MAX_EXTCNT_DATA_FORK_SMALL
>> + * i.e. 2^48 - 2^31
>> + */
>> + ASSERT(nr_to_add <= (1 << 17));
>
> That's a comment for the function head and/or the format
> documentation in xfs_format.h, not hidden in the code itself as a
> magic number. i.e. it is a format definition because it is bound by
> on-disk format constants, not by code constratints. Hence this
> should probably be defined in xfs_format.h alongside the large/small
> extent counts, such as:
>
> #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)
>
> And the ASSERT checking the incoming nr_to_add placed right at the
> top of the function because the assert then documents API
> constraints and always catches violations of them.
>
Ok. I will implement the change suggested above.
--
chandan
