Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

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

On Thursday, February 27, 2020 5:23 PM Brian Foster wrote: 
> On Thu, Feb 27, 2020 at 02:29:16PM +0530, Chandan Rajendra wrote:
> > On Wednesday, February 26, 2020 10:12 PM Brian Foster wrote: 
> > > On Wed, Feb 26, 2020 at 08:33:12PM +0530, Chandan Rajendra wrote:
> > > > On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> > > > > On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > > > > > The number of Bmbt blocks that is required can be calculated only once by
> > > > > > passing the sum of total number of dabtree blocks and remote blocks to
> > > > > > XFS_NEXTENTADD_SPACE_RES() macro.
> > > > > > 
> > > > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > > > ---
> > > > > 
> > > > > According to the cover letter this is fixing a reservation calculation
> > > > > issue, though the commit log kind of gives the impression it's a
> > > > > refactor. Can you elaborate on what this fixes in the commit log?
> > > > > 
> > > > 
> > > > XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
> > > > needed for mapping the 'block count' passed to it as the second argument.
> > > > When calculating the number of leaf blocks, it accommodates the 'block count'
> > > > argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
> > > > group it decides that a bmbt leaf block is required. For each of the leaf
> > > > blocks that needs to be allocated, it assumes that there will be a split of
> > > > the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
> > > > with the maximum height of the tree.
> > > > 
> > > > With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
> > > > through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
> > > > is for remote attr blocks) we miss out on the opportunity to group the bmbt
> > > > leaf blocks and hence overcompensate on the bmbt blocks calculation.
> > > > 
> > > > Please let me know if my understanding is incorrect.
> > > > 
> > > 
> > > Ok, thanks. I think I follow the intent. This patch is actually intended
> > > to reduce block reservation by simplifying this calculation, right?
> > 
> > I noticed xfs/132 test failing when I had changed the code to have 32-bit
> > xattr extent counter. The corresponding mount failure was due to log size
> > checks failing in xfs_log_mount(). The difference in value returned by
> > xfs_log_calc_minimum_size() => xfs_log_get_max_trans_res() =>
> > xfs_log_calc_max_attrsetm_res() was pretty large.
> > 
> > Upon code inspection I found the inconsistencies in
> > xfs_log_calc_max_attrsetm_res() which are described in the cover letter and as
> > part of the commit message of the last patch.
> > 
> 
> Ok, so the fixes to xfs_log_calc_max_attrsetm_res() are what actually
> fixed the test failure? If so, that strikes me as a good independent fix
> candidate (re: refactoring) because the commit log for that one should
> probably describe the observable problem and the fix separate from other
> issues.
> 
> > After a quick chat with Dave on irc, we figured that the best approach was to
> > convert xfs_attr_calc_size() into a helper function so that the size
> > calculations for an xattr set operation is placed in a single function. These
> > values can then be used by other functions like xfs_attr_set() and
> > xfs_log_calc_max_attrsetm_res().
> > 
> > Along the way, I found that the mount time reservation was incorrectly done as
> > well. For E.g. dabtree splits getting accounted as part of mount time
> > reservation was wrong. Due to these reasons and others listed in the cover
> > letter I ended up changing the mount time and run time reservation
> > calculations.
> > 
> > Hence, The reduced reservation sizes are actually a side effect of fixing the
> > inconsistencies.
> > 
> 
> Ok, so most of the rest sounds like bogosity discovered by code
> inspection. That's not that surprising given that transaction
> reservations are worst case values and thus it seems we sometimes get
> away with bogus calculations just so long as the reservations are large
> enough. :)
> 
> As it is, the final result of this patchset looks nice to me, it's just
> a matter of getting there more incrementally to facilitate reviewing the
> changes being made. FWIW, if we do end up with a final "fix the broken
> xattr res calculation" patch at the end of the series, I think it would
> be helpful to have a very deliberate commit log that contains something
> like the following:
> 
> 'The xattr reservation currently consists of:
> 
> 	- superblock
> 	- dabtree * maxdepth
> 	- ...
> 
> This calculation is bogus because it double accounts X as part of Y and
> Z, doesn't account for AGF, etc. etc. ...
> 
> The xattr reservation needs to account the following:
> 
> 	- superblock
> 	- agf
> 	- dabtree * maxdepth
> 	- rmtblocks
> 	- ...
> 
> ... '
>

I agree with both the comments. I will try to get the patchset
going in the direction suggested above.

Thanks for taking time to review the patchset.

-- 
chandan