| Message ID | 20200224040044.30923-4-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Fix log reservation calculation for xattr insert operation | expand |
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? Brian > fs/xfs/libxfs/xfs_attr.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 942ba552e0bdd..a708b142f69b6 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -154,12 +154,10 @@ xfs_attr_calc_size( > size = xfs_attr_leaf_newentsize(args->geo, args->namelen, > args->valuelen, local); > total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); > - bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK); > if (*local) { > if (size > (args->geo->blksize / 2)) { > /* Double split possible */ > total_dablks *= 2; > - bmbt_blks *= 2; > } > rmt_blks = 0; > } else { > @@ -168,10 +166,11 @@ xfs_attr_calc_size( > * make room for the attribute value itself. > */ > rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen); > - bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks, > - XFS_ATTR_FORK); > } > > + bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks, > + XFS_ATTR_FORK); > + > return total_dablks + rmt_blks + bmbt_blks; > } > > -- > 2.19.1 >
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. > > > fs/xfs/libxfs/xfs_attr.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > index 942ba552e0bdd..a708b142f69b6 100644 > > --- a/fs/xfs/libxfs/xfs_attr.c > > +++ b/fs/xfs/libxfs/xfs_attr.c > > @@ -154,12 +154,10 @@ xfs_attr_calc_size( > > size = xfs_attr_leaf_newentsize(args->geo, args->namelen, > > args->valuelen, local); > > total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); > > - bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK); > > if (*local) { > > if (size > (args->geo->blksize / 2)) { > > /* Double split possible */ > > total_dablks *= 2; > > - bmbt_blks *= 2; > > } > > rmt_blks = 0; > > } else { > > @@ -168,10 +166,11 @@ xfs_attr_calc_size( > > * make room for the attribute value itself. > > */ > > rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen); > > - bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks, > > - XFS_ATTR_FORK); > > } > > > > + bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks, > > + XFS_ATTR_FORK); > > + > > return total_dablks + rmt_blks + bmbt_blks; > > } > > > >
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'm not hugely familiar with the dabtree code, but is it possible the existing reservations are written this way because each dabtree extension along with a remote block allocation are independent xfs_bmapi_write() calls? IOW, perhaps we cannot assume these can all land in the same bmbt blocks across the xattr operation? ISTM that might explain that XFS_DAENTER_BMAPS() calculates the reservation for a single attr block and multiplies it by the max depth, but I could easily be misunderstanding something. What is the motivation for this patch btw? Have you observed a problem or excessive reservation sizes, or is this by code inspection? Brian > > > > > fs/xfs/libxfs/xfs_attr.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > index 942ba552e0bdd..a708b142f69b6 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > @@ -154,12 +154,10 @@ xfs_attr_calc_size( > > > size = xfs_attr_leaf_newentsize(args->geo, args->namelen, > > > args->valuelen, local); > > > total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); > > > - bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK); > > > if (*local) { > > > if (size > (args->geo->blksize / 2)) { > > > /* Double split possible */ > > > total_dablks *= 2; > > > - bmbt_blks *= 2; > > > } > > > rmt_blks = 0; > > > } else { > > > @@ -168,10 +166,11 @@ xfs_attr_calc_size( > > > * make room for the attribute value itself. > > > */ > > > rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen); > > > - bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks, > > > - XFS_ATTR_FORK); > > > } > > > > > > + bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks, > > > + XFS_ATTR_FORK); > > > + > > > return total_dablks + rmt_blks + bmbt_blks; > > > } > > > > > > > > > -- > chandan > > >
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. 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. > > I'm not hugely familiar with the dabtree code, but is it possible the > existing reservations are written this way because each dabtree > extension along with a remote block allocation are independent > xfs_bmapi_write() calls? IOW, perhaps we cannot assume these can all > land in the same bmbt blocks across the xattr operation? ISTM that might > explain that XFS_DAENTER_BMAPS() calculates the reservation for a single > attr block and multiplies it by the max depth, but I could easily be > misunderstanding something. I think you are right. I will keep the bmbt calculations separate for dabtree and remote blocks and add them up at the end of the function. > > What is the motivation for this patch btw? Have you observed a problem > or excessive reservation sizes, or is this by code inspection? > > Brian > > > > > > > > fs/xfs/libxfs/xfs_attr.c | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > > index 942ba552e0bdd..a708b142f69b6 100644 > > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > > @@ -154,12 +154,10 @@ xfs_attr_calc_size( > > > > size = xfs_attr_leaf_newentsize(args->geo, args->namelen, > > > > args->valuelen, local); > > > > total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); > > > > - bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK); > > > > if (*local) { > > > > if (size > (args->geo->blksize / 2)) { > > > > /* Double split possible */ > > > > total_dablks *= 2; > > > > - bmbt_blks *= 2; > > > > } > > > > rmt_blks = 0; > > > > } else { > > > > @@ -168,10 +166,11 @@ xfs_attr_calc_size( > > > > * make room for the attribute value itself. > > > > */ > > > > rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen); > > > > - bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks, > > > > - XFS_ATTR_FORK); > > > > } > > > > > > > > + bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks, > > > > + XFS_ATTR_FORK); > > > > + > > > > return total_dablks + rmt_blks + bmbt_blks; > > > > } > > > > > > > > > > > > > >
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'm not hugely familiar with the dabtree code, but is it possible the > > existing reservations are written this way because each dabtree > > extension along with a remote block allocation are independent > > xfs_bmapi_write() calls? IOW, perhaps we cannot assume these can all > > land in the same bmbt blocks across the xattr operation? ISTM that might > > explain that XFS_DAENTER_BMAPS() calculates the reservation for a single > > attr block and multiplies it by the max depth, but I could easily be > > misunderstanding something. > > I think you are right. I will keep the bmbt calculations separate for dabtree > and remote blocks and add them up at the end of the function. > Ok. I think it's probably safer to preserve historical behavior in that regard, unless somebody can confirm otherwise in the meantime. Brian > > > > What is the motivation for this patch btw? Have you observed a problem > > or excessive reservation sizes, or is this by code inspection? > > > > Brian > > > > > > > > > > > fs/xfs/libxfs/xfs_attr.c | 7 +++---- > > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > > > index 942ba552e0bdd..a708b142f69b6 100644 > > > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > > > @@ -154,12 +154,10 @@ xfs_attr_calc_size( > > > > > size = xfs_attr_leaf_newentsize(args->geo, args->namelen, > > > > > args->valuelen, local); > > > > > total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); > > > > > - bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK); > > > > > if (*local) { > > > > > if (size > (args->geo->blksize / 2)) { > > > > > /* Double split possible */ > > > > > total_dablks *= 2; > > > > > - bmbt_blks *= 2; > > > > > } > > > > > rmt_blks = 0; > > > > > } else { > > > > > @@ -168,10 +166,11 @@ xfs_attr_calc_size( > > > > > * make room for the attribute value itself. > > > > > */ > > > > > rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen); > > > > > - bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks, > > > > > - XFS_ATTR_FORK); > > > > > } > > > > > > > > > > + bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks, > > > > > + XFS_ATTR_FORK); > > > > > + > > > > > return total_dablks + rmt_blks + bmbt_blks; > > > > > } > > > > > > > > > > > > > > > > > > > > > > -- > chandan > > >
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.
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 942ba552e0bdd..a708b142f69b6 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -154,12 +154,10 @@ xfs_attr_calc_size( size = xfs_attr_leaf_newentsize(args->geo, args->namelen, args->valuelen, local); total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); - bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK); if (*local) { if (size > (args->geo->blksize / 2)) { /* Double split possible */ total_dablks *= 2; - bmbt_blks *= 2; } rmt_blks = 0; } else { @@ -168,10 +166,11 @@ xfs_attr_calc_size( * make room for the attribute value itself. */ rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen); - bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks, - XFS_ATTR_FORK); } + bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks, + XFS_ATTR_FORK); + return total_dablks + rmt_blks + bmbt_blks; }
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> --- fs/xfs/libxfs/xfs_attr.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)