Re: [PATCH V2 02/10] xfs: Check for extent overflow when trivally adding a new extent

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Aug 19, 2020 at 07:49:33AM +1000, Dave Chinner wrote:
> On Mon, Aug 17, 2020 at 07:53:07AM +0100, Christoph Hellwig wrote:
> > On Fri, Aug 14, 2020 at 01:38:25PM +0530, Chandan Babu R wrote:
> > > When adding a new data extent (without modifying an inode's existing
> > > extents) the extent count increases only by 1. This commit checks for
> > > extent count overflow in such cases.
> > > 
> > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_bmap.c       | 8 ++++++++
> > >  fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
> > >  fs/xfs/xfs_bmap_util.c         | 5 +++++
> > >  fs/xfs/xfs_dquot.c             | 8 +++++++-
> > >  fs/xfs/xfs_iomap.c             | 5 +++++
> > >  fs/xfs/xfs_rtalloc.c           | 5 +++++
> > >  6 files changed, 32 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 9c40d5971035..e64f645415b1 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
> > >  		return error;
> > >  
> > >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > +
> > > +	if (whichfork == XFS_DATA_FORK) {
> > 
> > Should we add COW fork special casing to xfs_iext_count_may_overflow
> > instead?

That seems like a reasonable idea.

> > 
> > > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > > +				XFS_IEXT_ADD_CNT);
> > 
> > I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> > for a counter parameter makes a lot more sense to me.
> 
> I explicitly asked Chandan to convert all the magic numbers
> sprinkled in the previous patch to defined values. It was impossible
> to know whether the intended value was correct when it's just an
> open coded number because we don't know what the number actually
> stands for. And, in future, if we change the behaviour of a specific
> operation, then we only have to change a single value rather than
> having to track down and determine if every magic "1" is for an
> extent add operation or something different.

I prefer named flags over magic numbers too, though this named constant
doesn't have a comment describing what it does, and "ADD_CNT" doesn't
really tell me much.  The subsequent patches have comments, so maybe
this should just become:

/*
 * Worst-case increase in the fork extent count when we're adding a
 * single extent to a fork and there's no possibility of splitting an
 * existing mapping.
 */
#define XFS_IEXT_ADD_NOSPLIT	(1)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com