Re: [PATCH V3 01/10] xfs: Add helper for checking per-inode extent count overflow

From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, david@fromorbit.com, hch@infradead.org
Subject: Re: [PATCH V3 01/10] xfs: Add helper for checking per-inode extent count overflow
Date: Tue, 01 Sep 2020 15:14:21 +0530	[thread overview]
Message-ID: <6636871.qDoI5iUcXa@garuda> (raw)
In-Reply-To: <20200831164435.GO6096@magnolia>

On Monday 31 August 2020 10:14:35 PM IST Darrick J. Wong wrote:
> On Mon, Aug 31, 2020 at 09:08:23AM -0700, Darrick J. Wong wrote:
> > On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote:
> > > XFS does not check for possible overflow of per-inode extent counter
> > > fields when adding extents to either data or attr fork.
> > > 
> > > For e.g.
> > > 1. Insert 5 million xattrs (each having a value size of 255 bytes) and
> > >    then delete 50% of them in an alternating manner.
> > > 
> > > 2. On a 4k block sized XFS filesystem instance, the above causes 98511
> > >    extents to be created in the attr fork of the inode.
> > > 
> > >    xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
> > > 
> > > 3. The incore inode fork extent counter is a signed 32-bit
> > >    quantity. However the on-disk extent counter is an unsigned 16-bit
> > >    quantity and hence cannot hold 98511 extents.
> > > 
> > > 4. The following incorrect value is stored in the attr extent counter,
> > >    # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
> > >    core.naextents = -32561
> > > 
> > > This commit adds a new helper function (i.e.
> > > xfs_iext_count_may_overflow()) to check for overflow of the per-inode
> > > data and xattr extent counters. Future patches will use this function to
> > > make sure that an FS operation won't cause the extent counter to
> > > overflow.
> > > 
> > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > 
> > Seems reasonable so far...
> > 
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > --D
> > 
> > > ---
> > >  fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++
> > >  fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
> > >  2 files changed, 25 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > > index 0cf853d42d62..3a084aea8f85 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > > @@ -23,6 +23,7 @@
> > >  #include "xfs_da_btree.h"
> > >  #include "xfs_dir2_priv.h"
> > >  #include "xfs_attr_leaf.h"
> > > +#include "xfs_types.h"
> > >  
> > >  kmem_zone_t *xfs_ifork_zone;
> > >  
> > > @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr(
> > >  
> > >  	return 0;
> > >  }
> > > +
> > > +int
> > > +xfs_iext_count_may_overflow(
> > > +	struct xfs_inode	*ip,
> > > +	int			whichfork,
> > > +	int			nr_to_add)
> > > +{
> > > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> > > +	uint64_t		max_exts;
> > > +	uint64_t		nr_exts;
> > > +
> > > +	if (whichfork == XFS_COW_FORK)
> > > +		return 0;
> > > +
> > > +	max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM;
> > > +
> > > +	nr_exts = ifp->if_nextents + nr_to_add;
> > > +	if (nr_exts < ifp->if_nextents || nr_exts > max_exts)
> > > +		return -EFBIG;
> 
> Something I thought of after the fact -- can you add a new fault
> injection point to lower the max extent count?  That way we can
> facilitate the construction of fstests cases to check the operation of
> the new predicate without having to spend lots of time constructing huge
> fragmented files.

Sure, I will do that.

> 
> (There /are/ test cases somewhere, riiight? ;))

Apart from executing xfstests, I had tested the patchset with the use case
described in the commit message of this patch. But with an error injection
facility available, it should be easier to add tests to fstests. I will work
on that. Thanks for the suggestion.

> 
> No need to add it here, you can tack it onto the end of the series as a
> new patch.
> 
> --D
> 
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > > index a4953e95c4f3..0beb8e2a00be 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > > @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip);
> > >  
> > >  int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> > >  int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
> > > +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork,
> > > +		int nr_to_add);
> > >  
> > >  #endif	/* __XFS_INODE_FORK_H__ */
> 

-- 
chandan