Re: [PATCH V3 04/10] xfs: Check for extent overflow when adding/removing xattrs

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

On Monday 31 August 2020 10:07:59 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:43AM +0530, Chandan Babu R wrote:
> > Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to be
> > added. One extra extent for dabtree in case a local attr is large enough
> > to cause a double split.  It can also cause extent count to increase
> > proportional to the size of a remote xattr's value.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       | 13 +++++++++++++
> >  fs/xfs/libxfs/xfs_inode_fork.h |  9 +++++++++
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index d4583a0d1b3f..c481389da40f 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -396,6 +396,7 @@ xfs_attr_set(
> >  	struct xfs_trans_res	tres;
> >  	bool			rsvd = (args->attr_filter & XFS_ATTR_ROOT);
> >  	int			error, local;
> > +	int			rmt_blks = 0;
> >  	unsigned int		total;
> >  
> >  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> > @@ -442,11 +443,15 @@ xfs_attr_set(
> >  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> >  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> >  		total = args->total;
> > +
> > +		if (!local)
> > +			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> >  	} else {
> >  		XFS_STATS_INC(mp, xs_attr_remove);
> >  
> >  		tres = M_RES(mp)->tr_attrrm;
> >  		total = XFS_ATTRRM_SPACE_RES(mp);
> > +		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
> >  	}
> >  
> >  	/*
> > @@ -460,6 +465,14 @@ xfs_attr_set(
> >  
> >  	xfs_ilock(dp, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(args->trans, dp, 0);
> > +
> > +	if (args->value || xfs_inode_hasattr(dp)) {
> > +		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> > +				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> 
> What happens if the free space is fragmented and each of these rmt
> blocks results in a separate allocation?

In such a case, We would have "rmt_blks" number of new extents. These are
accounted for by "max(1, rmt_blks)" in XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)
macro. The two arguments to max() i.e. "1" and "rmt_blks" are mutually
exclusive.
"1" occurs when a local value is large enough to cause a double split of a
dabtree leaf.
"rmt_blks" occurs when an xattr value is large enough to be stored
non-locally.

> 
> I'm also not sure why we'd need to account for the remote blocks if
> we're removing an attr?  Those mappings simply go away, right?
>

Lets say the following extent mappings are present in the attr fork of an
inode,
 | Dabtree block | Hole | Dabtree block |

Lets say, to store an xattr's remote value we allocate blocks which cause the
following,
 | Dabtree block | Remote value | Dabtree block |

i.e the region labelled above as "Remote value" is contiguous with both its
neighbours in terms of both "file offset" (belonging to attr fork) and "disk
offset" space. In such a case, xfs_bmap_add_extent_hole_real() would mark the
entire region shown above as just one extent.

A future xattr remove opertion, will fragment this extent into two causing
extent count to increase by 1. Considering the worst case scenario where each
of blocks holding the remote value ends up belonging to such an extent, the
macro XFS_IEXT_ATTR_MANIP_CNT() adds "rmt_blks" number to possible increase in
extent count.

> --D
> 
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> >  	if (args->value) {
> >  		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 2642e4847ee0..aae8e6e80b71 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -45,6 +45,15 @@ struct xfs_ifork {
> >   * i.e. | Old extent | Hole | Old extent |
> >   */
> >  #define XFS_IEXT_REMOVE_CNT		(1)
> > +/*
> > + * Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to
> > + * be added. One extra extent for dabtree in case a local attr is
> > + * large enough to cause a double split.  It can also cause extent
> > + * count to increase proportional to the size of a remote xattr's
> > + * value.
> > + */
> > +#define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
> > +	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
> >  
> >  /*
> >   * Fork handling.
> 

-- 
chandan