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;
> + }
> }
Did you forgot to remove the original xfs_iext_count_upgrade() call?
> 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)
You don't need to write "This macro represents" in a comment above
the macro that that the comment is describing. If you need to refer
to the actual macro, use it's name directly.
As it is, the comment could be improved:
/*
* When we upgrade an inode to the large extent counts, the maximum
* value by which the extent count can increase is bound by the
* change in size of the on-disk field. No upgrade operation should
* ever be adding more than a few tens of, so if we get a really
* large value it is a sign of a code bug or corruption.
*/
#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)
Otherwise it looks OK.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
