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@xxxxxxxxx> > > > > > > --- > > > > > > > > > > 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
