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

[Chandan Babu R <chandanrlinux@gmail.com>]

On Wednesday 19 August 2020 3:27:46 AM IST Darrick J. Wong wrote:
> 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)
>

That is perfect. Thanks for the suggestion. I will add that in the next
version of this patchset.

-- 
chandan