Re: [PATCH V4 7/7] xfs: Fix log reservation calculation for xattr insert operation

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

On Sunday, February 23, 2020 3:21 PM Amir Goldstein wrote: 
> On Sun, Feb 23, 2020 at 9:28 AM Chandan Rajendra
> <chandanrlinux@gmail.com> 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 by count, block number, rmap 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.
> >
> > Due to the reasons mentioned above, 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 changes xfs_attr_calc_size() to also
> > calculate the number of dabtree blocks that need to be logged.
> >
> > xfs_attr_set() uses the following values computed by xfs_attr_calc_size()
> > 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.
> >
> > ... to compute number of bytes that need 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>
> 
> Hi Chandan,
>
Hi Amir,

> It would have been useful to get this sort of overview in a cover
> letter instead of
> having to find it in the last patch.
>
Sure, I will re-send the patchset with a cover letter.

> I suppose it is a coincident that this work ended up in our mailboxes together
> with Allison's delayed attr work, but it is interesting to know if the two works
> affect each other in any way.

My patchset fixes bugs in the calculation of log space reservation for "xattr
set" operation. I read the code at
"https://github.com/allisonhenderson/xfs_work.git delay_ready_attrs_v7" and
found that the bugs still exist in that code as well. So IMHO, the two
patchsets do not mutually exclude each other.

-- 
chandan