Re: [PATCH V3 2/2] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra <chandan@linux.ibm.com>]

On Wednesday, February 12, 2020 8:43 PM Brian Foster wrote: 
> On Wed, Jan 29, 2020 at 10:29:39AM +0530, Chandan Rajendra wrote:
> > Log space reservation for xattr insert operation can be divided into two
> > parts,
> > 1. Mount time
> >    - Inode
> >    - Superblock for accounting space allocations
> >    - AGF for accounting space used be count, block number, rmapbt and refcnt
> >      btrees.
> > 
> > 2. The remaining log space can only be calculated at run time because,
> >    - A local xattr can be large enough to cause a double split of the dabtree.
> >    - The value of the xattr can be large enough to be stored in remote
> >      blocks. The contents of the remote blocks are not logged.
> > 
> >    The log space reservation could be,
> >    - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
> >      number of blocks are required if xattr is large enough to cause another
> >      split of the dabtree path from root to leaf block.
> >    - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
> >      entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
> >      case of a double split of the dabtree path from root to leaf blocks.
> >    - Space for logging blocks of count, block number, rmap and refcnt btrees.
> > 
> > Presently, mount time log reservation includes block count required for a
> > single split of the dabtree. The dabtree block count is also taken into
> > account by xfs_attr_calc_size().
> > 
> > Also, AGF log space reservation isn't accounted for. Hence log reservation
> > calculation for xattr insert operation gives an incorrect value.
> > 
> > Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > 
> > To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
> > 1. The number of dabtree blocks that need to be logged.
> > 2. The number of remote blocks that need to be allocated.
> > 3. The number of dabtree blocks that need to be allocated.
> > 4. The number of bmbt blocks that need to be allocated.
> > 5. The total number of blocks that need to be allocated.
> > 
> > xfs_attr_set() uses this information to compute number of bytes that needs to
> > be reserved in the log.
> > 
> > This commit also modifies xfs_log_calc_max_attrsetm_res() to invoke
> > xfs_attr_calc_size() to obtain the number of blocks to be logged which it uses
> > to figure out the total number of bytes to be logged.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> > Changelog:
> > V1 -> V2:
> > 1. Use convenience variables to reduce indentation of code.
> > 
> > V2 -> V3:
> > 1. Introduce 'struct xfs_attr_set_resv' to be used an as out parameter
> >    holding xattr reservation values.
> > 2. Calculate number of bmbt blocks and total allocation blocks within
> >    xfs_attr_calc_size().
> > 
> >  fs/xfs/libxfs/xfs_attr.c       | 93 +++++++++++++++++++---------------
> >  fs/xfs/libxfs/xfs_attr.h       | 20 +++++++-
> >  fs/xfs/libxfs/xfs_log_rlimit.c | 14 ++---
> >  fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++++++----------
> >  fs/xfs/libxfs/xfs_trans_resv.h |  2 +
> >  5 files changed, 107 insertions(+), 74 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 1eae1db74f6cd..1f3b001a1092e 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> ...
> > @@ -248,6 +211,53 @@ xfs_attr_try_sf_addname(
> >  	return error ? error : error2;
> >  }
> >  
> > +/*
> > + * Calculate how many blocks we need for the new attribute,
> > + */
> > +void
> > +xfs_attr_calc_size(
> > +	struct xfs_mount		*mp,
> > +	struct xfs_attr_set_resv	*resv,
> > +	int				namelen,
> > +	int				valuelen,
> > +	int				*local)
> > +{
> > +	unsigned int		blksize;
> > +	int			size;
> > +
> > +	blksize = mp->m_dir_geo->blksize;
> > +
> > +	/*
> > +	 * Determine space new attribute will use, and if it would be
> > +	 * "local" or "remote" (note: local != inline).
> > +	 */
> > +	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
> > +
> > +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > +	resv->log_dablks = 2 * resv->total_dablks;
> > +
> 
> It looks like this changes the setxattr transaction reservation
> calculation at the same time as refactoring how the reservation is
> calculated, which makes it hard to even identify what is changing. Can
> you split the general refactoring and calculation changes into
> independent patches? E.g., refactor the existing calculation first and
> then subsequently fix up the calculation..?

Sure.

I will also rebase my patches on top of Christoph's "Clean attr
interface" patchset and post the next version.

-- 
chandan