diff mbox series

[04/28] xfs: refactor log recovery item dispatch for pass1 commit functions

Message ID 158864105772.182683.2888229701435255975.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: refactor log recovery | expand

Commit Message

Darrick J. Wong May 5, 2020, 1:10 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Move the pass1 commit code into the per-item source code files and use
the dispatch function to call them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_log_recover.h |    4 ++
 fs/xfs/xfs_buf_item_recover.c   |   27 ++++++++++
 fs/xfs/xfs_dquot_item_recover.c |   28 +++++++++++
 fs/xfs/xfs_log_recover.c        |  101 +++++----------------------------------
 4 files changed, 71 insertions(+), 89 deletions(-)

Comments

Chandan Babu R May 5, 2020, 4:40 a.m. UTC | #1 
On Tuesday 5 May 2020 6:40:57 AM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the pass1 commit code into the per-item source code files and use
> the dispatch function to call them.
>

Buf and Quotaoff items need to be processed during pass1's commit phase. This
is correctly done in this patch.

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

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_log_recover.h |    4 ++
>  fs/xfs/xfs_buf_item_recover.c   |   27 ++++++++++
>  fs/xfs/xfs_dquot_item_recover.c |   28 +++++++++++
>  fs/xfs/xfs_log_recover.c        |  101 +++++----------------------------------
>  4 files changed, 71 insertions(+), 89 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
> index ff80871138bb..384b70d58993 100644
> --- a/fs/xfs/libxfs/xfs_log_recover.h
> +++ b/fs/xfs/libxfs/xfs_log_recover.h
> @@ -34,6 +34,9 @@ struct xlog_recover_item_ops {
>  
>  	/* Start readahead for pass2, if provided. */
>  	void (*ra_pass2)(struct xlog *log, struct xlog_recover_item *item);
> +
> +	/* Do whatever work we need to do for pass1, if provided. */
> +	int (*commit_pass1)(struct xlog *log, struct xlog_recover_item *item);
>  };
>  
>  extern const struct xlog_recover_item_ops xlog_icreate_item_ops;
> @@ -97,5 +100,6 @@ struct xlog_recover {
>  
>  void xlog_buf_readahead(struct xlog *log, xfs_daddr_t blkno, uint len,
>  		const struct xfs_buf_ops *ops);
> +bool xlog_add_buffer_cancelled(struct xlog *log, xfs_daddr_t blkno, uint len);
>  
>  #endif	/* __XFS_LOG_RECOVER_H__ */
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index a1327196b690..802f2206516d 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -42,8 +42,35 @@ xlog_recover_buf_ra_pass2(
>  	xlog_buf_readahead(log, buf_f->blf_blkno, buf_f->blf_len, NULL);
>  }
>  
> +/*
> + * Build up the table of buf cancel records so that we don't replay cancelled
> + * data in the second pass.
> + */
> +static int
> +xlog_recover_buf_commit_pass1(
> +	struct xlog			*log,
> +	struct xlog_recover_item	*item)
> +{
> +	struct xfs_buf_log_format	*bf = item->ri_buf[0].i_addr;
> +
> +	if (!xfs_buf_log_check_iovec(&item->ri_buf[0])) {
> +		xfs_err(log->l_mp, "bad buffer log item size (%d)",
> +				item->ri_buf[0].i_len);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	if (!(bf->blf_flags & XFS_BLF_CANCEL))
> +		trace_xfs_log_recover_buf_not_cancel(log, bf);
> +	else if (xlog_add_buffer_cancelled(log, bf->blf_blkno, bf->blf_len))
> +		trace_xfs_log_recover_buf_cancel_add(log, bf);
> +	else
> +		trace_xfs_log_recover_buf_cancel_ref_inc(log, bf);
> +	return 0;
> +}
> +
>  const struct xlog_recover_item_ops xlog_buf_item_ops = {
>  	.item_type		= XFS_LI_BUF,
>  	.reorder		= xlog_recover_buf_reorder,
>  	.ra_pass2		= xlog_recover_buf_ra_pass2,
> +	.commit_pass1		= xlog_recover_buf_commit_pass1,
>  };
> diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c
> index 215274173b70..ebc44c1bc2b1 100644
> --- a/fs/xfs/xfs_dquot_item_recover.c
> +++ b/fs/xfs/xfs_dquot_item_recover.c
> @@ -58,6 +58,34 @@ const struct xlog_recover_item_ops xlog_dquot_item_ops = {
>  	.ra_pass2		= xlog_recover_dquot_ra_pass2,
>  };
>  
> +/*
> + * Recover QUOTAOFF records. We simply make a note of it in the xlog
> + * structure, so that we know not to do any dquot item or dquot buffer recovery,
> + * of that type.
> + */
> +STATIC int
> +xlog_recover_quotaoff_commit_pass1(
> +	struct xlog			*log,
> +	struct xlog_recover_item	*item)
> +{
> +	struct xfs_qoff_logformat	*qoff_f = item->ri_buf[0].i_addr;
> +	ASSERT(qoff_f);
> +
> +	/*
> +	 * The logitem format's flag tells us if this was user quotaoff,
> +	 * group/project quotaoff or both.
> +	 */
> +	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
> +		log->l_quotaoffs_flag |= XFS_DQ_USER;
> +	if (qoff_f->qf_flags & XFS_PQUOTA_ACCT)
> +		log->l_quotaoffs_flag |= XFS_DQ_PROJ;
> +	if (qoff_f->qf_flags & XFS_GQUOTA_ACCT)
> +		log->l_quotaoffs_flag |= XFS_DQ_GROUP;
> +
> +	return 0;
> +}
> +
>  const struct xlog_recover_item_ops xlog_quotaoff_item_ops = {
>  	.item_type		= XFS_LI_QUOTAOFF,
> +	.commit_pass1		= xlog_recover_quotaoff_commit_pass1,
>  };
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index ea566747d8e1..b3627ebf870e 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1953,7 +1953,7 @@ xlog_find_buffer_cancelled(
>  	return NULL;
>  }
>  
> -static bool
> +bool
>  xlog_add_buffer_cancelled(
>  	struct xlog		*log,
>  	xfs_daddr_t		blkno,
> @@ -2034,32 +2034,6 @@ xlog_buf_readahead(
>  		xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
>  }
>  
> -/*
> - * Build up the table of buf cancel records so that we don't replay cancelled
> - * data in the second pass.
> - */
> -static int
> -xlog_recover_buffer_pass1(
> -	struct xlog			*log,
> -	struct xlog_recover_item	*item)
> -{
> -	struct xfs_buf_log_format	*bf = item->ri_buf[0].i_addr;
> -
> -	if (!xfs_buf_log_check_iovec(&item->ri_buf[0])) {
> -		xfs_err(log->l_mp, "bad buffer log item size (%d)",
> -				item->ri_buf[0].i_len);
> -		return -EFSCORRUPTED;
> -	}
> -
> -	if (!(bf->blf_flags & XFS_BLF_CANCEL))
> -		trace_xfs_log_recover_buf_not_cancel(log, bf);
> -	else if (xlog_add_buffer_cancelled(log, bf->blf_blkno, bf->blf_len))
> -		trace_xfs_log_recover_buf_cancel_add(log, bf);
> -	else
> -		trace_xfs_log_recover_buf_cancel_ref_inc(log, bf);
> -	return 0;
> -}
> -
>  /*
>   * Perform recovery for a buffer full of inodes.  In these buffers, the only
>   * data which should be recovered is that which corresponds to the
> @@ -3197,33 +3171,6 @@ xlog_recover_inode_pass2(
>  	return error;
>  }
>  
> -/*
> - * Recover QUOTAOFF records. We simply make a note of it in the xlog
> - * structure, so that we know not to do any dquot item or dquot buffer recovery,
> - * of that type.
> - */
> -STATIC int
> -xlog_recover_quotaoff_pass1(
> -	struct xlog			*log,
> -	struct xlog_recover_item	*item)
> -{
> -	xfs_qoff_logformat_t	*qoff_f = item->ri_buf[0].i_addr;
> -	ASSERT(qoff_f);
> -
> -	/*
> -	 * The logitem format's flag tells us if this was user quotaoff,
> -	 * group/project quotaoff or both.
> -	 */
> -	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
> -		log->l_quotaoffs_flag |= XFS_DQ_USER;
> -	if (qoff_f->qf_flags & XFS_PQUOTA_ACCT)
> -		log->l_quotaoffs_flag |= XFS_DQ_PROJ;
> -	if (qoff_f->qf_flags & XFS_GQUOTA_ACCT)
> -		log->l_quotaoffs_flag |= XFS_DQ_GROUP;
> -
> -	return 0;
> -}
> -
>  /*
>   * Recover a dquot record
>   */
> @@ -3890,40 +3837,6 @@ xlog_recover_do_icreate_pass2(
>  				     length, be32_to_cpu(icl->icl_gen));
>  }
>  
> -STATIC int
> -xlog_recover_commit_pass1(
> -	struct xlog			*log,
> -	struct xlog_recover		*trans,
> -	struct xlog_recover_item	*item)
> -{
> -	trace_xfs_log_recover_item_recover(log, trans, item, XLOG_RECOVER_PASS1);
> -
> -	switch (ITEM_TYPE(item)) {
> -	case XFS_LI_BUF:
> -		return xlog_recover_buffer_pass1(log, item);
> -	case XFS_LI_QUOTAOFF:
> -		return xlog_recover_quotaoff_pass1(log, item);
> -	case XFS_LI_INODE:
> -	case XFS_LI_EFI:
> -	case XFS_LI_EFD:
> -	case XFS_LI_DQUOT:
> -	case XFS_LI_ICREATE:
> -	case XFS_LI_RUI:
> -	case XFS_LI_RUD:
> -	case XFS_LI_CUI:
> -	case XFS_LI_CUD:
> -	case XFS_LI_BUI:
> -	case XFS_LI_BUD:
> -		/* nothing to do in pass 1 */
> -		return 0;
> -	default:
> -		xfs_warn(log->l_mp, "%s: invalid item type (%d)",
> -			__func__, ITEM_TYPE(item));
> -		ASSERT(0);
> -		return -EFSCORRUPTED;
> -	}
> -}
> -
>  STATIC int
>  xlog_recover_commit_pass2(
>  	struct xlog			*log,
> @@ -4021,9 +3934,19 @@ xlog_recover_commit_trans(
>  		return error;
>  
>  	list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
> +		trace_xfs_log_recover_item_recover(log, trans, item, pass);
> +
> +		if (!item->ri_ops) {
> +			xfs_warn(log->l_mp, "%s: invalid item type (%d)",
> +				__func__, ITEM_TYPE(item));
> +			ASSERT(0);
> +			return -EFSCORRUPTED;
> +		}
> +
>  		switch (pass) {
>  		case XLOG_RECOVER_PASS1:
> -			error = xlog_recover_commit_pass1(log, trans, item);
> +			if (item->ri_ops->commit_pass1)
> +				error = item->ri_ops->commit_pass1(log, item);
>  			break;
>  		case XLOG_RECOVER_PASS2:
>  			if (item->ri_ops->ra_pass2)
> 
>
Christoph Hellwig May 6, 2020, 3:07 p.m. UTC | #2 
>  	list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
> +		trace_xfs_log_recover_item_recover(log, trans, item, pass);
> +
> +		if (!item->ri_ops) {
> +			xfs_warn(log->l_mp, "%s: invalid item type (%d)",
> +				__func__, ITEM_TYPE(item));
> +			ASSERT(0);
> +			return -EFSCORRUPTED;
> +		}

Given that we check for ri_ops during the reorder phase this can't
happen.  I think we should remove this check.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index ff80871138bb..384b70d58993 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -34,6 +34,9 @@  struct xlog_recover_item_ops {
 
 	/* Start readahead for pass2, if provided. */
 	void (*ra_pass2)(struct xlog *log, struct xlog_recover_item *item);
+
+	/* Do whatever work we need to do for pass1, if provided. */
+	int (*commit_pass1)(struct xlog *log, struct xlog_recover_item *item);
 };
 
 extern const struct xlog_recover_item_ops xlog_icreate_item_ops;
@@ -97,5 +100,6 @@  struct xlog_recover {
 
 void xlog_buf_readahead(struct xlog *log, xfs_daddr_t blkno, uint len,
 		const struct xfs_buf_ops *ops);
+bool xlog_add_buffer_cancelled(struct xlog *log, xfs_daddr_t blkno, uint len);
 
 #endif	/* __XFS_LOG_RECOVER_H__ */
diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index a1327196b690..802f2206516d 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -42,8 +42,35 @@  xlog_recover_buf_ra_pass2(
 	xlog_buf_readahead(log, buf_f->blf_blkno, buf_f->blf_len, NULL);
 }
 
+/*
+ * Build up the table of buf cancel records so that we don't replay cancelled
+ * data in the second pass.
+ */
+static int
+xlog_recover_buf_commit_pass1(
+	struct xlog			*log,
+	struct xlog_recover_item	*item)
+{
+	struct xfs_buf_log_format	*bf = item->ri_buf[0].i_addr;
+
+	if (!xfs_buf_log_check_iovec(&item->ri_buf[0])) {
+		xfs_err(log->l_mp, "bad buffer log item size (%d)",
+				item->ri_buf[0].i_len);
+		return -EFSCORRUPTED;
+	}
+
+	if (!(bf->blf_flags & XFS_BLF_CANCEL))
+		trace_xfs_log_recover_buf_not_cancel(log, bf);
+	else if (xlog_add_buffer_cancelled(log, bf->blf_blkno, bf->blf_len))
+		trace_xfs_log_recover_buf_cancel_add(log, bf);
+	else
+		trace_xfs_log_recover_buf_cancel_ref_inc(log, bf);
+	return 0;
+}
+
 const struct xlog_recover_item_ops xlog_buf_item_ops = {
 	.item_type		= XFS_LI_BUF,
 	.reorder		= xlog_recover_buf_reorder,
 	.ra_pass2		= xlog_recover_buf_ra_pass2,
+	.commit_pass1		= xlog_recover_buf_commit_pass1,
 };
diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c
index 215274173b70..ebc44c1bc2b1 100644
--- a/fs/xfs/xfs_dquot_item_recover.c
+++ b/fs/xfs/xfs_dquot_item_recover.c
@@ -58,6 +58,34 @@  const struct xlog_recover_item_ops xlog_dquot_item_ops = {
 	.ra_pass2		= xlog_recover_dquot_ra_pass2,
 };
 
+/*
+ * Recover QUOTAOFF records. We simply make a note of it in the xlog
+ * structure, so that we know not to do any dquot item or dquot buffer recovery,
+ * of that type.
+ */
+STATIC int
+xlog_recover_quotaoff_commit_pass1(
+	struct xlog			*log,
+	struct xlog_recover_item	*item)
+{
+	struct xfs_qoff_logformat	*qoff_f = item->ri_buf[0].i_addr;
+	ASSERT(qoff_f);
+
+	/*
+	 * The logitem format's flag tells us if this was user quotaoff,
+	 * group/project quotaoff or both.
+	 */
+	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
+		log->l_quotaoffs_flag |= XFS_DQ_USER;
+	if (qoff_f->qf_flags & XFS_PQUOTA_ACCT)
+		log->l_quotaoffs_flag |= XFS_DQ_PROJ;
+	if (qoff_f->qf_flags & XFS_GQUOTA_ACCT)
+		log->l_quotaoffs_flag |= XFS_DQ_GROUP;
+
+	return 0;
+}
+
 const struct xlog_recover_item_ops xlog_quotaoff_item_ops = {
 	.item_type		= XFS_LI_QUOTAOFF,
+	.commit_pass1		= xlog_recover_quotaoff_commit_pass1,
 };
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index ea566747d8e1..b3627ebf870e 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1953,7 +1953,7 @@  xlog_find_buffer_cancelled(
 	return NULL;
 }
 
-static bool
+bool
 xlog_add_buffer_cancelled(
 	struct xlog		*log,
 	xfs_daddr_t		blkno,
@@ -2034,32 +2034,6 @@  xlog_buf_readahead(
 		xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
 }
 
-/*
- * Build up the table of buf cancel records so that we don't replay cancelled
- * data in the second pass.
- */
-static int
-xlog_recover_buffer_pass1(
-	struct xlog			*log,
-	struct xlog_recover_item	*item)
-{
-	struct xfs_buf_log_format	*bf = item->ri_buf[0].i_addr;
-
-	if (!xfs_buf_log_check_iovec(&item->ri_buf[0])) {
-		xfs_err(log->l_mp, "bad buffer log item size (%d)",
-				item->ri_buf[0].i_len);
-		return -EFSCORRUPTED;
-	}
-
-	if (!(bf->blf_flags & XFS_BLF_CANCEL))
-		trace_xfs_log_recover_buf_not_cancel(log, bf);
-	else if (xlog_add_buffer_cancelled(log, bf->blf_blkno, bf->blf_len))
-		trace_xfs_log_recover_buf_cancel_add(log, bf);
-	else
-		trace_xfs_log_recover_buf_cancel_ref_inc(log, bf);
-	return 0;
-}
-
 /*
  * Perform recovery for a buffer full of inodes.  In these buffers, the only
  * data which should be recovered is that which corresponds to the
@@ -3197,33 +3171,6 @@  xlog_recover_inode_pass2(
 	return error;
 }
 
-/*
- * Recover QUOTAOFF records. We simply make a note of it in the xlog
- * structure, so that we know not to do any dquot item or dquot buffer recovery,
- * of that type.
- */
-STATIC int
-xlog_recover_quotaoff_pass1(
-	struct xlog			*log,
-	struct xlog_recover_item	*item)
-{
-	xfs_qoff_logformat_t	*qoff_f = item->ri_buf[0].i_addr;
-	ASSERT(qoff_f);
-
-	/*
-	 * The logitem format's flag tells us if this was user quotaoff,
-	 * group/project quotaoff or both.
-	 */
-	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
-		log->l_quotaoffs_flag |= XFS_DQ_USER;
-	if (qoff_f->qf_flags & XFS_PQUOTA_ACCT)
-		log->l_quotaoffs_flag |= XFS_DQ_PROJ;
-	if (qoff_f->qf_flags & XFS_GQUOTA_ACCT)
-		log->l_quotaoffs_flag |= XFS_DQ_GROUP;
-
-	return 0;
-}
-
 /*
  * Recover a dquot record
  */
@@ -3890,40 +3837,6 @@  xlog_recover_do_icreate_pass2(
 				     length, be32_to_cpu(icl->icl_gen));
 }
 
-STATIC int
-xlog_recover_commit_pass1(
-	struct xlog			*log,
-	struct xlog_recover		*trans,
-	struct xlog_recover_item	*item)
-{
-	trace_xfs_log_recover_item_recover(log, trans, item, XLOG_RECOVER_PASS1);
-
-	switch (ITEM_TYPE(item)) {
-	case XFS_LI_BUF:
-		return xlog_recover_buffer_pass1(log, item);
-	case XFS_LI_QUOTAOFF:
-		return xlog_recover_quotaoff_pass1(log, item);
-	case XFS_LI_INODE:
-	case XFS_LI_EFI:
-	case XFS_LI_EFD:
-	case XFS_LI_DQUOT:
-	case XFS_LI_ICREATE:
-	case XFS_LI_RUI:
-	case XFS_LI_RUD:
-	case XFS_LI_CUI:
-	case XFS_LI_CUD:
-	case XFS_LI_BUI:
-	case XFS_LI_BUD:
-		/* nothing to do in pass 1 */
-		return 0;
-	default:
-		xfs_warn(log->l_mp, "%s: invalid item type (%d)",
-			__func__, ITEM_TYPE(item));
-		ASSERT(0);
-		return -EFSCORRUPTED;
-	}
-}
-
 STATIC int
 xlog_recover_commit_pass2(
 	struct xlog			*log,
@@ -4021,9 +3934,19 @@  xlog_recover_commit_trans(
 		return error;
 
 	list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
+		trace_xfs_log_recover_item_recover(log, trans, item, pass);
+
+		if (!item->ri_ops) {
+			xfs_warn(log->l_mp, "%s: invalid item type (%d)",
+				__func__, ITEM_TYPE(item));
+			ASSERT(0);
+			return -EFSCORRUPTED;
+		}
+
 		switch (pass) {
 		case XLOG_RECOVER_PASS1:
-			error = xlog_recover_commit_pass1(log, trans, item);
+			if (item->ri_ops->commit_pass1)
+				error = item->ri_ops->commit_pass1(log, item);
 			break;
 		case XLOG_RECOVER_PASS2:
 			if (item->ri_ops->ra_pass2)