[PATCH v2 0/7] xfs: inode transaction reservation fixups

[PATCH v2 0/7] xfs: inode transaction reservation fixups

[Brian Foster]

Hi all,

Here's v2 of the inode tx reservation fixups. This drops the agfl fixup
patch for the time being and adds some additional reservation fixups and
refactoring based on Dave's feedback. This survives xfstests in a few
different configurations and also happens to survive my inode unlink
reservation overrun tests.

Thoughts, reviews, flames appreciated.

Brian

v2:
- Update AGI unlinked list and truncate reservations
- Update commit log for ifree refactor patch.
- Additional refactoring patches.
- Dropped agfl fixup patch.
v1: https://marc.info/?l=linux-xfs&m=151181428031884&w=2

Brian Foster (7):
  xfs: print transaction log reservation on overrun
  xfs: include inobt buffers in ifree tx log reservation
  xfs: fix up agi unlinked list reservations
  xfs: truncate transaction does not modify the inobt
  xfs: include an allocfree res for inobt modifications
  xfs: refactor inode chunk alloc/free tx reservation
  xfs: eliminate duplicate icreate tx reservation functions

 fs/xfs/libxfs/xfs_trans_resv.c | 202 ++++++++++++++++++++---------------------
 fs/xfs/xfs_log.c               |   4 +-
 2 files changed, 99 insertions(+), 107 deletions(-)

-- 
2.13.6

[PATCH v2 1/7] xfs: print transaction log reservation on overrun

[Brian Foster]

The transaction dump code displays the content and reservation
consumption of a particular transaction in the event of an overrun.
It currently displays the reservation associated with the
transaction ticket, but not the original reservation attached to the
transaction.

The latter value reflects the original transaction reservation
calculation before additional reservation overhead is assigned, such
as for the CIL context header and potential split region headers.

Update xlog_print_trans() to also print the original transaction
reservation in the event of overrun. This provides a reference point
to identify how much reservation overhead was added to a particular
ticket by xfs_log_calc_unit_res().

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 38d4227895ae..3a37fa1ed25e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2117,7 +2117,9 @@ xlog_print_trans(
 
 	/* dump core transaction and ticket info */
 	xfs_warn(mp, "transaction summary:");
-	xfs_warn(mp, "  flags	= 0x%x", tp->t_flags);
+	xfs_warn(mp, "  log res   = %d", tp->t_log_res);
+	xfs_warn(mp, "  log count = %d", tp->t_log_count);
+	xfs_warn(mp, "  flags     = 0x%x", tp->t_flags);
 
 	xlog_print_tic_res(mp, tp->t_ticket);
 
-- 
2.13.6

Re: [PATCH v2 1/7] xfs: print transaction log reservation on overrun

[Darrick J. Wong]

On Thu, Nov 30, 2017 at 01:58:30PM -0500, Brian Foster wrote:
> The transaction dump code displays the content and reservation
> consumption of a particular transaction in the event of an overrun.
> It currently displays the reservation associated with the
> transaction ticket, but not the original reservation attached to the
> transaction.
> 
> The latter value reflects the original transaction reservation
> calculation before additional reservation overhead is assigned, such
> as for the CIL context header and potential split region headers.
> 
> Update xlog_print_trans() to also print the original transaction
> reservation in the event of overrun. This provides a reference point
> to identify how much reservation overhead was added to a particular
> ticket by xfs_log_calc_unit_res().
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/xfs_log.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 38d4227895ae..3a37fa1ed25e 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2117,7 +2117,9 @@ xlog_print_trans(
>  
>  	/* dump core transaction and ticket info */
>  	xfs_warn(mp, "transaction summary:");
> -	xfs_warn(mp, "  flags	= 0x%x", tp->t_flags);
> +	xfs_warn(mp, "  log res   = %d", tp->t_log_res);
> +	xfs_warn(mp, "  log count = %d", tp->t_log_count);
> +	xfs_warn(mp, "  flags     = 0x%x", tp->t_flags);
>  
>  	xlog_print_tic_res(mp, tp->t_ticket);
>  
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/7] xfs: include inobt buffers in ifree tx log reservation

[Brian Foster]

The tr_ifree transaction handles inode unlinks and inode chunk
frees. The current transaction calculation does not accurately
reflect worst case changes to the inode btree, however. The inobt
portion of the current transaction reservation only covers
modification of a single inobt buffer (for the particular inode
record). This is a historical artifact from the days before XFS
supported full inode chunk removal.

When support for inode chunk removal was added in commit
254f6311ed1b ("Implement deletion of inode clusters in XFS."), the
additional log reservation required for chunk removal was not added
correctly. The new reservation only considered the header overhead
of associated buffers rather than the full contents of the btrees
and AGF and AGFL buffers affected by the transaction. The
reservation for the free space btrees was subsequently fixed up in
commit 5fe6abb82f76 ("Add space for inode and allocation btrees to
ITRUNCATE log reservation"), but the res. for full inobt joins has
never been added.

Further review of the ifree reservation uncovered a couple more
problems:

- The undocumented +2 blocks are intended for the AGF and AGFL, but
  are also not sized correctly and should be logged as full sectors
  (not FSBs).
- The additional single block header is undocumented and serves no
  apparent purpose.

Update xfs_calc_ifree_reservation() to include a full inobt join in
the reservation calculation. Refactor the undocumented blocks
appropriately and fix up the comments to reflect the current
calculation.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 6bd916bd35e2..838566b85622 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -490,10 +490,9 @@ xfs_calc_symlink_reservation(
 /*
  * In freeing an inode we can modify:
  *    the inode being freed: inode size
- *    the super block free inode counter: sector size
- *    the agi hash list and counters: sector size
- *    the inode btree entry: block size
- *    the on disk inode before ours in the agi hash list: inode cluster size
+ *    the super block free inode counter, AGF and AGFL: sector size
+ *    the on disk inode (agi unlinked list removal)
+ *    the inode chunk is marked stale (headers only)
  *    the inode btree: max depth * blocksize
  *    the allocation btrees: 2 trees * (max depth - 1) * block size
  *    the finobt (record insertion, removal or modification)
@@ -504,12 +503,10 @@ xfs_calc_ifree_reservation(
 {
 	return XFS_DQUOT_LOGRES(mp) +
 		xfs_calc_inode_res(mp, 1) +
-		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
 		xfs_calc_iunlink_remove_reservation(mp) +
-		xfs_calc_buf_res(1, 0) +
-		xfs_calc_buf_res(2 + mp->m_ialloc_blks +
-				 mp->m_in_maxlevels, 0) +
+		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
+		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
 		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
 				 XFS_FSB_TO_B(mp, 1)) +
 		xfs_calc_finobt_res(mp, 0, 1);
-- 
2.13.6

Re: [PATCH v2 2/7] xfs: include inobt buffers in ifree tx log reservation

[Dave Chinner]

On Thu, Nov 30, 2017 at 01:58:31PM -0500, Brian Foster wrote:
> The tr_ifree transaction handles inode unlinks and inode chunk
> frees. The current transaction calculation does not accurately
> reflect worst case changes to the inode btree, however. The inobt
> portion of the current transaction reservation only covers
> modification of a single inobt buffer (for the particular inode
> record). This is a historical artifact from the days before XFS
> supported full inode chunk removal.
> 
> When support for inode chunk removal was added in commit
> 254f6311ed1b ("Implement deletion of inode clusters in XFS."), the
> additional log reservation required for chunk removal was not added
> correctly. The new reservation only considered the header overhead
> of associated buffers rather than the full contents of the btrees
> and AGF and AGFL buffers affected by the transaction. The
> reservation for the free space btrees was subsequently fixed up in
> commit 5fe6abb82f76 ("Add space for inode and allocation btrees to
> ITRUNCATE log reservation"), but the res. for full inobt joins has
> never been added.
> 
> Further review of the ifree reservation uncovered a couple more
> problems:
> 
> - The undocumented +2 blocks are intended for the AGF and AGFL, but
>   are also not sized correctly and should be logged as full sectors
>   (not FSBs).
> - The additional single block header is undocumented and serves no
>   apparent purpose.
> 
> Update xfs_calc_ifree_reservation() to include a full inobt join in
> the reservation calculation. Refactor the undocumented blocks
> appropriately and fix up the comments to reflect the current
> calculation.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/7] xfs: include inobt buffers in ifree tx log reservation

[Darrick J. Wong]

On Thu, Nov 30, 2017 at 01:58:31PM -0500, Brian Foster wrote:
> The tr_ifree transaction handles inode unlinks and inode chunk
> frees. The current transaction calculation does not accurately
> reflect worst case changes to the inode btree, however. The inobt
> portion of the current transaction reservation only covers
> modification of a single inobt buffer (for the particular inode
> record). This is a historical artifact from the days before XFS
> supported full inode chunk removal.
> 
> When support for inode chunk removal was added in commit
> 254f6311ed1b ("Implement deletion of inode clusters in XFS."), the
> additional log reservation required for chunk removal was not added
> correctly. The new reservation only considered the header overhead
> of associated buffers rather than the full contents of the btrees
> and AGF and AGFL buffers affected by the transaction. The
> reservation for the free space btrees was subsequently fixed up in
> commit 5fe6abb82f76 ("Add space for inode and allocation btrees to
> ITRUNCATE log reservation"), but the res. for full inobt joins has
> never been added.
> 
> Further review of the ifree reservation uncovered a couple more
> problems:
> 
> - The undocumented +2 blocks are intended for the AGF and AGFL, but
>   are also not sized correctly and should be logged as full sectors
>   (not FSBs).
> - The additional single block header is undocumented and serves no
>   apparent purpose.
> 
> Update xfs_calc_ifree_reservation() to include a full inobt join in
> the reservation calculation. Refactor the undocumented blocks
> appropriately and fix up the comments to reflect the current
> calculation.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 6bd916bd35e2..838566b85622 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -490,10 +490,9 @@ xfs_calc_symlink_reservation(
>  /*
>   * In freeing an inode we can modify:
>   *    the inode being freed: inode size
> - *    the super block free inode counter: sector size
> - *    the agi hash list and counters: sector size
> - *    the inode btree entry: block size
> - *    the on disk inode before ours in the agi hash list: inode cluster size
> + *    the super block free inode counter, AGF and AGFL: sector size
> + *    the on disk inode (agi unlinked list removal)
> + *    the inode chunk is marked stale (headers only)
>   *    the inode btree: max depth * blocksize
>   *    the allocation btrees: 2 trees * (max depth - 1) * block size
>   *    the finobt (record insertion, removal or modification)
> @@ -504,12 +503,10 @@ xfs_calc_ifree_reservation(
>  {
>  	return XFS_DQUOT_LOGRES(mp) +
>  		xfs_calc_inode_res(mp, 1) +
> -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
>  		xfs_calc_iunlink_remove_reservation(mp) +
> -		xfs_calc_buf_res(1, 0) +
> -		xfs_calc_buf_res(2 + mp->m_ialloc_blks +
> -				 mp->m_in_maxlevels, 0) +
> +		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
> +		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
>  		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
>  				 XFS_FSB_TO_B(mp, 1)) +
>  		xfs_calc_finobt_res(mp, 0, 1);
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/7] xfs: fix up agi unlinked list reservations

[Brian Foster]

The current AGI unlinked list addition and removal reservations do
not reflect the worst case log usage. An unlinked list removal can
log up to two on-disk inode clusters but only includes reservation
for one. An unlinked list addition logs the on-disk cluster but
includes reservation for an in-core inode.

Update the AGI unlinked list reservation helpers to calculate the
correct worst case reservation for the associated operations.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 838566b85622..173b1bc13ffe 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -282,13 +282,14 @@ xfs_calc_rename_reservation(
  * For removing an inode from unlinked list at first, we can modify:
  *    the agi hash list and counters: sector size
  *    the on disk inode before ours in the agi hash list: inode cluster size
+ *    the on disk inode in the agi hash list: inode cluster size
  */
 STATIC uint
 xfs_calc_iunlink_remove_reservation(
 	struct xfs_mount        *mp)
 {
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-	       max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
+	       2 * max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
 }
 
 /*
@@ -320,13 +321,13 @@ xfs_calc_link_reservation(
 /*
  * For adding an inode to unlinked list we can modify:
  *    the agi hash list: sector size
- *    the unlinked inode: inode size
+ *    the on disk inode: inode cluster size
  */
 STATIC uint
 xfs_calc_iunlink_add_reservation(xfs_mount_t *mp)
 {
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-		xfs_calc_inode_res(mp, 1);
+		max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
 }
 
 /*
-- 
2.13.6

Re: [PATCH v2 3/7] xfs: fix up agi unlinked list reservations

[Dave Chinner]

On Thu, Nov 30, 2017 at 01:58:32PM -0500, Brian Foster wrote:
> The current AGI unlinked list addition and removal reservations do
> not reflect the worst case log usage. An unlinked list removal can
> log up to two on-disk inode clusters but only includes reservation
> for one. An unlinked list addition logs the on-disk cluster but
> includes reservation for an in-core inode.
> 
> Update the AGI unlinked list reservation helpers to calculate the
> correct worst case reservation for the associated operations.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

looks sane.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 3/7] xfs: fix up agi unlinked list reservations

[Darrick J. Wong]

On Thu, Nov 30, 2017 at 01:58:32PM -0500, Brian Foster wrote:
> The current AGI unlinked list addition and removal reservations do
> not reflect the worst case log usage. An unlinked list removal can
> log up to two on-disk inode clusters but only includes reservation
> for one. An unlinked list addition logs the on-disk cluster but
> includes reservation for an in-core inode.
> 
> Update the AGI unlinked list reservation helpers to calculate the
> correct worst case reservation for the associated operations.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 838566b85622..173b1bc13ffe 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -282,13 +282,14 @@ xfs_calc_rename_reservation(
>   * For removing an inode from unlinked list at first, we can modify:
>   *    the agi hash list and counters: sector size
>   *    the on disk inode before ours in the agi hash list: inode cluster size
> + *    the on disk inode in the agi hash list: inode cluster size
>   */
>  STATIC uint
>  xfs_calc_iunlink_remove_reservation(
>  	struct xfs_mount        *mp)
>  {
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -	       max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
> +	       2 * max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
>  }
>  
>  /*
> @@ -320,13 +321,13 @@ xfs_calc_link_reservation(
>  /*
>   * For adding an inode to unlinked list we can modify:
>   *    the agi hash list: sector size
> - *    the unlinked inode: inode size
> + *    the on disk inode: inode cluster size
>   */
>  STATIC uint
>  xfs_calc_iunlink_add_reservation(xfs_mount_t *mp)
>  {
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_inode_res(mp, 1);
> +		max_t(uint, XFS_FSB_TO_B(mp, 1), mp->m_inode_cluster_size);
>  }
>  
>  /*
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/7] xfs: truncate transaction does not modify the inobt

[Brian Foster]

The truncate transaction does not ever modify the inode btree, but
includes an associated log reservation. Update
xfs_calc_itruncate_reservation() to remove the reservation
associated with inobt updates.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 173b1bc13ffe..037a1295d289 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -232,8 +232,6 @@ xfs_calc_write_reservation(
  *    the super block to reflect the freed blocks: sector size
  *    worst case split in allocation btrees per extent assuming 4 extents:
  *		4 exts * 2 trees * (2 * max depth - 1) * block size
- *    the inode btree: max depth * blocksize
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
  */
 STATIC uint
 xfs_calc_itruncate_reservation(
@@ -245,12 +243,7 @@ xfs_calc_itruncate_reservation(
 				      XFS_FSB_TO_B(mp, 1))),
 		    (xfs_calc_buf_res(9, mp->m_sb.sb_sectsize) +
 		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 4),
-				      XFS_FSB_TO_B(mp, 1)) +
-		    xfs_calc_buf_res(5, 0) +
-		    xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				     XFS_FSB_TO_B(mp, 1)) +
-		    xfs_calc_buf_res(2 + mp->m_ialloc_blks +
-				     mp->m_in_maxlevels, 0)));
+				      XFS_FSB_TO_B(mp, 1))));
 }
 
 /*
-- 
2.13.6

Re: [PATCH v2 4/7] xfs: truncate transaction does not modify the inobt

[Dave Chinner]

On Thu, Nov 30, 2017 at 01:58:33PM -0500, Brian Foster wrote:
> The truncate transaction does not ever modify the inode btree, but
> includes an associated log reservation. Update
> xfs_calc_itruncate_reservation() to remove the reservation
> associated with inobt updates.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Makes sense.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 4/7] xfs: truncate transaction does not modify the inobt

[Darrick J. Wong]

On Thu, Nov 30, 2017 at 01:58:33PM -0500, Brian Foster wrote:
> The truncate transaction does not ever modify the inode btree, but
> includes an associated log reservation. Update
> xfs_calc_itruncate_reservation() to remove the reservation
> associated with inobt updates.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 173b1bc13ffe..037a1295d289 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -232,8 +232,6 @@ xfs_calc_write_reservation(
>   *    the super block to reflect the freed blocks: sector size
>   *    worst case split in allocation btrees per extent assuming 4 extents:
>   *		4 exts * 2 trees * (2 * max depth - 1) * block size
> - *    the inode btree: max depth * blocksize
> - *    the allocation btrees: 2 trees * (max depth - 1) * block size
>   */
>  STATIC uint
>  xfs_calc_itruncate_reservation(
> @@ -245,12 +243,7 @@ xfs_calc_itruncate_reservation(
>  				      XFS_FSB_TO_B(mp, 1))),
>  		    (xfs_calc_buf_res(9, mp->m_sb.sb_sectsize) +
>  		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 4),
> -				      XFS_FSB_TO_B(mp, 1)) +
> -		    xfs_calc_buf_res(5, 0) +
> -		    xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				     XFS_FSB_TO_B(mp, 1)) +
> -		    xfs_calc_buf_res(2 + mp->m_ialloc_blks +
> -				     mp->m_in_maxlevels, 0)));
> +				      XFS_FSB_TO_B(mp, 1))));
>  }
>  
>  /*
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/7] xfs: include an allocfree res for inobt modifications

[Brian Foster]

Analysis of recent reports of log reservation overruns and code
inspection has uncovered that the reservations associated with inode
operations may not cover the worst case scenarios. In particular,
many cases only include one allocfree res. for a particular
operation even though said operations may also entail AGFL fixups
and inode btree block allocations in addition to the actual inode
chunk allocation. This can easily turn into two or three block
allocations (or frees) per operation.

In theory, the only way to define the worst case reservation is to
include an allocfree res for each individual allocation in a
transaction. Since that is impractical (we can perform multiple agfl
fixups per tx and not every allocation results in a full tree
operation), we need to find a reasonable compromise that addresses
the deficiency in practice without blowing out the size of the
transactions.

Since the inode btrees are not filled by the AGFL, record insertion
and removal can directly result in block allocations and frees
depending on the shape of the tree. These allocations and frees
occur in the same transaction context as the inobt update itself,
but are separate from the allocation/free that might be required for
an inode chunk. Therefore, it makes sense to assume that an [f]inobt
insert/remove can directly result in one or more block allocations
on behalf of the tree.

Refactor the inode transaction reservations to include one allocfree
res. per inode btree modification to cover allocations required by
the tree itself. This separates the reservation required to allocate
the inode chunk from the reservation required for inobt record
insertion/removal. Apply the same logic to the finobt. This results
in killing off the finobt modify condition because we no longer
assume that the broader transaction reservation will cover finobt
block allocations and finobt shape changes can occur in either of
the inobt allocation or modify situations.

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 84 +++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 037a1295d289..19f3a226a357 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -132,44 +132,43 @@ xfs_calc_inode_res(
 }
 
 /*
- * The free inode btree is a conditional feature and the log reservation
- * requirements differ slightly from that of the traditional inode allocation
- * btree. The finobt tracks records for inode chunks with at least one free
- * inode. A record can be removed from the tree for an inode allocation
- * or free and thus the finobt reservation is unconditional across:
+ * Inode btree record insertion/removal modifies the inode btree and free space
+ * btrees (since the inobt does not use the agfl). This requires the following
+ * reservation:
  *
- * 	- inode allocation
- * 	- inode free
- * 	- inode chunk allocation
+ * the inode btree: max depth * blocksize
+ * the allocation btrees: 2 trees * (max depth - 1) * block size
  *
- * The 'modify' param indicates to include the record modification scenario. The
- * 'alloc' param indicates to include the reservation for free space btree
- * modifications on behalf of finobt modifications. This is required only for
- * transactions that do not already account for free space btree modifications.
+ * The caller must account for SB and AG header modifications, etc.
+ */
+STATIC uint
+xfs_calc_inobt_res(
+	struct xfs_mount	*mp)
+{
+	return xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
+				 XFS_FSB_TO_B(mp, 1));
+}
+
+/*
+ * The free inode btree is a conditional feature. The behavior differs slightly
+ * from that of the traditional inode btree in that the finobt tracks records
+ * for inode chunks with at least one free inode. A record can be removed from
+ * the tree during individual inode allocation. Therefore the finobt
+ * reservation is unconditional for both the inode chunk allocation and
+ * individual inode allocation (modify) cases.
  *
- * the free inode btree: max depth * block size
- * the allocation btrees: 2 trees * (max depth - 1) * block size
- * the free inode btree entry: block size
+ * Behavior aside, the reservation for finobt modification is equivalent to the
+ * traditional inobt: cover a full finobt shape change plus block allocation.
  */
 STATIC uint
 xfs_calc_finobt_res(
-	struct xfs_mount	*mp,
-	int			alloc,
-	int			modify)
+	struct xfs_mount	*mp)
 {
-	uint res;
-
 	if (!xfs_sb_version_hasfinobt(&mp->m_sb))
 		return 0;
 
-	res = xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1));
-	if (alloc)
-		res += xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-					XFS_FSB_TO_B(mp, 1));
-	if (modify)
-		res += (uint)XFS_FSB_TO_B(mp, 1);
-
-	return res;
+	return xfs_calc_inobt_res(mp);
 }
 
 /*
@@ -373,7 +372,7 @@ xfs_calc_create_resv_modify(
 		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
 		(uint)XFS_FSB_TO_B(mp, 1) +
 		xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp), XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_finobt_res(mp, 1, 1);
+		xfs_calc_finobt_res(mp);
 }
 
 /*
@@ -381,8 +380,8 @@ xfs_calc_create_resv_modify(
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
  *    the inode blocks allocated: mp->m_ialloc_blks * blocksize
- *    the inode btree: max depth * blocksize
  *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inode btree (record insertion)
  */
 STATIC uint
 xfs_calc_create_resv_alloc(
@@ -391,9 +390,9 @@ xfs_calc_create_resv_alloc(
 	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
 		mp->m_sb.sb_sectsize +
 		xfs_calc_buf_res(mp->m_ialloc_blks, XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
 		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				 XFS_FSB_TO_B(mp, 1));
+				 XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_inobt_res(mp);
 }
 
 STATIC uint
@@ -409,8 +408,8 @@ __xfs_calc_create_reservation(
  * For icreate we can allocate some inodes giving:
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
- *    the inode btree: max depth * blocksize
  *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inobt (record insertion)
  *    the finobt (record insertion)
  */
 STATIC uint
@@ -419,10 +418,10 @@ xfs_calc_icreate_resv_alloc(
 {
 	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
 		mp->m_sb.sb_sectsize +
-		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
 		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
 				 XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_finobt_res(mp, 0, 0);
+		xfs_calc_inobt_res(mp) +
+		xfs_calc_finobt_res(mp);
 }
 
 STATIC uint
@@ -487,9 +486,14 @@ xfs_calc_symlink_reservation(
  *    the super block free inode counter, AGF and AGFL: sector size
  *    the on disk inode (agi unlinked list removal)
  *    the inode chunk is marked stale (headers only)
- *    the inode btree: max depth * blocksize
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inode btree
  *    the finobt (record insertion, removal or modification)
+ *
+ * Note that the allocfree res. for the inode chunk itself is not included
+ * because the extent free occurs after a transaction roll. We could take the
+ * maximum of the pre/post roll operations, but the pre-roll reservation already
+ * includes at least one allocfree res. for the inobt and is thus guaranteed to
+ * be larger.
  */
 STATIC uint
 xfs_calc_ifree_reservation(
@@ -500,10 +504,8 @@ xfs_calc_ifree_reservation(
 		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
 		xfs_calc_iunlink_remove_reservation(mp) +
 		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
-		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				 XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_finobt_res(mp, 0, 1);
+		xfs_calc_inobt_res(mp) +
+		xfs_calc_finobt_res(mp);
 }
 
 /*
-- 
2.13.6

Re: [PATCH v2 5/7] xfs: include an allocfree res for inobt modifications

[Darrick J. Wong]

On Thu, Nov 30, 2017 at 01:58:34PM -0500, Brian Foster wrote:
> Analysis of recent reports of log reservation overruns and code
> inspection has uncovered that the reservations associated with inode
> operations may not cover the worst case scenarios. In particular,
> many cases only include one allocfree res. for a particular
> operation even though said operations may also entail AGFL fixups
> and inode btree block allocations in addition to the actual inode
> chunk allocation. This can easily turn into two or three block
> allocations (or frees) per operation.
> 
> In theory, the only way to define the worst case reservation is to
> include an allocfree res for each individual allocation in a
> transaction. Since that is impractical (we can perform multiple agfl
> fixups per tx and not every allocation results in a full tree
> operation), we need to find a reasonable compromise that addresses
> the deficiency in practice without blowing out the size of the
> transactions.
> 
> Since the inode btrees are not filled by the AGFL, record insertion
> and removal can directly result in block allocations and frees
> depending on the shape of the tree. These allocations and frees
> occur in the same transaction context as the inobt update itself,
> but are separate from the allocation/free that might be required for
> an inode chunk. Therefore, it makes sense to assume that an [f]inobt
> insert/remove can directly result in one or more block allocations
> on behalf of the tree.
> 
> Refactor the inode transaction reservations to include one allocfree
> res. per inode btree modification to cover allocations required by
> the tree itself. This separates the reservation required to allocate
> the inode chunk from the reservation required for inobt record
> insertion/removal. Apply the same logic to the finobt. This results
> in killing off the finobt modify condition because we no longer
> assume that the broader transaction reservation will cover finobt
> block allocations and finobt shape changes can occur in either of
> the inobt allocation or modify situations.
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 84 +++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 037a1295d289..19f3a226a357 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -132,44 +132,43 @@ xfs_calc_inode_res(
>  }
>  
>  /*
> - * The free inode btree is a conditional feature and the log reservation
> - * requirements differ slightly from that of the traditional inode allocation
> - * btree. The finobt tracks records for inode chunks with at least one free
> - * inode. A record can be removed from the tree for an inode allocation
> - * or free and thus the finobt reservation is unconditional across:
> + * Inode btree record insertion/removal modifies the inode btree and free space
> + * btrees (since the inobt does not use the agfl). This requires the following
> + * reservation:
>   *
> - * 	- inode allocation
> - * 	- inode free
> - * 	- inode chunk allocation
> + * the inode btree: max depth * blocksize
> + * the allocation btrees: 2 trees * (max depth - 1) * block size
>   *
> - * The 'modify' param indicates to include the record modification scenario. The
> - * 'alloc' param indicates to include the reservation for free space btree
> - * modifications on behalf of finobt modifications. This is required only for
> - * transactions that do not already account for free space btree modifications.
> + * The caller must account for SB and AG header modifications, etc.
> + */
> +STATIC uint
> +xfs_calc_inobt_res(
> +	struct xfs_mount	*mp)
> +{
> +	return xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> +				 XFS_FSB_TO_B(mp, 1));
> +}
> +
> +/*
> + * The free inode btree is a conditional feature. The behavior differs slightly
> + * from that of the traditional inode btree in that the finobt tracks records
> + * for inode chunks with at least one free inode. A record can be removed from
> + * the tree during individual inode allocation. Therefore the finobt
> + * reservation is unconditional for both the inode chunk allocation and
> + * individual inode allocation (modify) cases.
>   *
> - * the free inode btree: max depth * block size
> - * the allocation btrees: 2 trees * (max depth - 1) * block size
> - * the free inode btree entry: block size
> + * Behavior aside, the reservation for finobt modification is equivalent to the
> + * traditional inobt: cover a full finobt shape change plus block allocation.
>   */
>  STATIC uint
>  xfs_calc_finobt_res(
> -	struct xfs_mount	*mp,
> -	int			alloc,
> -	int			modify)
> +	struct xfs_mount	*mp)
>  {
> -	uint res;
> -
>  	if (!xfs_sb_version_hasfinobt(&mp->m_sb))
>  		return 0;
>  
> -	res = xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1));
> -	if (alloc)
> -		res += xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -					XFS_FSB_TO_B(mp, 1));
> -	if (modify)
> -		res += (uint)XFS_FSB_TO_B(mp, 1);
> -
> -	return res;
> +	return xfs_calc_inobt_res(mp);
>  }
>  
>  /*
> @@ -373,7 +372,7 @@ xfs_calc_create_resv_modify(
>  		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
>  		(uint)XFS_FSB_TO_B(mp, 1) +
>  		xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp), XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_finobt_res(mp, 1, 1);
> +		xfs_calc_finobt_res(mp);
>  }
>  
>  /*
> @@ -381,8 +380,8 @@ xfs_calc_create_resv_modify(
>   *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
>   *    the superblock for the nlink flag: sector size
>   *    the inode blocks allocated: mp->m_ialloc_blks * blocksize
> - *    the inode btree: max depth * blocksize
>   *    the allocation btrees: 2 trees * (max depth - 1) * block size
> + *    the inode btree (record insertion)
>   */
>  STATIC uint
>  xfs_calc_create_resv_alloc(
> @@ -391,9 +390,9 @@ xfs_calc_create_resv_alloc(
>  	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
>  		mp->m_sb.sb_sectsize +
>  		xfs_calc_buf_res(mp->m_ialloc_blks, XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
>  		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1));
> +				 XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_inobt_res(mp);
>  }
>  
>  STATIC uint
> @@ -409,8 +408,8 @@ __xfs_calc_create_reservation(
>   * For icreate we can allocate some inodes giving:
>   *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
>   *    the superblock for the nlink flag: sector size
> - *    the inode btree: max depth * blocksize
>   *    the allocation btrees: 2 trees * (max depth - 1) * block size
> + *    the inobt (record insertion)
>   *    the finobt (record insertion)
>   */
>  STATIC uint
> @@ -419,10 +418,10 @@ xfs_calc_icreate_resv_alloc(
>  {
>  	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
>  		mp->m_sb.sb_sectsize +
> -		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
>  		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
>  				 XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_finobt_res(mp, 0, 0);
> +		xfs_calc_inobt_res(mp) +
> +		xfs_calc_finobt_res(mp);
>  }
>  
>  STATIC uint
> @@ -487,9 +486,14 @@ xfs_calc_symlink_reservation(
>   *    the super block free inode counter, AGF and AGFL: sector size
>   *    the on disk inode (agi unlinked list removal)
>   *    the inode chunk is marked stale (headers only)
> - *    the inode btree: max depth * blocksize
> - *    the allocation btrees: 2 trees * (max depth - 1) * block size
> + *    the inode btree
>   *    the finobt (record insertion, removal or modification)
> + *
> + * Note that the allocfree res. for the inode chunk itself is not included
> + * because the extent free occurs after a transaction roll. We could take the
> + * maximum of the pre/post roll operations, but the pre-roll reservation already
> + * includes at least one allocfree res. for the inobt and is thus guaranteed to
> + * be larger.
>   */
>  STATIC uint
>  xfs_calc_ifree_reservation(
> @@ -500,10 +504,8 @@ xfs_calc_ifree_reservation(
>  		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
>  		xfs_calc_iunlink_remove_reservation(mp) +
>  		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
> -		xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_finobt_res(mp, 0, 1);
> +		xfs_calc_inobt_res(mp) +
> +		xfs_calc_finobt_res(mp);
>  }
>  
>  /*
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/7] xfs: refactor inode chunk alloc/free tx reservation

[Brian Foster]

The reservation for the various forms of inode allocation is
scattered across several different functions. This includes two
variants of chunk allocation (v5 icreate transactions vs. older
create transactions) and the inode free transaction.

To clean up some of this code and clarify the purpose of specific
allocfree reservations, continue the pattern of defining helper
functions for smaller operational units of broader transactions.
Refactor the reservation into an inode chunk alloc/free helper that
considers the various conditions based on filesystem format.

An inode chunk free involves an extent free and buffer
invalidations. The latter requires reservation for log headers only.
An inode chunk allocation modifies the free space btrees and logs
the chunk on v4 supers. v5 supers initialize the inode chunk using
ordered buffers and so do not log the chunk.

As a side effect of this refactoring, add one more allocfree res to
the ifree transaction. Technically this does not serve a specific
purpose because inode chunks are freed via deferred operations and
thus occur after a transaction roll. tr_ifree has a bit of a history
of tx overruns caused by too many agfl fixups during sustained file
deletion workloads, so add this extra reservation as a form of
padding nonetheless.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 67 ++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 19f3a226a357..432dd7d7afea 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -34,6 +34,9 @@
 #include "xfs_trans_space.h"
 #include "xfs_trace.h"
 
+#define _ALLOC	true
+#define _FREE	false
+
 /*
  * A buffer has a format structure overhead in the log in addition
  * to the data, so we need to take this into account when reserving
@@ -172,6 +175,41 @@ xfs_calc_finobt_res(
 }
 
 /*
+ * Calculate the reservation required to allocate or free an inode chunk. This
+ * includes:
+ *
+ * the allocation btrees: 2 trees * (max depth - 1) * block size
+ * the inode chunk: m_ialloc_blks * N
+ *
+ * The size N of the inode chunk reservation depends on whether it is for
+ * allocation or free and which type of create transaction is in use. An inode
+ * chunk free always invalidates the buffers and only requires reservation for
+ * headers (N == 0). An inode chunk allocation requires a chunk sized
+ * reservation on v4 and older superblocks to initialize the chunk. No chunk
+ * reservation is required for allocation on v5 supers, which use ordered
+ * buffers to initialize.
+ */
+STATIC uint
+xfs_calc_inode_chunk_res(
+	struct xfs_mount	*mp,
+	bool			alloc)
+{
+	uint			res, size = 0;
+
+	res = xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
+			       XFS_FSB_TO_B(mp, 1));
+	if (alloc) {
+		/* icreate tx uses ordered buffers */
+		if (xfs_sb_version_hascrc(&mp->m_sb))
+			return res;
+		size = XFS_FSB_TO_B(mp, 1);
+	}
+
+	res += xfs_calc_buf_res(mp->m_ialloc_blks, size);
+	return res;
+}
+
+/*
  * Various log reservation values.
  *
  * These are based on the size of the file system block because that is what
@@ -379,8 +417,7 @@ xfs_calc_create_resv_modify(
  * For create we can allocate some inodes giving:
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
- *    the inode blocks allocated: mp->m_ialloc_blks * blocksize
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inode chunk (allocation/init)
  *    the inode btree (record insertion)
  */
 STATIC uint
@@ -389,9 +426,7 @@ xfs_calc_create_resv_alloc(
 {
 	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
 		mp->m_sb.sb_sectsize +
-		xfs_calc_buf_res(mp->m_ialloc_blks, XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				 XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_inode_chunk_res(mp, _ALLOC) +
 		xfs_calc_inobt_res(mp);
 }
 
@@ -408,7 +443,7 @@ __xfs_calc_create_reservation(
  * For icreate we can allocate some inodes giving:
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inode chunk (allocation, no init)
  *    the inobt (record insertion)
  *    the finobt (record insertion)
  */
@@ -418,8 +453,7 @@ xfs_calc_icreate_resv_alloc(
 {
 	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
 		mp->m_sb.sb_sectsize +
-		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				 XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_inode_chunk_res(mp, _ALLOC) +
 		xfs_calc_inobt_res(mp) +
 		xfs_calc_finobt_res(mp);
 }
@@ -485,15 +519,15 @@ xfs_calc_symlink_reservation(
  *    the inode being freed: inode size
  *    the super block free inode counter, AGF and AGFL: sector size
  *    the on disk inode (agi unlinked list removal)
- *    the inode chunk is marked stale (headers only)
+ *    the inode chunk (invalidated, headers only)
  *    the inode btree
  *    the finobt (record insertion, removal or modification)
  *
- * Note that the allocfree res. for the inode chunk itself is not included
- * because the extent free occurs after a transaction roll. We could take the
- * maximum of the pre/post roll operations, but the pre-roll reservation already
- * includes at least one allocfree res. for the inobt and is thus guaranteed to
- * be larger.
+ * Note that the inode chunk res. includes an allocfree res. for freeing of the
+ * inode chunk. This is technically extraneous because the inode chunk free is
+ * deferred (it occurs after a transaction roll). Include the extra reservation
+ * anyways since we've had reports of ifree transaction overruns due to too many
+ * agfl fixups during inode chunk frees.
  */
 STATIC uint
 xfs_calc_ifree_reservation(
@@ -503,7 +537,7 @@ xfs_calc_ifree_reservation(
 		xfs_calc_inode_res(mp, 1) +
 		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
 		xfs_calc_iunlink_remove_reservation(mp) +
-		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
+		xfs_calc_inode_chunk_res(mp, _FREE) +
 		xfs_calc_inobt_res(mp) +
 		xfs_calc_finobt_res(mp);
 }
@@ -795,6 +829,9 @@ xfs_calc_sb_reservation(
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
 }
 
+#undef _ALLOC
+#undef _FREE
+
 void
 xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
-- 
2.13.6

Re: [PATCH v2 6/7] xfs: refactor inode chunk alloc/free tx reservation

[Dave Chinner]

On Thu, Nov 30, 2017 at 01:58:35PM -0500, Brian Foster wrote:
> The reservation for the various forms of inode allocation is
> scattered across several different functions. This includes two
> variants of chunk allocation (v5 icreate transactions vs. older
> create transactions) and the inode free transaction.
> 
> To clean up some of this code and clarify the purpose of specific
> allocfree reservations, continue the pattern of defining helper
> functions for smaller operational units of broader transactions.
> Refactor the reservation into an inode chunk alloc/free helper that
> considers the various conditions based on filesystem format.
> 
> An inode chunk free involves an extent free and buffer
> invalidations. The latter requires reservation for log headers only.
> An inode chunk allocation modifies the free space btrees and logs
> the chunk on v4 supers. v5 supers initialize the inode chunk using
> ordered buffers and so do not log the chunk.
> 
> As a side effect of this refactoring, add one more allocfree res to
> the ifree transaction. Technically this does not serve a specific
> purpose because inode chunks are freed via deferred operations and
> thus occur after a transaction roll. tr_ifree has a bit of a history
> of tx overruns caused by too many agfl fixups during sustained file
> deletion workloads, so add this extra reservation as a form of
> padding nonetheless.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Minor quibble below, otherwise looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 67 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 19f3a226a357..432dd7d7afea 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -34,6 +34,9 @@
>  #include "xfs_trans_space.h"
>  #include "xfs_trace.h"
>  
> +#define _ALLOC	true
> +#define _FREE	false
> +
>  /*
>   * A buffer has a format structure overhead in the log in addition
>   * to the data, so we need to take this into account when reserving

These are defined at the top of the file so most functions see
them (i.e. scope is the file wide)....

> @@ -795,6 +829,9 @@ xfs_calc_sb_reservation(
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
>  }
>  
> +#undef _ALLOC
> +#undef _FREE
> +
>  void
>  xfs_trans_resv_calc(
>  	struct xfs_mount	*mp,

... so why bother undef'ing them seemingly at random in the middle of
the file? Doesn't seem necessary, and will just be more code to
move around in future...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 6/7] xfs: refactor inode chunk alloc/free tx reservation

[Brian Foster]

On Mon, Dec 04, 2017 at 08:52:04AM +1100, Dave Chinner wrote:
> On Thu, Nov 30, 2017 at 01:58:35PM -0500, Brian Foster wrote:
> > The reservation for the various forms of inode allocation is
> > scattered across several different functions. This includes two
> > variants of chunk allocation (v5 icreate transactions vs. older
> > create transactions) and the inode free transaction.
> > 
> > To clean up some of this code and clarify the purpose of specific
> > allocfree reservations, continue the pattern of defining helper
> > functions for smaller operational units of broader transactions.
> > Refactor the reservation into an inode chunk alloc/free helper that
> > considers the various conditions based on filesystem format.
> > 
> > An inode chunk free involves an extent free and buffer
> > invalidations. The latter requires reservation for log headers only.
> > An inode chunk allocation modifies the free space btrees and logs
> > the chunk on v4 supers. v5 supers initialize the inode chunk using
> > ordered buffers and so do not log the chunk.
> > 
> > As a side effect of this refactoring, add one more allocfree res to
> > the ifree transaction. Technically this does not serve a specific
> > purpose because inode chunks are freed via deferred operations and
> > thus occur after a transaction roll. tr_ifree has a bit of a history
> > of tx overruns caused by too many agfl fixups during sustained file
> > deletion workloads, so add this extra reservation as a form of
> > padding nonetheless.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Minor quibble below, otherwise looks fine.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> > ---
> >  fs/xfs/libxfs/xfs_trans_resv.c | 67 ++++++++++++++++++++++++++++++++----------
> >  1 file changed, 52 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 19f3a226a357..432dd7d7afea 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -34,6 +34,9 @@
> >  #include "xfs_trans_space.h"
> >  #include "xfs_trace.h"
> >  
> > +#define _ALLOC	true
> > +#define _FREE	false
> > +
> >  /*
> >   * A buffer has a format structure overhead in the log in addition
> >   * to the data, so we need to take this into account when reserving
> 
> These are defined at the top of the file so most functions see
> them (i.e. scope is the file wide)....
> 
> > @@ -795,6 +829,9 @@ xfs_calc_sb_reservation(
> >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> >  }
> >  
> > +#undef _ALLOC
> > +#undef _FREE
> > +
> >  void
> >  xfs_trans_resv_calc(
> >  	struct xfs_mount	*mp,
> 
> ... so why bother undef'ing them seemingly at random in the middle of
> the file? Doesn't seem necessary, and will just be more code to
> move around in future...
> 

The intent was to limit them to the reservation calculation functions,
just for clarity I suppose. It just happens to encompass most of the
file and start at the top (just about everything within the ifdef/undef
is local scope). I'm fine with it either way, however. I'll post a v3
without the undefs.. thanks for the review!

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 6/7] xfs: refactor inode chunk alloc/free tx reservation

[Brian Foster]

The reservation for the various forms of inode allocation is
scattered across several different functions. This includes two
variants of chunk allocation (v5 icreate transactions vs. older
create transactions) and the inode free transaction.

To clean up some of this code and clarify the purpose of specific
allocfree reservations, continue the pattern of defining helper
functions for smaller operational units of broader transactions.
Refactor the reservation into an inode chunk alloc/free helper that
considers the various conditions based on filesystem format.

An inode chunk free involves an extent free and buffer
invalidations. The latter requires reservation for log headers only.
An inode chunk allocation modifies the free space btrees and logs
the chunk on v4 supers. v5 supers initialize the inode chunk using
ordered buffers and so do not log the chunk.

As a side effect of this refactoring, add one more allocfree res to
the ifree transaction. Technically this does not serve a specific
purpose because inode chunks are freed via deferred operations and
thus occur after a transaction roll. tr_ifree has a bit of a history
of tx overruns caused by too many agfl fixups during sustained file
deletion workloads, so add this extra reservation as a form of
padding nonetheless.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---

v3:
- Drop undefs.

 fs/xfs/libxfs/xfs_trans_resv.c | 64 ++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 19f3a226a357..75259a1346eb 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -34,6 +34,9 @@
 #include "xfs_trans_space.h"
 #include "xfs_trace.h"
 
+#define _ALLOC	true
+#define _FREE	false
+
 /*
  * A buffer has a format structure overhead in the log in addition
  * to the data, so we need to take this into account when reserving
@@ -172,6 +175,41 @@ xfs_calc_finobt_res(
 }
 
 /*
+ * Calculate the reservation required to allocate or free an inode chunk. This
+ * includes:
+ *
+ * the allocation btrees: 2 trees * (max depth - 1) * block size
+ * the inode chunk: m_ialloc_blks * N
+ *
+ * The size N of the inode chunk reservation depends on whether it is for
+ * allocation or free and which type of create transaction is in use. An inode
+ * chunk free always invalidates the buffers and only requires reservation for
+ * headers (N == 0). An inode chunk allocation requires a chunk sized
+ * reservation on v4 and older superblocks to initialize the chunk. No chunk
+ * reservation is required for allocation on v5 supers, which use ordered
+ * buffers to initialize.
+ */
+STATIC uint
+xfs_calc_inode_chunk_res(
+	struct xfs_mount	*mp,
+	bool			alloc)
+{
+	uint			res, size = 0;
+
+	res = xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
+			       XFS_FSB_TO_B(mp, 1));
+	if (alloc) {
+		/* icreate tx uses ordered buffers */
+		if (xfs_sb_version_hascrc(&mp->m_sb))
+			return res;
+		size = XFS_FSB_TO_B(mp, 1);
+	}
+
+	res += xfs_calc_buf_res(mp->m_ialloc_blks, size);
+	return res;
+}
+
+/*
  * Various log reservation values.
  *
  * These are based on the size of the file system block because that is what
@@ -379,8 +417,7 @@ xfs_calc_create_resv_modify(
  * For create we can allocate some inodes giving:
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
- *    the inode blocks allocated: mp->m_ialloc_blks * blocksize
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inode chunk (allocation/init)
  *    the inode btree (record insertion)
  */
 STATIC uint
@@ -389,9 +426,7 @@ xfs_calc_create_resv_alloc(
 {
 	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
 		mp->m_sb.sb_sectsize +
-		xfs_calc_buf_res(mp->m_ialloc_blks, XFS_FSB_TO_B(mp, 1)) +
-		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				 XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_inode_chunk_res(mp, _ALLOC) +
 		xfs_calc_inobt_res(mp);
 }
 
@@ -408,7 +443,7 @@ __xfs_calc_create_reservation(
  * For icreate we can allocate some inodes giving:
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
+ *    the inode chunk (allocation, no init)
  *    the inobt (record insertion)
  *    the finobt (record insertion)
  */
@@ -418,8 +453,7 @@ xfs_calc_icreate_resv_alloc(
 {
 	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
 		mp->m_sb.sb_sectsize +
-		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				 XFS_FSB_TO_B(mp, 1)) +
+		xfs_calc_inode_chunk_res(mp, _ALLOC) +
 		xfs_calc_inobt_res(mp) +
 		xfs_calc_finobt_res(mp);
 }
@@ -485,15 +519,15 @@ xfs_calc_symlink_reservation(
  *    the inode being freed: inode size
  *    the super block free inode counter, AGF and AGFL: sector size
  *    the on disk inode (agi unlinked list removal)
- *    the inode chunk is marked stale (headers only)
+ *    the inode chunk (invalidated, headers only)
  *    the inode btree
  *    the finobt (record insertion, removal or modification)
  *
- * Note that the allocfree res. for the inode chunk itself is not included
- * because the extent free occurs after a transaction roll. We could take the
- * maximum of the pre/post roll operations, but the pre-roll reservation already
- * includes at least one allocfree res. for the inobt and is thus guaranteed to
- * be larger.
+ * Note that the inode chunk res. includes an allocfree res. for freeing of the
+ * inode chunk. This is technically extraneous because the inode chunk free is
+ * deferred (it occurs after a transaction roll). Include the extra reservation
+ * anyways since we've had reports of ifree transaction overruns due to too many
+ * agfl fixups during inode chunk frees.
  */
 STATIC uint
 xfs_calc_ifree_reservation(
@@ -503,7 +537,7 @@ xfs_calc_ifree_reservation(
 		xfs_calc_inode_res(mp, 1) +
 		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
 		xfs_calc_iunlink_remove_reservation(mp) +
-		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
+		xfs_calc_inode_chunk_res(mp, _FREE) +
 		xfs_calc_inobt_res(mp) +
 		xfs_calc_finobt_res(mp);
 }
-- 
2.13.6

Re: [PATCH v3 6/7] xfs: refactor inode chunk alloc/free tx reservation

[Darrick J. Wong]

On Mon, Dec 04, 2017 at 07:21:55AM -0500, Brian Foster wrote:
> The reservation for the various forms of inode allocation is
> scattered across several different functions. This includes two
> variants of chunk allocation (v5 icreate transactions vs. older
> create transactions) and the inode free transaction.
> 
> To clean up some of this code and clarify the purpose of specific
> allocfree reservations, continue the pattern of defining helper
> functions for smaller operational units of broader transactions.
> Refactor the reservation into an inode chunk alloc/free helper that
> considers the various conditions based on filesystem format.
> 
> An inode chunk free involves an extent free and buffer
> invalidations. The latter requires reservation for log headers only.
> An inode chunk allocation modifies the free space btrees and logs
> the chunk on v4 supers. v5 supers initialize the inode chunk using
> ordered buffers and so do not log the chunk.
> 
> As a side effect of this refactoring, add one more allocfree res to
> the ifree transaction. Technically this does not serve a specific
> purpose because inode chunks are freed via deferred operations and
> thus occur after a transaction roll. tr_ifree has a bit of a history
> of tx overruns caused by too many agfl fixups during sustained file
> deletion workloads, so add this extra reservation as a form of
> padding nonetheless.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> ---
> 
> v3:
> - Drop undefs.
> 
>  fs/xfs/libxfs/xfs_trans_resv.c | 64 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 19f3a226a357..75259a1346eb 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -34,6 +34,9 @@
>  #include "xfs_trans_space.h"
>  #include "xfs_trace.h"
>  
> +#define _ALLOC	true
> +#define _FREE	false
> +
>  /*
>   * A buffer has a format structure overhead in the log in addition
>   * to the data, so we need to take this into account when reserving
> @@ -172,6 +175,41 @@ xfs_calc_finobt_res(
>  }
>  
>  /*
> + * Calculate the reservation required to allocate or free an inode chunk. This
> + * includes:
> + *
> + * the allocation btrees: 2 trees * (max depth - 1) * block size
> + * the inode chunk: m_ialloc_blks * N
> + *
> + * The size N of the inode chunk reservation depends on whether it is for
> + * allocation or free and which type of create transaction is in use. An inode
> + * chunk free always invalidates the buffers and only requires reservation for
> + * headers (N == 0). An inode chunk allocation requires a chunk sized
> + * reservation on v4 and older superblocks to initialize the chunk. No chunk
> + * reservation is required for allocation on v5 supers, which use ordered
> + * buffers to initialize.
> + */
> +STATIC uint
> +xfs_calc_inode_chunk_res(
> +	struct xfs_mount	*mp,
> +	bool			alloc)
> +{
> +	uint			res, size = 0;
> +
> +	res = xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> +			       XFS_FSB_TO_B(mp, 1));
> +	if (alloc) {
> +		/* icreate tx uses ordered buffers */
> +		if (xfs_sb_version_hascrc(&mp->m_sb))
> +			return res;

Hmmm.  I had a moment of "wait, we're exiting early??" but realized that
size is zero, so jumping out early is fine.

Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> +		size = XFS_FSB_TO_B(mp, 1);
> +	}
> +
> +	res += xfs_calc_buf_res(mp->m_ialloc_blks, size);
> +	return res;
> +}
> +
> +/*
>   * Various log reservation values.
>   *
>   * These are based on the size of the file system block because that is what
> @@ -379,8 +417,7 @@ xfs_calc_create_resv_modify(
>   * For create we can allocate some inodes giving:
>   *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
>   *    the superblock for the nlink flag: sector size
> - *    the inode blocks allocated: mp->m_ialloc_blks * blocksize
> - *    the allocation btrees: 2 trees * (max depth - 1) * block size
> + *    the inode chunk (allocation/init)
>   *    the inode btree (record insertion)
>   */
>  STATIC uint
> @@ -389,9 +426,7 @@ xfs_calc_create_resv_alloc(
>  {
>  	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
>  		mp->m_sb.sb_sectsize +
> -		xfs_calc_buf_res(mp->m_ialloc_blks, XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_inode_chunk_res(mp, _ALLOC) +
>  		xfs_calc_inobt_res(mp);
>  }
>  
> @@ -408,7 +443,7 @@ __xfs_calc_create_reservation(
>   * For icreate we can allocate some inodes giving:
>   *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
>   *    the superblock for the nlink flag: sector size
> - *    the allocation btrees: 2 trees * (max depth - 1) * block size
> + *    the inode chunk (allocation, no init)
>   *    the inobt (record insertion)
>   *    the finobt (record insertion)
>   */
> @@ -418,8 +453,7 @@ xfs_calc_icreate_resv_alloc(
>  {
>  	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
>  		mp->m_sb.sb_sectsize +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_inode_chunk_res(mp, _ALLOC) +
>  		xfs_calc_inobt_res(mp) +
>  		xfs_calc_finobt_res(mp);
>  }
> @@ -485,15 +519,15 @@ xfs_calc_symlink_reservation(
>   *    the inode being freed: inode size
>   *    the super block free inode counter, AGF and AGFL: sector size
>   *    the on disk inode (agi unlinked list removal)
> - *    the inode chunk is marked stale (headers only)
> + *    the inode chunk (invalidated, headers only)
>   *    the inode btree
>   *    the finobt (record insertion, removal or modification)
>   *
> - * Note that the allocfree res. for the inode chunk itself is not included
> - * because the extent free occurs after a transaction roll. We could take the
> - * maximum of the pre/post roll operations, but the pre-roll reservation already
> - * includes at least one allocfree res. for the inobt and is thus guaranteed to
> - * be larger.
> + * Note that the inode chunk res. includes an allocfree res. for freeing of the
> + * inode chunk. This is technically extraneous because the inode chunk free is
> + * deferred (it occurs after a transaction roll). Include the extra reservation
> + * anyways since we've had reports of ifree transaction overruns due to too many
> + * agfl fixups during inode chunk frees.
>   */
>  STATIC uint
>  xfs_calc_ifree_reservation(
> @@ -503,7 +537,7 @@ xfs_calc_ifree_reservation(
>  		xfs_calc_inode_res(mp, 1) +
>  		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
>  		xfs_calc_iunlink_remove_reservation(mp) +
> -		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
> +		xfs_calc_inode_chunk_res(mp, _FREE) +
>  		xfs_calc_inobt_res(mp) +
>  		xfs_calc_finobt_res(mp);
>  }
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 7/7] xfs: eliminate duplicate icreate tx reservation functions

[Brian Foster]

The create transaction reservation calculation has two different
branches of code depending on whether the filesystem is a v5 format
fs or older. Each branch considers the max reservation between the
allocation case (new chunk allocation + record insert) and the
modify case (chunk exists, record modification) of inode allocation.

The modify case is the same for both superblock versions with the
exception of the finobt. The finobt helper checks the feature bit,
however, and so the modify case already shares the same code.

Now that inode chunk allocation has been refactored into a helper
that checks the superblock version to calculate the appropriate
reservation for the create transaction, the only remaining
difference between the create and icreate branches is the call to
the finobt helper. As noted above, the finobt helper is a no-op when
the feature is not enabled. Therefore, these branches are
effectively duplicate and can be condensed.

Remove the xfs_calc_create_*() branch of functions and update the
various callers to use the xfs_calc_icreate_*() variant. The latter
creates the same reservation size for v4 create transactions as the
removed branch. As such, this patch does not result in transaction
reservation changes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++-------------------------------------
 1 file changed, 6 insertions(+), 46 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 432dd7d7afea..f91e6680b0c5 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -414,38 +414,12 @@ xfs_calc_create_resv_modify(
 }
 
 /*
- * For create we can allocate some inodes giving:
- *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
- *    the superblock for the nlink flag: sector size
- *    the inode chunk (allocation/init)
- *    the inode btree (record insertion)
- */
-STATIC uint
-xfs_calc_create_resv_alloc(
-	struct xfs_mount	*mp)
-{
-	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
-		mp->m_sb.sb_sectsize +
-		xfs_calc_inode_chunk_res(mp, _ALLOC) +
-		xfs_calc_inobt_res(mp);
-}
-
-STATIC uint
-__xfs_calc_create_reservation(
-	struct xfs_mount	*mp)
-{
-	return XFS_DQUOT_LOGRES(mp) +
-		MAX(xfs_calc_create_resv_alloc(mp),
-		    xfs_calc_create_resv_modify(mp));
-}
-
-/*
  * For icreate we can allocate some inodes giving:
  *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
  *    the superblock for the nlink flag: sector size
- *    the inode chunk (allocation, no init)
+ *    the inode chunk (allocation, optional init)
  *    the inobt (record insertion)
- *    the finobt (record insertion)
+ *    the finobt (optional, record insertion)
  */
 STATIC uint
 xfs_calc_icreate_resv_alloc(
@@ -467,26 +441,12 @@ xfs_calc_icreate_reservation(xfs_mount_t *mp)
 }
 
 STATIC uint
-xfs_calc_create_reservation(
-	struct xfs_mount	*mp)
-{
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		return xfs_calc_icreate_reservation(mp);
-	return __xfs_calc_create_reservation(mp);
-
-}
-
-STATIC uint
 xfs_calc_create_tmpfile_reservation(
 	struct xfs_mount        *mp)
 {
 	uint	res = XFS_DQUOT_LOGRES(mp);
 
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		res += xfs_calc_icreate_resv_alloc(mp);
-	else
-		res += xfs_calc_create_resv_alloc(mp);
-
+	res += xfs_calc_icreate_resv_alloc(mp);
 	return res + xfs_calc_iunlink_add_reservation(mp);
 }
 
@@ -497,7 +457,7 @@ STATIC uint
 xfs_calc_mkdir_reservation(
 	struct xfs_mount	*mp)
 {
-	return xfs_calc_create_reservation(mp);
+	return xfs_calc_icreate_reservation(mp);
 }
 
 
@@ -510,7 +470,7 @@ STATIC uint
 xfs_calc_symlink_reservation(
 	struct xfs_mount	*mp)
 {
-	return xfs_calc_create_reservation(mp) +
+	return xfs_calc_icreate_reservation(mp) +
 	       xfs_calc_buf_res(1, XFS_SYMLINK_MAXLEN);
 }
 
@@ -872,7 +832,7 @@ xfs_trans_resv_calc(
 	resp->tr_symlink.tr_logcount = XFS_SYMLINK_LOG_COUNT;
 	resp->tr_symlink.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
-	resp->tr_create.tr_logres = xfs_calc_create_reservation(mp);
+	resp->tr_create.tr_logres = xfs_calc_icreate_reservation(mp);
 	resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT;
 	resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
-- 
2.13.6

Re: [PATCH v2 7/7] xfs: eliminate duplicate icreate tx reservation functions

[Dave Chinner]

On Thu, Nov 30, 2017 at 01:58:36PM -0500, Brian Foster wrote:
> The create transaction reservation calculation has two different
> branches of code depending on whether the filesystem is a v5 format
> fs or older. Each branch considers the max reservation between the
> allocation case (new chunk allocation + record insert) and the
> modify case (chunk exists, record modification) of inode allocation.
> 
> The modify case is the same for both superblock versions with the
> exception of the finobt. The finobt helper checks the feature bit,
> however, and so the modify case already shares the same code.
> 
> Now that inode chunk allocation has been refactored into a helper
> that checks the superblock version to calculate the appropriate
> reservation for the create transaction, the only remaining
> difference between the create and icreate branches is the call to
> the finobt helper. As noted above, the finobt helper is a no-op when
> the feature is not enabled. Therefore, these branches are
> effectively duplicate and can be condensed.
> 
> Remove the xfs_calc_create_*() branch of functions and update the
> various callers to use the xfs_calc_icreate_*() variant. The latter
> creates the same reservation size for v4 create transactions as the
> removed branch. As such, this patch does not result in transaction
> reservation changes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Nice simplification. Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 7/7] xfs: eliminate duplicate icreate tx reservation functions

[Darrick J. Wong]

On Thu, Nov 30, 2017 at 01:58:36PM -0500, Brian Foster wrote:
> The create transaction reservation calculation has two different
> branches of code depending on whether the filesystem is a v5 format
> fs or older. Each branch considers the max reservation between the
> allocation case (new chunk allocation + record insert) and the
> modify case (chunk exists, record modification) of inode allocation.
> 
> The modify case is the same for both superblock versions with the
> exception of the finobt. The finobt helper checks the feature bit,
> however, and so the modify case already shares the same code.
> 
> Now that inode chunk allocation has been refactored into a helper
> that checks the superblock version to calculate the appropriate
> reservation for the create transaction, the only remaining
> difference between the create and icreate branches is the call to
> the finobt helper. As noted above, the finobt helper is a no-op when
> the feature is not enabled. Therefore, these branches are
> effectively duplicate and can be condensed.
> 
> Remove the xfs_calc_create_*() branch of functions and update the
> various callers to use the xfs_calc_icreate_*() variant. The latter
> creates the same reservation size for v4 create transactions as the
> removed branch. As such, this patch does not result in transaction
> reservation changes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok for 4.16, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++-------------------------------------
>  1 file changed, 6 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 432dd7d7afea..f91e6680b0c5 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -414,38 +414,12 @@ xfs_calc_create_resv_modify(
>  }
>  
>  /*
> - * For create we can allocate some inodes giving:
> - *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
> - *    the superblock for the nlink flag: sector size
> - *    the inode chunk (allocation/init)
> - *    the inode btree (record insertion)
> - */
> -STATIC uint
> -xfs_calc_create_resv_alloc(
> -	struct xfs_mount	*mp)
> -{
> -	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
> -		mp->m_sb.sb_sectsize +
> -		xfs_calc_inode_chunk_res(mp, _ALLOC) +
> -		xfs_calc_inobt_res(mp);
> -}
> -
> -STATIC uint
> -__xfs_calc_create_reservation(
> -	struct xfs_mount	*mp)
> -{
> -	return XFS_DQUOT_LOGRES(mp) +
> -		MAX(xfs_calc_create_resv_alloc(mp),
> -		    xfs_calc_create_resv_modify(mp));
> -}
> -
> -/*
>   * For icreate we can allocate some inodes giving:
>   *    the agi and agf of the ag getting the new inodes: 2 * sectorsize
>   *    the superblock for the nlink flag: sector size
> - *    the inode chunk (allocation, no init)
> + *    the inode chunk (allocation, optional init)
>   *    the inobt (record insertion)
> - *    the finobt (record insertion)
> + *    the finobt (optional, record insertion)
>   */
>  STATIC uint
>  xfs_calc_icreate_resv_alloc(
> @@ -467,26 +441,12 @@ xfs_calc_icreate_reservation(xfs_mount_t *mp)
>  }
>  
>  STATIC uint
> -xfs_calc_create_reservation(
> -	struct xfs_mount	*mp)
> -{
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		return xfs_calc_icreate_reservation(mp);
> -	return __xfs_calc_create_reservation(mp);
> -
> -}
> -
> -STATIC uint
>  xfs_calc_create_tmpfile_reservation(
>  	struct xfs_mount        *mp)
>  {
>  	uint	res = XFS_DQUOT_LOGRES(mp);
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		res += xfs_calc_icreate_resv_alloc(mp);
> -	else
> -		res += xfs_calc_create_resv_alloc(mp);
> -
> +	res += xfs_calc_icreate_resv_alloc(mp);
>  	return res + xfs_calc_iunlink_add_reservation(mp);
>  }
>  
> @@ -497,7 +457,7 @@ STATIC uint
>  xfs_calc_mkdir_reservation(
>  	struct xfs_mount	*mp)
>  {
> -	return xfs_calc_create_reservation(mp);
> +	return xfs_calc_icreate_reservation(mp);
>  }
>  
>  
> @@ -510,7 +470,7 @@ STATIC uint
>  xfs_calc_symlink_reservation(
>  	struct xfs_mount	*mp)
>  {
> -	return xfs_calc_create_reservation(mp) +
> +	return xfs_calc_icreate_reservation(mp) +
>  	       xfs_calc_buf_res(1, XFS_SYMLINK_MAXLEN);
>  }
>  
> @@ -872,7 +832,7 @@ xfs_trans_resv_calc(
>  	resp->tr_symlink.tr_logcount = XFS_SYMLINK_LOG_COUNT;
>  	resp->tr_symlink.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>  
> -	resp->tr_create.tr_logres = xfs_calc_create_reservation(mp);
> +	resp->tr_create.tr_logres = xfs_calc_icreate_reservation(mp);
>  	resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT;
>  	resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>  
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] xfs: inode transaction reservation fixups

[Brian Foster]

On Thu, Nov 30, 2017 at 01:58:29PM -0500, Brian Foster wrote:
> Hi all,
> 
> Here's v2 of the inode tx reservation fixups. This drops the agfl fixup
> patch for the time being and adds some additional reservation fixups and
> refactoring based on Dave's feedback. This survives xfstests in a few
> different configurations and also happens to survive my inode unlink
> reservation overrun tests.
> 
> Thoughts, reviews, flames appreciated.
> 
> Brian
> 
> v2:
> - Update AGI unlinked list and truncate reservations
> - Update commit log for ifree refactor patch.
> - Additional refactoring patches.
> - Dropped agfl fixup patch.
> v1: https://marc.info/?l=linux-xfs&m=151181428031884&w=2
> 

Ping...

It looks like this is all reviewed, but perhaps just not merged yet (or
I'm just not looking in the right place)..?

Brian

> Brian Foster (7):
>   xfs: print transaction log reservation on overrun
>   xfs: include inobt buffers in ifree tx log reservation
>   xfs: fix up agi unlinked list reservations
>   xfs: truncate transaction does not modify the inobt
>   xfs: include an allocfree res for inobt modifications
>   xfs: refactor inode chunk alloc/free tx reservation
>   xfs: eliminate duplicate icreate tx reservation functions
> 
>  fs/xfs/libxfs/xfs_trans_resv.c | 202 ++++++++++++++++++++---------------------
>  fs/xfs/xfs_log.c               |   4 +-
>  2 files changed, 99 insertions(+), 107 deletions(-)
> 
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] xfs: inode transaction reservation fixups

[Darrick J. Wong]

On Mon, Jan 08, 2018 at 09:08:44AM -0500, Brian Foster wrote:
> On Thu, Nov 30, 2017 at 01:58:29PM -0500, Brian Foster wrote:
> > Hi all,
> > 
> > Here's v2 of the inode tx reservation fixups. This drops the agfl fixup
> > patch for the time being and adds some additional reservation fixups and
> > refactoring based on Dave's feedback. This survives xfstests in a few
> > different configurations and also happens to survive my inode unlink
> > reservation overrun tests.
> > 
> > Thoughts, reviews, flames appreciated.
> > 
> > Brian
> > 
> > v2:
> > - Update AGI unlinked list and truncate reservations
> > - Update commit log for ifree refactor patch.
> > - Additional refactoring patches.
> > - Dropped agfl fixup patch.
> > v1: https://marc.info/?l=linux-xfs&m=151181428031884&w=2
> > 
> 
> Ping...
> 
> It looks like this is all reviewed, but perhaps just not merged yet (or
> I'm just not looking in the right place)..?

It's all reviewed and ready to go, but I haven't had a chance to push
things out for testing due to last week's ... uh, fire drill. :/

Anyway, that's all over so I'll go work on that today.

--D

> Brian
> 
> > Brian Foster (7):
> >   xfs: print transaction log reservation on overrun
> >   xfs: include inobt buffers in ifree tx log reservation
> >   xfs: fix up agi unlinked list reservations
> >   xfs: truncate transaction does not modify the inobt
> >   xfs: include an allocfree res for inobt modifications
> >   xfs: refactor inode chunk alloc/free tx reservation
> >   xfs: eliminate duplicate icreate tx reservation functions
> > 
> >  fs/xfs/libxfs/xfs_trans_resv.c | 202 ++++++++++++++++++++---------------------
> >  fs/xfs/xfs_log.c               |   4 +-
> >  2 files changed, 99 insertions(+), 107 deletions(-)
> > 
> > -- 
> > 2.13.6
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html