On Thu, Apr 07, 2022 at 11:22:25AM +1000, Dave Chinner 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;
> > + }
> > }
>
> 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
Nit: missing object?
"...more than a few tens of extents, so if we get..."
--D
> * 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
