two small log recovery cleanups

two small log recovery cleanups

[Christoph Hellwig]

Hi all,

two small log recovery cleanups based on my review of the refactoring
series from Darrick.

[PATCH 1/2] xfs: refactor the buffer cancellation table helpers

[Christoph Hellwig]

Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and
xlog_check_buffer_cancelled with two obvious helpers:

 xlog_is_buffer_cancelled, which returns true if there is a buffer in
 the cancellation table, and
 xlog_put_buffer_cancelled, which also decrements the reference count
 of the buffer cancellation table.

Both share a little helper to look up the entry.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 59 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 11c3502b07b13..750a81b941ea4 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1(
 	return 0;
 }
 
-/*
- * Check to see whether the buffer being recovered has a corresponding
- * entry in the buffer cancel record table. If it is, return the cancel
- * buffer structure to the caller.
- */
-STATIC struct xfs_buf_cancel *
-xlog_peek_buffer_cancelled(
+static struct xfs_buf_cancel *
+xlog_find_buffer_cancelled(
 	struct xlog		*log,
 	xfs_daddr_t		blkno,
-	uint			len,
-	unsigned short			flags)
+	uint			len)
 {
 	struct list_head	*bucket;
 	struct xfs_buf_cancel	*bcp;
 
-	if (!log->l_buf_cancel_table) {
-		/* empty table means no cancelled buffers in the log */
-		ASSERT(!(flags & XFS_BLF_CANCEL));
+	if (!log->l_buf_cancel_table)
 		return NULL;
-	}
 
 	bucket = XLOG_BUF_CANCEL_BUCKET(log, blkno);
 	list_for_each_entry(bcp, bucket, bc_list) {
@@ -1999,50 +1990,48 @@ xlog_peek_buffer_cancelled(
 			return bcp;
 	}
 
-	/*
-	 * We didn't find a corresponding entry in the table, so return 0 so
-	 * that the buffer is NOT cancelled.
-	 */
-	ASSERT(!(flags & XFS_BLF_CANCEL));
 	return NULL;
 }
 
 /*
- * If the buffer is being cancelled then return 1 so that it will be cancelled,
- * otherwise return 0.  If the buffer is actually a buffer cancel item
- * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the
- * table and remove it from the table if this is the last reference.
+ * Check if there is and entry for blkno, len in the buffer cancel record table.
+ */
+static bool
+xlog_is_buffer_cancelled(
+	struct xlog		*log,
+	xfs_daddr_t		blkno,
+	uint			len)
+{
+	return xlog_find_buffer_cancelled(log, blkno, len) != NULL;
+}
+
+/*
+ * Check if there is and entry for blkno, len in the buffer cancel record table,
+ * and decremented the reference count on it if there is one.
  *
- * We remove the cancel record from the table when we encounter its last
- * occurrence in the log so that if the same buffer is re-used again after its
- * last cancellation we actually replay the changes made at that point.
+ * Remove the cancel record once the refcount hits zero, so that if the same
+ * buffer is re-used again after its last cancellation we actually replay the
+ * changes made at that point.
  */
-STATIC int
-xlog_check_buffer_cancelled(
+static bool
+xlog_put_buffer_cancelled(
 	struct xlog		*log,
 	xfs_daddr_t		blkno,
-	uint			len,
-	unsigned short			flags)
+	uint			len)
 {
 	struct xfs_buf_cancel	*bcp;
 
-	bcp = xlog_peek_buffer_cancelled(log, blkno, len, flags);
-	if (!bcp)
-		return 0;
+	bcp = xlog_find_buffer_cancelled(log, blkno, len);
+	if (!bcp) {
+		ASSERT(0);
+		return false;
+	}
 
-	/*
-	 * We've go a match, so return 1 so that the recovery of this buffer
-	 * is cancelled.  If this buffer is actually a buffer cancel log
-	 * item, then decrement the refcount on the one in the table and
-	 * remove it if this is the last reference.
-	 */
-	if (flags & XFS_BLF_CANCEL) {
-		if (--bcp->bc_refcount == 0) {
-			list_del(&bcp->bc_list);
-			kmem_free(bcp);
-		}
+	if (--bcp->bc_refcount == 0) {
+		list_del(&bcp->bc_list);
+		kmem_free(bcp);
 	}
-	return 1;
+	return true;
 }
 
 /*
@@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2(
 	 * In this pass we only want to recover all the buffers which have
 	 * not been cancelled and are not cancellation buffers themselves.
 	 */
-	if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
-			buf_f->blf_len, buf_f->blf_flags)) {
-		trace_xfs_log_recover_buf_cancel(log, buf_f);
-		return 0;
+	if (buf_f->blf_flags & XFS_BLF_CANCEL) {
+		if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno,
+				buf_f->blf_len))
+			goto cancelled;
+	} else {
+
+		if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno,
+				buf_f->blf_len))
+			goto cancelled;
 	}
 
 	trace_xfs_log_recover_buf_recover(log, buf_f);
@@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2(
 out_release:
 	xfs_buf_relse(bp);
 	return error;
+cancelled:
+	trace_xfs_log_recover_buf_cancel(log, buf_f);
+	return 0;
 }
 
 /*
@@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2(
 	 * Inode buffers can be freed, look out for it,
 	 * and do not replay the inode.
 	 */
-	if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno,
-					in_f->ilf_len, 0)) {
+	if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) {
 		error = 0;
 		trace_xfs_log_recover_inode_cancel(log, in_f);
 		goto error;
@@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2(
 
 		daddr = XFS_AGB_TO_DADDR(mp, agno,
 				agbno + i * igeo->blocks_per_cluster);
-		if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0))
+		if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster))
 			cancel_count++;
 	}
 
@@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2(
 	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
 	struct xfs_mount		*mp = log->l_mp;
 
-	if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
-			buf_f->blf_len, buf_f->blf_flags)) {
+	if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len))
 		return;
-	}
-
 	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
 				buf_f->blf_len, NULL);
 }
@@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2(
 			return;
 	}
 
-	if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
+	if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len))
 		return;
-
 	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
 				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
 }
@@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2(
 	ASSERT(dq_f->qlf_len == 1);
 
 	len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
-	if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
+	if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len))
 		return;
-
 	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
 			  &xfs_dquot_buf_ra_ops);
 }
-- 
2.26.1

[PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans

[Christoph Hellwig]

This list contains pretty much everything that is not a buffer.  The
comment calls it item_list, which is a much better name than inode
list, so switch the actual variable name to that as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_recover.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 750a81b941ea4..33cac61570abe 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1847,7 +1847,7 @@ xlog_recover_reorder_trans(
 	LIST_HEAD(cancel_list);
 	LIST_HEAD(buffer_list);
 	LIST_HEAD(inode_buffer_list);
-	LIST_HEAD(inode_list);
+	LIST_HEAD(item_list);
 
 	list_splice_init(&trans->r_itemq, &sort_list);
 	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
@@ -1883,7 +1883,7 @@ xlog_recover_reorder_trans(
 		case XFS_LI_BUD:
 			trace_xfs_log_recover_item_reorder_tail(log,
 							trans, item, pass);
-			list_move_tail(&item->ri_list, &inode_list);
+			list_move_tail(&item->ri_list, &item_list);
 			break;
 		default:
 			xfs_warn(log->l_mp,
@@ -1904,8 +1904,8 @@ xlog_recover_reorder_trans(
 	ASSERT(list_empty(&sort_list));
 	if (!list_empty(&buffer_list))
 		list_splice(&buffer_list, &trans->r_itemq);
-	if (!list_empty(&inode_list))
-		list_splice_tail(&inode_list, &trans->r_itemq);
+	if (!list_empty(&item_list))
+		list_splice_tail(&item_list, &trans->r_itemq);
 	if (!list_empty(&inode_buffer_list))
 		list_splice_tail(&inode_buffer_list, &trans->r_itemq);
 	if (!list_empty(&cancel_list))
-- 
2.26.1

Re: [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans

[Darrick J. Wong]

On Mon, Apr 27, 2020 at 03:52:29PM +0200, Christoph Hellwig wrote:
> This list contains pretty much everything that is not a buffer.  The
> comment calls it item_list, which is a much better name than inode
> list, so switch the actual variable name to that as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

LGTM, who doesn't know where the "inode_list" name came from... :)

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

--D

> ---
>  fs/xfs/xfs_log_recover.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 750a81b941ea4..33cac61570abe 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1847,7 +1847,7 @@ xlog_recover_reorder_trans(
>  	LIST_HEAD(cancel_list);
>  	LIST_HEAD(buffer_list);
>  	LIST_HEAD(inode_buffer_list);
> -	LIST_HEAD(inode_list);
> +	LIST_HEAD(item_list);
>  
>  	list_splice_init(&trans->r_itemq, &sort_list);
>  	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
> @@ -1883,7 +1883,7 @@ xlog_recover_reorder_trans(
>  		case XFS_LI_BUD:
>  			trace_xfs_log_recover_item_reorder_tail(log,
>  							trans, item, pass);
> -			list_move_tail(&item->ri_list, &inode_list);
> +			list_move_tail(&item->ri_list, &item_list);
>  			break;
>  		default:
>  			xfs_warn(log->l_mp,
> @@ -1904,8 +1904,8 @@ xlog_recover_reorder_trans(
>  	ASSERT(list_empty(&sort_list));
>  	if (!list_empty(&buffer_list))
>  		list_splice(&buffer_list, &trans->r_itemq);
> -	if (!list_empty(&inode_list))
> -		list_splice_tail(&inode_list, &trans->r_itemq);
> +	if (!list_empty(&item_list))
> +		list_splice_tail(&item_list, &trans->r_itemq);
>  	if (!list_empty(&inode_buffer_list))
>  		list_splice_tail(&inode_buffer_list, &trans->r_itemq);
>  	if (!list_empty(&cancel_list))
> -- 
> 2.26.1
> 

Re: [PATCH 1/2] xfs: refactor the buffer cancellation table helpers

[Darrick J. Wong]

On Mon, Apr 27, 2020 at 03:52:28PM +0200, Christoph Hellwig wrote:
> Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and
> xlog_check_buffer_cancelled with two obvious helpers:
> 
>  xlog_is_buffer_cancelled, which returns true if there is a buffer in
>  the cancellation table, and
>  xlog_put_buffer_cancelled, which also decrements the reference count
>  of the buffer cancellation table.
> 
> Both share a little helper to look up the entry.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok, though it took me a while to wrap my head around the
convolution.  Will give all of these a spin.

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

--D

> ---
>  fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++---------------------
>  1 file changed, 50 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 11c3502b07b13..750a81b941ea4 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1(
>  	return 0;
>  }
>  
> -/*
> - * Check to see whether the buffer being recovered has a corresponding
> - * entry in the buffer cancel record table. If it is, return the cancel
> - * buffer structure to the caller.
> - */
> -STATIC struct xfs_buf_cancel *
> -xlog_peek_buffer_cancelled(
> +static struct xfs_buf_cancel *
> +xlog_find_buffer_cancelled(
>  	struct xlog		*log,
>  	xfs_daddr_t		blkno,
> -	uint			len,
> -	unsigned short			flags)
> +	uint			len)
>  {
>  	struct list_head	*bucket;
>  	struct xfs_buf_cancel	*bcp;
>  
> -	if (!log->l_buf_cancel_table) {
> -		/* empty table means no cancelled buffers in the log */
> -		ASSERT(!(flags & XFS_BLF_CANCEL));
> +	if (!log->l_buf_cancel_table)
>  		return NULL;
> -	}
>  
>  	bucket = XLOG_BUF_CANCEL_BUCKET(log, blkno);
>  	list_for_each_entry(bcp, bucket, bc_list) {
> @@ -1999,50 +1990,48 @@ xlog_peek_buffer_cancelled(
>  			return bcp;
>  	}
>  
> -	/*
> -	 * We didn't find a corresponding entry in the table, so return 0 so
> -	 * that the buffer is NOT cancelled.
> -	 */
> -	ASSERT(!(flags & XFS_BLF_CANCEL));
>  	return NULL;
>  }
>  
>  /*
> - * If the buffer is being cancelled then return 1 so that it will be cancelled,
> - * otherwise return 0.  If the buffer is actually a buffer cancel item
> - * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the
> - * table and remove it from the table if this is the last reference.
> + * Check if there is and entry for blkno, len in the buffer cancel record table.
> + */
> +static bool
> +xlog_is_buffer_cancelled(
> +	struct xlog		*log,
> +	xfs_daddr_t		blkno,
> +	uint			len)
> +{
> +	return xlog_find_buffer_cancelled(log, blkno, len) != NULL;
> +}
> +
> +/*
> + * Check if there is and entry for blkno, len in the buffer cancel record table,
> + * and decremented the reference count on it if there is one.
>   *
> - * We remove the cancel record from the table when we encounter its last
> - * occurrence in the log so that if the same buffer is re-used again after its
> - * last cancellation we actually replay the changes made at that point.
> + * Remove the cancel record once the refcount hits zero, so that if the same
> + * buffer is re-used again after its last cancellation we actually replay the
> + * changes made at that point.
>   */
> -STATIC int
> -xlog_check_buffer_cancelled(
> +static bool
> +xlog_put_buffer_cancelled(
>  	struct xlog		*log,
>  	xfs_daddr_t		blkno,
> -	uint			len,
> -	unsigned short			flags)
> +	uint			len)
>  {
>  	struct xfs_buf_cancel	*bcp;
>  
> -	bcp = xlog_peek_buffer_cancelled(log, blkno, len, flags);
> -	if (!bcp)
> -		return 0;
> +	bcp = xlog_find_buffer_cancelled(log, blkno, len);
> +	if (!bcp) {
> +		ASSERT(0);
> +		return false;
> +	}
>  
> -	/*
> -	 * We've go a match, so return 1 so that the recovery of this buffer
> -	 * is cancelled.  If this buffer is actually a buffer cancel log
> -	 * item, then decrement the refcount on the one in the table and
> -	 * remove it if this is the last reference.
> -	 */
> -	if (flags & XFS_BLF_CANCEL) {
> -		if (--bcp->bc_refcount == 0) {
> -			list_del(&bcp->bc_list);
> -			kmem_free(bcp);
> -		}
> +	if (--bcp->bc_refcount == 0) {
> +		list_del(&bcp->bc_list);
> +		kmem_free(bcp);
>  	}
> -	return 1;
> +	return true;
>  }
>  
>  /*
> @@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2(
>  	 * In this pass we only want to recover all the buffers which have
>  	 * not been cancelled and are not cancellation buffers themselves.
>  	 */
> -	if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
> -			buf_f->blf_len, buf_f->blf_flags)) {
> -		trace_xfs_log_recover_buf_cancel(log, buf_f);
> -		return 0;
> +	if (buf_f->blf_flags & XFS_BLF_CANCEL) {
> +		if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno,
> +				buf_f->blf_len))
> +			goto cancelled;
> +	} else {
> +
> +		if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno,
> +				buf_f->blf_len))
> +			goto cancelled;
>  	}
>  
>  	trace_xfs_log_recover_buf_recover(log, buf_f);
> @@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2(
>  out_release:
>  	xfs_buf_relse(bp);
>  	return error;
> +cancelled:
> +	trace_xfs_log_recover_buf_cancel(log, buf_f);
> +	return 0;
>  }
>  
>  /*
> @@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2(
>  	 * Inode buffers can be freed, look out for it,
>  	 * and do not replay the inode.
>  	 */
> -	if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno,
> -					in_f->ilf_len, 0)) {
> +	if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) {
>  		error = 0;
>  		trace_xfs_log_recover_inode_cancel(log, in_f);
>  		goto error;
> @@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2(
>  
>  		daddr = XFS_AGB_TO_DADDR(mp, agno,
>  				agbno + i * igeo->blocks_per_cluster);
> -		if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0))
> +		if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster))
>  			cancel_count++;
>  	}
>  
> @@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2(
>  	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
>  	struct xfs_mount		*mp = log->l_mp;
>  
> -	if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
> -			buf_f->blf_len, buf_f->blf_flags)) {
> +	if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len))
>  		return;
> -	}
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
>  				buf_f->blf_len, NULL);
>  }
> @@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2(
>  			return;
>  	}
>  
> -	if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> +	if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len))
>  		return;
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
>  				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
>  }
> @@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2(
>  	ASSERT(dq_f->qlf_len == 1);
>  
>  	len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
> -	if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
> +	if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len))
>  		return;
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
>  			  &xfs_dquot_buf_ra_ops);
>  }
> -- 
> 2.26.1
> 

[PATCH 3/2] xfs: factor out a xlog_buf_readahead helper

[Christoph Hellwig]

Add a little helper to readahead a buffer if it hasn't been cancelled.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_recover.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 33cac61570abe..4cb8f24f3aa63 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2034,6 +2034,17 @@ xlog_put_buffer_cancelled(
 	return true;
 }
 
+static void
+xlog_buf_readahead(
+	struct xlog		*log,
+	xfs_daddr_t		blkno,
+	uint			len,
+	const struct xfs_buf_ops *ops)
+{
+	if (!xlog_is_buffer_cancelled(log, blkno, len))
+		xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
+}
+
 /*
  * Perform recovery for a buffer full of inodes.  In these buffers, the only
  * data which should be recovered is that which corresponds to the
@@ -3870,12 +3881,8 @@ xlog_recover_buffer_ra_pass2(
 	struct xlog_recover_item        *item)
 {
 	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
-	struct xfs_mount		*mp = log->l_mp;
 
-	if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len))
-		return;
-	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
-				buf_f->blf_len, NULL);
+	xlog_buf_readahead(log, buf_f->blf_blkno, buf_f->blf_len, NULL);
 }
 
 STATIC void
@@ -3885,7 +3892,6 @@ xlog_recover_inode_ra_pass2(
 {
 	struct xfs_inode_log_format	ilf_buf;
 	struct xfs_inode_log_format	*ilfp;
-	struct xfs_mount		*mp = log->l_mp;
 	int			error;
 
 	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
@@ -3898,10 +3904,8 @@ xlog_recover_inode_ra_pass2(
 			return;
 	}
 
-	if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len))
-		return;
-	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
-				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
+	xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
+			   &xfs_inode_buf_ra_ops);
 }
 
 STATIC void
@@ -3913,8 +3917,6 @@ xlog_recover_dquot_ra_pass2(
 	struct xfs_disk_dquot	*recddq;
 	struct xfs_dq_logformat	*dq_f;
 	uint			type;
-	int			len;
-
 
 	if (mp->m_qflags == 0)
 		return;
@@ -3934,11 +3936,9 @@ xlog_recover_dquot_ra_pass2(
 	ASSERT(dq_f);
 	ASSERT(dq_f->qlf_len == 1);
 
-	len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
-	if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len))
-		return;
-	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
-			  &xfs_dquot_buf_ra_ops);
+	xlog_buf_readahead(log, dq_f->qlf_blkno,
+			XFS_FSB_TO_BB(mp, dq_f->qlf_len),
+			&xfs_dquot_buf_ra_ops);
 }
 
 STATIC void
-- 
2.26.1

[PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2

[Christoph Hellwig]

Don't bother to allocate memory and convert the log item when we
only need the block number and the length.  Just extract them directly
and call xlog_buf_readahead separately in each branch.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_recover.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 4cb8f24f3aa63..fe4dad5b77a95 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3890,22 +3890,17 @@ xlog_recover_inode_ra_pass2(
 	struct xlog                     *log,
 	struct xlog_recover_item        *item)
 {
-	struct xfs_inode_log_format	ilf_buf;
-	struct xfs_inode_log_format	*ilfp;
-	int			error;
-
 	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
-		ilfp = item->ri_buf[0].i_addr;
+		struct xfs_inode_log_format	*ilfp = item->ri_buf[0].i_addr;
+
+		xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
+				   &xfs_inode_buf_ra_ops);
 	} else {
-		ilfp = &ilf_buf;
-		memset(ilfp, 0, sizeof(*ilfp));
-		error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp);
-		if (error)
-			return;
-	}
+		struct xfs_inode_log_format_32	*ilfp = item->ri_buf[0].i_addr;
 
-	xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
-			   &xfs_inode_buf_ra_ops);
+		xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
+				   &xfs_inode_buf_ra_ops);
+	}
 }
 
 STATIC void
-- 
2.26.1

Re: [PATCH 1/2] xfs: refactor the buffer cancellation table helpers

[Brian Foster]

On Mon, Apr 27, 2020 at 03:52:28PM +0200, Christoph Hellwig wrote:
> Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and
> xlog_check_buffer_cancelled with two obvious helpers:
> 
>  xlog_is_buffer_cancelled, which returns true if there is a buffer in
>  the cancellation table, and
>  xlog_put_buffer_cancelled, which also decrements the reference count
>  of the buffer cancellation table.
> 
> Both share a little helper to look up the entry.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++---------------------
>  1 file changed, 50 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 11c3502b07b13..750a81b941ea4 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1(
>  	return 0;
>  }
>  
...
>  /*
> - * If the buffer is being cancelled then return 1 so that it will be cancelled,
> - * otherwise return 0.  If the buffer is actually a buffer cancel item
> - * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the
> - * table and remove it from the table if this is the last reference.
> + * Check if there is and entry for blkno, len in the buffer cancel record table.
> + */
> +static bool
> +xlog_is_buffer_cancelled(
> +	struct xlog		*log,
> +	xfs_daddr_t		blkno,
> +	uint			len)
> +{
> +	return xlog_find_buffer_cancelled(log, blkno, len) != NULL;
> +}
> +
> +/*
> + * Check if there is and entry for blkno, len in the buffer cancel record table,

Nit:			an

> + * and decremented the reference count on it if there is one.

Nit:	  decrement

...
> @@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2(
>  	 * In this pass we only want to recover all the buffers which have
>  	 * not been cancelled and are not cancellation buffers themselves.
>  	 */
> -	if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
> -			buf_f->blf_len, buf_f->blf_flags)) {
> -		trace_xfs_log_recover_buf_cancel(log, buf_f);
> -		return 0;
> +	if (buf_f->blf_flags & XFS_BLF_CANCEL) {
> +		if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno,
> +				buf_f->blf_len))
> +			goto cancelled;
> +	} else {
> +

Extra whitespace above. With those nits fixed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +		if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno,
> +				buf_f->blf_len))
> +			goto cancelled;
>  	}
>  
>  	trace_xfs_log_recover_buf_recover(log, buf_f);
> @@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2(
>  out_release:
>  	xfs_buf_relse(bp);
>  	return error;
> +cancelled:
> +	trace_xfs_log_recover_buf_cancel(log, buf_f);
> +	return 0;
>  }
>  
>  /*
> @@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2(
>  	 * Inode buffers can be freed, look out for it,
>  	 * and do not replay the inode.
>  	 */
> -	if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno,
> -					in_f->ilf_len, 0)) {
> +	if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) {
>  		error = 0;
>  		trace_xfs_log_recover_inode_cancel(log, in_f);
>  		goto error;
> @@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2(
>  
>  		daddr = XFS_AGB_TO_DADDR(mp, agno,
>  				agbno + i * igeo->blocks_per_cluster);
> -		if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0))
> +		if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster))
>  			cancel_count++;
>  	}
>  
> @@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2(
>  	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
>  	struct xfs_mount		*mp = log->l_mp;
>  
> -	if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
> -			buf_f->blf_len, buf_f->blf_flags)) {
> +	if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len))
>  		return;
> -	}
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
>  				buf_f->blf_len, NULL);
>  }
> @@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2(
>  			return;
>  	}
>  
> -	if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> +	if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len))
>  		return;
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
>  				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
>  }
> @@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2(
>  	ASSERT(dq_f->qlf_len == 1);
>  
>  	len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
> -	if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
> +	if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len))
>  		return;
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
>  			  &xfs_dquot_buf_ra_ops);
>  }
> -- 
> 2.26.1
> 

Re: [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans

[Brian Foster]

On Mon, Apr 27, 2020 at 03:52:29PM +0200, Christoph Hellwig wrote:
> This list contains pretty much everything that is not a buffer.  The
> comment calls it item_list, which is a much better name than inode
> list, so switch the actual variable name to that as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log_recover.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 750a81b941ea4..33cac61570abe 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1847,7 +1847,7 @@ xlog_recover_reorder_trans(
>  	LIST_HEAD(cancel_list);
>  	LIST_HEAD(buffer_list);
>  	LIST_HEAD(inode_buffer_list);
> -	LIST_HEAD(inode_list);
> +	LIST_HEAD(item_list);
>  
>  	list_splice_init(&trans->r_itemq, &sort_list);
>  	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
> @@ -1883,7 +1883,7 @@ xlog_recover_reorder_trans(
>  		case XFS_LI_BUD:
>  			trace_xfs_log_recover_item_reorder_tail(log,
>  							trans, item, pass);
> -			list_move_tail(&item->ri_list, &inode_list);
> +			list_move_tail(&item->ri_list, &item_list);
>  			break;
>  		default:
>  			xfs_warn(log->l_mp,
> @@ -1904,8 +1904,8 @@ xlog_recover_reorder_trans(
>  	ASSERT(list_empty(&sort_list));
>  	if (!list_empty(&buffer_list))
>  		list_splice(&buffer_list, &trans->r_itemq);
> -	if (!list_empty(&inode_list))
> -		list_splice_tail(&inode_list, &trans->r_itemq);
> +	if (!list_empty(&item_list))
> +		list_splice_tail(&item_list, &trans->r_itemq);
>  	if (!list_empty(&inode_buffer_list))
>  		list_splice_tail(&inode_buffer_list, &trans->r_itemq);
>  	if (!list_empty(&cancel_list))
> -- 
> 2.26.1
> 

Re: [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper

[Brian Foster]

On Mon, Apr 27, 2020 at 09:33:32PM +0200, Christoph Hellwig wrote:
> Add a little helper to readahead a buffer if it hasn't been cancelled.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log_recover.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 33cac61570abe..4cb8f24f3aa63 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2034,6 +2034,17 @@ xlog_put_buffer_cancelled(
>  	return true;
>  }
>  
> +static void
> +xlog_buf_readahead(
> +	struct xlog		*log,
> +	xfs_daddr_t		blkno,
> +	uint			len,
> +	const struct xfs_buf_ops *ops)
> +{
> +	if (!xlog_is_buffer_cancelled(log, blkno, len))
> +		xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
> +}
> +
>  /*
>   * Perform recovery for a buffer full of inodes.  In these buffers, the only
>   * data which should be recovered is that which corresponds to the
> @@ -3870,12 +3881,8 @@ xlog_recover_buffer_ra_pass2(
>  	struct xlog_recover_item        *item)
>  {
>  	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
> -	struct xfs_mount		*mp = log->l_mp;
>  
> -	if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len))
> -		return;
> -	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
> -				buf_f->blf_len, NULL);
> +	xlog_buf_readahead(log, buf_f->blf_blkno, buf_f->blf_len, NULL);
>  }
>  
>  STATIC void
> @@ -3885,7 +3892,6 @@ xlog_recover_inode_ra_pass2(
>  {
>  	struct xfs_inode_log_format	ilf_buf;
>  	struct xfs_inode_log_format	*ilfp;
> -	struct xfs_mount		*mp = log->l_mp;
>  	int			error;
>  
>  	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
> @@ -3898,10 +3904,8 @@ xlog_recover_inode_ra_pass2(
>  			return;
>  	}
>  
> -	if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len))
> -		return;
> -	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
> -				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
> +	xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
> +			   &xfs_inode_buf_ra_ops);
>  }
>  
>  STATIC void
> @@ -3913,8 +3917,6 @@ xlog_recover_dquot_ra_pass2(
>  	struct xfs_disk_dquot	*recddq;
>  	struct xfs_dq_logformat	*dq_f;
>  	uint			type;
> -	int			len;
> -
>  
>  	if (mp->m_qflags == 0)
>  		return;
> @@ -3934,11 +3936,9 @@ xlog_recover_dquot_ra_pass2(
>  	ASSERT(dq_f);
>  	ASSERT(dq_f->qlf_len == 1);
>  
> -	len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
> -	if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len))
> -		return;
> -	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
> -			  &xfs_dquot_buf_ra_ops);
> +	xlog_buf_readahead(log, dq_f->qlf_blkno,
> +			XFS_FSB_TO_BB(mp, dq_f->qlf_len),
> +			&xfs_dquot_buf_ra_ops);
>  }
>  
>  STATIC void
> -- 
> 2.26.1
> 

Re: [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2

[Brian Foster]

On Mon, Apr 27, 2020 at 09:33:49PM +0200, Christoph Hellwig wrote:
> Don't bother to allocate memory and convert the log item when we
> only need the block number and the length.  Just extract them directly
> and call xlog_buf_readahead separately in each branch.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log_recover.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 4cb8f24f3aa63..fe4dad5b77a95 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3890,22 +3890,17 @@ xlog_recover_inode_ra_pass2(
>  	struct xlog                     *log,
>  	struct xlog_recover_item        *item)
>  {
> -	struct xfs_inode_log_format	ilf_buf;
> -	struct xfs_inode_log_format	*ilfp;
> -	int			error;
> -
>  	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
> -		ilfp = item->ri_buf[0].i_addr;
> +		struct xfs_inode_log_format	*ilfp = item->ri_buf[0].i_addr;
> +
> +		xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
> +				   &xfs_inode_buf_ra_ops);
>  	} else {
> -		ilfp = &ilf_buf;
> -		memset(ilfp, 0, sizeof(*ilfp));
> -		error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp);
> -		if (error)
> -			return;
> -	}
> +		struct xfs_inode_log_format_32	*ilfp = item->ri_buf[0].i_addr;
>  
> -	xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
> -			   &xfs_inode_buf_ra_ops);
> +		xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len,
> +				   &xfs_inode_buf_ra_ops);
> +	}
>  }
>  
>  STATIC void
> -- 
> 2.26.1
>