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

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

On Monday 17 August 2020 12:23:07 PM IST 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?

I agree. Making xfs_iext_count_may_overflow() to always return success in the
case of CoW fork would mean that the if condition can be removed and hence
makes the code more readable.

> 
> > +		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.

The reason to do this was to consolidate the comment descriptions at one
place. For e.g. the comment for XFS_IEXT_DIR_MANIP_CNT (from "[PATCH V2 05/10]
xfs: Check for extent overflow when adding/removing dir entries") is slightly
larger. Using constants (instead of macros) would mean that the same comment
has to be replicated across the 6 locations it is being used.

-- 
chandan