Re: [PATCH 26/28] xfs: move log recovery buffer cancellation code to xfs_buf_item_recover.c

Chandan Babu R
2020-05-06T06:43:00Z

On Tuesday 5 May 2020 6:43:18 AM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Move the helpers that handle incore buffer cancellation records to
> xfs_buf_item_recover.c since they're not directly related to the main
> log recovery machinery. No functional changes.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_buf_item_recover.c | 104 +++++++++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_log_recover.c | 102 ----------------------------------------
> 2 files changed, 104 insertions(+), 102 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 4ca6d47d6c95..99ec6ebbc7f4 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -23,6 +23,110 @@
> #include "xfs_dir2.h"
> #include "xfs_quota.h"
>
> +/*
> + * This structure is used during recovery to record the buf log items which
> + * have been canceled and should not be replayed.
> + */
> +struct xfs_buf_cancel {
> + xfs_daddr_t bc_blkno;
> + uint bc_len;
> + int bc_refcount;
> + struct list_head bc_list;
> +};
> +
> +static struct xfs_buf_cancel *
> +xlog_find_buffer_cancelled(
> + struct xlog *log,
> + xfs_daddr_t blkno,
> + uint len)
> +{
> + struct list_head *bucket;
> + struct xfs_buf_cancel *bcp;
> +
> + if (!log->l_buf_cancel_table)
> + return NULL;
> +
> + bucket = XLOG_BUF_CANCEL_BUCKET(log, blkno);
> + list_for_each_entry(bcp, bucket, bc_list) {
> + if (bcp->bc_blkno == blkno && bcp->bc_len == len)
> + return bcp;
> + }
> +
> + return NULL;
> +}
> +
> +bool
> +xlog_add_buffer_cancelled(
> + struct xlog *log,
> + xfs_daddr_t blkno,
> + uint len)

The users of xlog_add_buffer_cancelled() are within xfs_buf_item_recover.c and
hence this can be made static and the corresponding prototype declaration in
xfs_log_recover.h can be removed.

Other than that trivial issue everything else looks good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> +{
> + struct xfs_buf_cancel *bcp;
> +
> + /*
> + * If we find an existing cancel record, this indicates that the buffer
> + * was cancelled multiple times. To ensure that during pass 2 we keep
> + * the record in the table until we reach its last occurrence in the
> + * log, a reference count is kept to tell how many times we expect to
> + * see this record during the second pass.
> + */
> + bcp = xlog_find_buffer_cancelled(log, blkno, len);
> + if (bcp) {
> + bcp->bc_refcount++;
> + return false;
> + }
> +
> + bcp = kmem_alloc(sizeof(struct xfs_buf_cancel), 0);
> + bcp->bc_blkno = blkno;
> + bcp->bc_len = len;
> + bcp->bc_refcount = 1;
> + list_add_tail(&bcp->bc_list, XLOG_BUF_CANCEL_BUCKET(log, blkno));
> + return true;
> +}
> +
> +/*
> + * Check if there is and entry for blkno, len in the buffer cancel record table.
> + */
> +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.
> + *
> + * 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.
> + */
> +bool
> +xlog_put_buffer_cancelled(
> + struct xlog *log,
> + xfs_daddr_t blkno,
> + uint len)
> +{
> + struct xfs_buf_cancel *bcp;
> +
> + bcp = xlog_find_buffer_cancelled(log, blkno, len);
> + if (!bcp) {
> + ASSERT(0);
> + return false;
> + }
> +
> + if (--bcp->bc_refcount == 0) {
> + list_del(&bcp->bc_list);
> + kmem_free(bcp);
> + }
> + return true;
> +}
> +
> +/* log buffer item recovery */
> +
> STATIC enum xlog_recover_reorder
> xlog_recover_buf_reorder(
> struct xlog_recover_item *item)
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index a49435db3be0..0c8a1f4bf4ad 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -55,17 +55,6 @@ STATIC int
> xlog_do_recovery_pass(
> struct xlog *, xfs_daddr_t, xfs_daddr_t, int, xfs_daddr_t *);
>
> -/*
> - * This structure is used during recovery to record the buf log items which
> - * have been canceled and should not be replayed.
> - */
> -struct xfs_buf_cancel {
> - xfs_daddr_t bc_blkno;
> - uint bc_len;
> - int bc_refcount;
> - struct list_head bc_list;
> -};
> -
> /*
> * Sector aligned buffer routines for buffer create/read/write/access
> */
> @@ -1981,97 +1970,6 @@ xlog_recover_reorder_trans(
> return error;
> }
>
> -static struct xfs_buf_cancel *
> -xlog_find_buffer_cancelled(
> - struct xlog *log,
> - xfs_daddr_t blkno,
> - uint len)
> -{
> - struct list_head *bucket;
> - struct xfs_buf_cancel *bcp;
> -
> - if (!log->l_buf_cancel_table)
> - return NULL;
> -
> - bucket = XLOG_BUF_CANCEL_BUCKET(log, blkno);
> - list_for_each_entry(bcp, bucket, bc_list) {
> - if (bcp->bc_blkno == blkno && bcp->bc_len == len)
> - return bcp;
> - }
> -
> - return NULL;
> -}
> -
> -bool
> -xlog_add_buffer_cancelled(
> - struct xlog *log,
> - xfs_daddr_t blkno,
> - uint len)
> -{
> - struct xfs_buf_cancel *bcp;
> -
> - /*
> - * If we find an existing cancel record, this indicates that the buffer
> - * was cancelled multiple times. To ensure that during pass 2 we keep
> - * the record in the table until we reach its last occurrence in the
> - * log, a reference count is kept to tell how many times we expect to
> - * see this record during the second pass.
> - */
> - bcp = xlog_find_buffer_cancelled(log, blkno, len);
> - if (bcp) {
> - bcp->bc_refcount++;
> - return false;
> - }
> -
> - bcp = kmem_alloc(sizeof(struct xfs_buf_cancel), 0);
> - bcp->bc_blkno = blkno;
> - bcp->bc_len = len;
> - bcp->bc_refcount = 1;
> - list_add_tail(&bcp->bc_list, XLOG_BUF_CANCEL_BUCKET(log, blkno));
> - return true;
> -}
> -
> -/*
> - * Check if there is and entry for blkno, len in the buffer cancel record table.
> - */
> -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.
> - *
> - * 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.
> - */
> -bool
> -xlog_put_buffer_cancelled(
> - struct xlog *log,
> - xfs_daddr_t blkno,
> - uint len)
> -{
> - struct xfs_buf_cancel *bcp;
> -
> - bcp = xlog_find_buffer_cancelled(log, blkno, len);
> - if (!bcp) {
> - ASSERT(0);
> - return false;
> - }
> -
> - if (--bcp->bc_refcount == 0) {
> - list_del(&bcp->bc_list);
> - kmem_free(bcp);
> - }
> - return true;
> -}
> -
> void
> xlog_buf_readahead(
> struct xlog *log,
>
>

--
chandan