Re: [PATCH V9 16/19] xfs: Conditionally upgrade existing inodes to use large extent counters

["Darrick J. Wong" <djwong@kernel.org>]

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@oracle.com>
> > ---
> >  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@fromorbit.com