[05/24] xfs: use ->t_dfops for recovery of [b|c]ui log items
diff mbox

Message ID 20180628163636.52564-6-bfoster@redhat.com
State New
Headers show

Commit Message

Brian Foster June 28, 2018, 4:36 p.m. UTC
Log recovery passes down a central dfops structure to recovery
handlers for bui and cui log items. Each of these handlers allocates
and commits a transaction and defers any remaining operations to be
completed by the main recovery sequence.

Since dfops outlives the transaction in this context, set and clear
->t_dfops appropriately such that the *_finish_item() paths and
below (i.e., xfs_bmapi*()) can expect to find the dfops in the
transaction without it being committed with the dfops attached. This
is required because transaction commit expects that an associated
dfops is finished and in this context the dfops may be populated at
commit time.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_bmap_item.c     | 3 +++
 fs/xfs/xfs_refcount_item.c | 3 +++
 2 files changed, 6 insertions(+)

Comments

Christoph Hellwig July 2, 2018, 1:45 p.m. UTC | #1
On Thu, Jun 28, 2018 at 12:36:17PM -0400, Brian Foster wrote:
> Log recovery passes down a central dfops structure to recovery
> handlers for bui and cui log items. Each of these handlers allocates
> and commits a transaction and defers any remaining operations to be
> completed by the main recovery sequence.
> 
> Since dfops outlives the transaction in this context, set and clear
> ->t_dfops appropriately such that the *_finish_item() paths and
> below (i.e., xfs_bmapi*()) can expect to find the dfops in the
> transaction without it being committed with the dfops attached. This
> is required because transaction commit expects that an associated
> dfops is finished and in this context the dfops may be populated at
> commit time.

Do we want comments explaining this in the code?

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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
Brian Foster July 2, 2018, 5:33 p.m. UTC | #2
On Mon, Jul 02, 2018 at 06:45:50AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 28, 2018 at 12:36:17PM -0400, Brian Foster wrote:
> > Log recovery passes down a central dfops structure to recovery
> > handlers for bui and cui log items. Each of these handlers allocates
> > and commits a transaction and defers any remaining operations to be
> > completed by the main recovery sequence.
> > 
> > Since dfops outlives the transaction in this context, set and clear
> > ->t_dfops appropriately such that the *_finish_item() paths and
> > below (i.e., xfs_bmapi*()) can expect to find the dfops in the
> > transaction without it being committed with the dfops attached. This
> > is required because transaction commit expects that an associated
> > dfops is finished and in this context the dfops may be populated at
> > commit time.
> 
> Do we want comments explaining this in the code?
> 

Yeah, I think I meant to do that and just lost track. Thanks.

Brian

> Otherwise looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> --
> 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

Patch
diff mbox

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 956ebd583e27..0a1f3e5176f3 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -441,6 +441,7 @@  xfs_bui_recover(
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
 	if (error)
 		return error;
+	tp->t_dfops = dfops;
 	budp = xfs_trans_get_bud(tp, buip);
 
 	/* Grab the inode. */
@@ -487,6 +488,7 @@  xfs_bui_recover(
 	}
 
 	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
+	tp->t_dfops = NULL;
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	IRELE(ip);
@@ -494,6 +496,7 @@  xfs_bui_recover(
 	return error;
 
 err_inode:
+	tp->t_dfops = NULL;
 	xfs_trans_cancel(tp);
 	if (ip) {
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 472a73e9d331..eeb6ec5e0a64 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -452,6 +452,7 @@  xfs_cui_recover(
 			mp->m_refc_maxlevels * 2, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
+	tp->t_dfops = dfops;
 	cudp = xfs_trans_get_cud(tp, cuip);
 
 	for (i = 0; i < cuip->cui_format.cui_nextents; i++) {
@@ -514,11 +515,13 @@  xfs_cui_recover(
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
 	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
+	tp->t_dfops = NULL;
 	error = xfs_trans_commit(tp);
 	return error;
 
 abort_error:
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
+	tp->t_dfops = NULL;
 	xfs_trans_cancel(tp);
 	return error;
 }