xfs: add the ability to join a buffer to a defer_ops

RFC PATCH In review Darrick J. Wong 2 participants 5 messages Nov 30, 2017 – Dec 7, 2017 XFS filesystem

lore

Share & tools

lore

Darrick J. Wong

author

2017-11-30 17:58 UTC #0 lore raw

RFC PATCH 1/2 xfs: add the ability to join a buffer to a defer_ops

From: Darrick J. Wong <[email protected]>

In certain cases we need to be able to maintain a buffer lock across a defer_finish call. Since there could be many (large) transactions committed as a result of a defer_finish, we have to hold the buffer across the roll, then immediately rejoin the buffer and mark it dirty in each transaction to keep the log moving forward.

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/libxfs/xfs_defer.c |   37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h |    5 ++++-
 2 files changed, 38 insertions(+), 4 deletions(-)

fs/xfs/libxfs/xfs_defer.c +34 -3

index 072ebfe..b5b3414 100644
@@ -249,6 +249,10 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 
+	/* Hold the (previously bjoin'd) buffer locked across the roll. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
+		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+
 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
 
 	/* Roll the transaction. */

@@ -264,6 +268,12 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
+	/* Rejoin the buffers and dirty them so the log moves forward. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
+		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
+		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
+	}
+
 	return error;
 }

@@ -299,6 +309,29 @@ xfs_defer_ijoin(
 }
 
 /*
+ * Add this buffer to the deferred op.  Each joined buffer is relogged
+ * each time we roll the transaction.
+ */
+int
+xfs_defer_bjoin(
+	struct xfs_defer_ops		*dop,
+	struct xfs_buf			*bp)
+{
+	int				i;
+
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (dop->dop_bufs[i] == bp)
+			return 0;
+		else if (dop->dop_bufs[i] == NULL) {
+			dop->dop_bufs[i] = bp;
+			return 0;
+		}
+	}
+
+	return -EFSCORRUPTED;
+}
+
+/*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
  * one has even happened), rolling the transaction, and finishing the

@@ -493,9 +526,7 @@ xfs_defer_init(
 	struct xfs_defer_ops		*dop,
 	xfs_fsblock_t			*fbp)
 {
-	dop->dop_committed = false;
-	dop->dop_low = false;
-	memset(&dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop, 0, sizeof(struct xfs_defer_ops));
 	*fbp = NULLFSBLOCK;
 	INIT_LIST_HEAD(&dop->dop_intake);
 	INIT_LIST_HEAD(&dop->dop_pending);

fs/xfs/libxfs/xfs_defer.h +4 -1

index d4f046d..045beac 100644
@@ -59,6 +59,7 @@ enum xfs_defer_ops_type {
 };
 
 #define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
 	bool			dop_committed;	/* did any trans commit? */

@@ -66,8 +67,9 @@ struct xfs_defer_ops {
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
-	/* relog these inodes with each roll */
+	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

@@ -77,6 +79,7 @@ void xfs_defer_cancel(struct xfs_defer_ops *dop);
 void xfs_defer_init(struct xfs_defer_ops *dop, xfs_fsblock_t *fbp);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
+int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {

Brian Foster

2017-12-01 13:36 UTC #1 lore raw

Quoted 10 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
In certain cases we need to be able to maintain a buffer lock across a defer_finish call. Since there could be many (large) transactions committed as a result of a defer_finish, we have to hold the buffer across the roll, then immediately rejoin the buffer and mark it dirty in each transaction to keep the log moving forward.
Signed-off-by Darrick J. Wong <[email protected]>
---

Seems about right to me. A couple things..

Quoted 9 lines

 fs/xfs/libxfs/xfs_defer.c |   37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h |    5 ++++-
 2 files changed, 38 insertions(+), 4 deletions(-)

fs/xfs/libxfs/xfs_defer.c +0 -0

index 072ebfe..b5b3414 100644

fs/xfs/libxfs/xfs_defer.c xfs_defer_trans_roll( · line 255

Quoted 7 lines

fs/xfs/libxfs/xfs_defer.c +4 -0

@@ -249,6 +249,10 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 
+	/* Hold the (previously bjoin'd) buffer locked across the roll. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
+		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+

It seems more consistent to dirty the buffer in the tx here and bjoin+bhold it in the loop below.

Quoted 30 lines

fs/xfs/libxfs/xfs_defer.c +16 -0

 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
 
 	/* Roll the transaction. */

@@ -264,6 +268,12 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
+	/* Rejoin the buffers and dirty them so the log moves forward. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
+		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
+		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
+	}
+
 	return error;
 }

@@ -299,6 +309,29 @@ xfs_defer_ijoin(
 }
 
 /*
+ * Add this buffer to the deferred op.  Each joined buffer is relogged
+ * each time we roll the transaction.
+ */
+int
+xfs_defer_bjoin(
+	struct xfs_defer_ops		*dop,
+	struct xfs_buf			*bp)
+{
+	int				i;
+

fs/xfs/libxfs/xfs_defer.c xfs_defer_ijoin( · line 331

Quoted 10 lines

fs/xfs/libxfs/xfs_defer.c +10 -0

+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (dop->dop_bufs[i] == bp)
+			return 0;
+		else if (dop->dop_bufs[i] == NULL) {
+			dop->dop_bufs[i] = bp;
+			return 0;
+		}
+	}
+
+	return -EFSCORRUPTED;

I notice that this looks exactly like xfs_defer_join(), but is -EFSCORRUPTED the right error here? It probably doesn't matter that much given that if we hit this we've already lost, but I wonder if an error that more reflects a programming error as opposed to inconsistent fs might be more appropriate..? -EINVAL, -EBUSY?

Brian

Quoted 52 lines

fs/xfs/libxfs/xfs_defer.c +4 -3

+}
+
+/*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
  * one has even happened), rolling the transaction, and finishing the

@@ -493,9 +526,7 @@ xfs_defer_init(
 	struct xfs_defer_ops		*dop,
 	xfs_fsblock_t			*fbp)
 {
-	dop->dop_committed = false;
-	dop->dop_low = false;
-	memset(&dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop, 0, sizeof(struct xfs_defer_ops));
 	*fbp = NULLFSBLOCK;
 	INIT_LIST_HEAD(&dop->dop_intake);
 	INIT_LIST_HEAD(&dop->dop_pending);

fs/xfs/libxfs/xfs_defer.h +4 -2

index d4f046d..045beac 100644
@@ -59,6 +59,7 @@ enum xfs_defer_ops_type {
 };
 
 #define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
 	bool			dop_committed;	/* did any trans commit? */

@@ -66,8 +67,9 @@ struct xfs_defer_ops {
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
-	/* relog these inodes with each roll */
+	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

@@ -77,6 +79,7 @@ void xfs_defer_cancel(struct xfs_defer_ops *dop);
 void xfs_defer_init(struct xfs_defer_ops *dop, xfs_fsblock_t *fbp);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
+int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

author

2017-12-01 16:39 UTC #2 lore raw

Quoted 26 lines · Brian Foster

Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
In certain cases we need to be able to maintain a buffer lock across a defer_finish call. Since there could be many (large) transactions committed as a result of a defer_finish, we have to hold the buffer across the roll, then immediately rejoin the buffer and mark it dirty in each transaction to keep the log moving forward.
Signed-off-by Darrick J. Wong <[email protected]>
---
Seems about right to me. A couple things..
Quoted 12 lines
 fs/xfs/libxfs/xfs_defer.c |   37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h |    5 ++++-
 2 files changed, 38 insertions(+), 4 deletions(-)
fs/xfs/libxfs/xfs_defer.c +0 -0
index 072ebfe..b5b3414 100644
@@ -249,6 +249,10 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 

Quoted 7 lines

fs/xfs/libxfs/xfs_defer.c xfs_defer_trans_roll( · line 255
Quoted 4 lines
fs/xfs/libxfs/xfs_defer.c +4 -0
+	/* Hold the (previously bjoin'd) buffer locked across the roll. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
+		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+
It seems more consistent to dirty the buffer in the tx here and bjoin+bhold it in the loop below.

I thought the purpose of calling bhold was to prevent the transaction commit (in xfs_trans_roll) from unlocking the buffer? Therefore you'd bhold it before the _roll and then bjoin/dirty the still-locked buffer afterwards to attach the buffer as a dirty buffer to the new transaction.

Quoted 38 lines

fs/xfs/libxfs/xfs_defer.c +24 -0

 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
 
 	/* Roll the transaction. */

@@ -264,6 +268,12 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
+	/* Rejoin the buffers and dirty them so the log moves forward. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
+		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
+		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
+	}
+
 	return error;
 }

@@ -299,6 +309,29 @@ xfs_defer_ijoin(
 }
 
 /*
+ * Add this buffer to the deferred op.  Each joined buffer is relogged
+ * each time we roll the transaction.
+ */
+int
+xfs_defer_bjoin(
+	struct xfs_defer_ops		*dop,
+	struct xfs_buf			*bp)
+{
+	int				i;
+
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (dop->dop_bufs[i] == bp)
+			return 0;
+		else if (dop->dop_bufs[i] == NULL) {
+			dop->dop_bufs[i] = bp;
+			return 0;
+		}
+	}

Quoted 8 lines

fs/xfs/libxfs/xfs_defer.c xfs_defer_ijoin( · line 331
Quoted 2 lines
fs/xfs/libxfs/xfs_defer.c +2 -0
+
+	return -EFSCORRUPTED;
I notice that this looks exactly like xfs_defer_join(), but is -EFSCORRUPTED the right error here? It probably doesn't matter that much given that if we hit this we've already lost, but I wonder if an error that more reflects a programming error as opposed to inconsistent fs might be more appropriate..? -EINVAL, -EBUSY?

Yeah, I'm not sure what error code applies to "programmer messed up" :)

Perhaps we should add an ASSERT(0) at the bottom of both functions.

--D

Quoted 58 lines

Brian

Quoted 49 lines

fs/xfs/libxfs/xfs_defer.c +4 -3

+}
+
+/*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
  * one has even happened), rolling the transaction, and finishing the

@@ -493,9 +526,7 @@ xfs_defer_init(
 	struct xfs_defer_ops		*dop,
 	xfs_fsblock_t			*fbp)
 {
-	dop->dop_committed = false;
-	dop->dop_low = false;
-	memset(&dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop, 0, sizeof(struct xfs_defer_ops));
 	*fbp = NULLFSBLOCK;
 	INIT_LIST_HEAD(&dop->dop_intake);
 	INIT_LIST_HEAD(&dop->dop_pending);

fs/xfs/libxfs/xfs_defer.h +4 -2

index d4f046d..045beac 100644
@@ -59,6 +59,7 @@ enum xfs_defer_ops_type {
 };
 
 #define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
 	bool			dop_committed;	/* did any trans commit? */

@@ -66,8 +67,9 @@ struct xfs_defer_ops {
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
-	/* relog these inodes with each roll */
+	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

@@ -77,6 +79,7 @@ void xfs_defer_cancel(struct xfs_defer_ops *dop);
 void xfs_defer_init(struct xfs_defer_ops *dop, xfs_fsblock_t *fbp);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
+int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Brian Foster

2017-12-01 17:37 UTC #3 lore raw

Quoted 35 lines · Darrick J. Wong

Quoted 26 lines · Brian Foster
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
In certain cases we need to be able to maintain a buffer lock across a defer_finish call. Since there could be many (large) transactions committed as a result of a defer_finish, we have to hold the buffer across the roll, then immediately rejoin the buffer and mark it dirty in each transaction to keep the log moving forward.
Signed-off-by Darrick J. Wong <[email protected]>
---
Seems about right to me. A couple things..
Quoted 12 lines
 fs/xfs/libxfs/xfs_defer.c |   37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h |    5 ++++-
 2 files changed, 38 insertions(+), 4 deletions(-)
fs/xfs/libxfs/xfs_defer.c +0 -0
index 072ebfe..b5b3414 100644
@@ -249,6 +249,10 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 
Quoted 7 lines
fs/xfs/libxfs/xfs_defer.c xfs_defer_trans_roll( · line 255
Quoted 4 lines
fs/xfs/libxfs/xfs_defer.c +4 -0
+	/* Hold the (previously bjoin'd) buffer locked across the roll. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
+		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+
It seems more consistent to dirty the buffer in the tx here and bjoin+bhold it in the loop below.

Quoted 6 lines

I thought the purpose of calling bhold was to prevent the transaction commit (in xfs_trans_roll) from unlocking the buffer? Therefore you'd bhold it before the _roll and then bjoin/dirty the still-locked buffer afterwards to attach the buffer as a dirty buffer to the new transaction.

Yeah, but that wouldn't necessarily change.. I guess what I overlooked before is that xfs_defer_bjoin() doesn't actually hold the buffer in the current tp, so we'd have to start off with call to do that in the current transaction. All in all, what throws me off a bit is that I'd expect the same semantics/behavior for buffers in this situation as we have for inodes...

xfs_attr_set() joins the inode the transaction without transferring the lock (which is analogous to _bjoin() + _bhold()). It does some real work, defer_ijoin()'s the inode and finishes deferred ops. xfs_defer_finish() ultimately returns with a transaction that holds the inode with a clean log item descriptor.

The analogous behavior for buffers in my mind is for xfs_attr_set() to bhold the buffer to the current transaction, defer_bjoin() it and ultimately return from xfs_defer_finish() with the buffer held, but not yet dirtied, in the current transaction. Hm?

Brian

Quoted 111 lines

Quoted 38 lines

fs/xfs/libxfs/xfs_defer.c +24 -0

 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
 
 	/* Roll the transaction. */

@@ -264,6 +268,12 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
+	/* Rejoin the buffers and dirty them so the log moves forward. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
+		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
+		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
+	}
+
 	return error;
 }

@@ -299,6 +309,29 @@ xfs_defer_ijoin(
 }
 
 /*
+ * Add this buffer to the deferred op.  Each joined buffer is relogged
+ * each time we roll the transaction.
+ */
+int
+xfs_defer_bjoin(
+	struct xfs_defer_ops		*dop,
+	struct xfs_buf			*bp)
+{
+	int				i;
+
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (dop->dop_bufs[i] == bp)
+			return 0;
+		else if (dop->dop_bufs[i] == NULL) {
+			dop->dop_bufs[i] = bp;
+			return 0;
+		}
+	}

Quoted 8 lines

fs/xfs/libxfs/xfs_defer.c xfs_defer_ijoin( · line 331
Quoted 2 lines
fs/xfs/libxfs/xfs_defer.c +2 -0
+
+	return -EFSCORRUPTED;
I notice that this looks exactly like xfs_defer_join(), but is -EFSCORRUPTED the right error here? It probably doesn't matter that much given that if we hit this we've already lost, but I wonder if an error that more reflects a programming error as opposed to inconsistent fs might be more appropriate..? -EINVAL, -EBUSY?

Yeah, I'm not sure what error code applies to "programmer messed up" :)

Perhaps we should add an ASSERT(0) at the bottom of both functions.

--D

Quoted 58 lines

Brian

Quoted 49 lines

fs/xfs/libxfs/xfs_defer.c +4 -3

+}
+
+/*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
  * one has even happened), rolling the transaction, and finishing the

@@ -493,9 +526,7 @@ xfs_defer_init(
 	struct xfs_defer_ops		*dop,
 	xfs_fsblock_t			*fbp)
 {
-	dop->dop_committed = false;
-	dop->dop_low = false;
-	memset(&dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop, 0, sizeof(struct xfs_defer_ops));
 	*fbp = NULLFSBLOCK;
 	INIT_LIST_HEAD(&dop->dop_intake);
 	INIT_LIST_HEAD(&dop->dop_pending);

fs/xfs/libxfs/xfs_defer.h +4 -2

index d4f046d..045beac 100644
@@ -59,6 +59,7 @@ enum xfs_defer_ops_type {
 };
 
 #define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
 	bool			dop_committed;	/* did any trans commit? */

@@ -66,8 +67,9 @@ struct xfs_defer_ops {
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
-	/* relog these inodes with each roll */
+	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

@@ -77,6 +79,7 @@ void xfs_defer_cancel(struct xfs_defer_ops *dop);
 void xfs_defer_init(struct xfs_defer_ops *dop, xfs_fsblock_t *fbp);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
+int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

author

2017-12-07 00:35 UTC #4 lore raw

Quoted 56 lines · Brian Foster

Quoted 35 lines · Darrick J. Wong
Quoted 26 lines · Brian Foster
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
In certain cases we need to be able to maintain a buffer lock across a defer_finish call. Since there could be many (large) transactions committed as a result of a defer_finish, we have to hold the buffer across the roll, then immediately rejoin the buffer and mark it dirty in each transaction to keep the log moving forward.
Signed-off-by Darrick J. Wong <[email protected]>
---
Seems about right to me. A couple things..
Quoted 12 lines
 fs/xfs/libxfs/xfs_defer.c |   37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_defer.h |    5 ++++-
 2 files changed, 38 insertions(+), 4 deletions(-)
fs/xfs/libxfs/xfs_defer.c +0 -0
index 072ebfe..b5b3414 100644
@@ -249,6 +249,10 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
 
Quoted 7 lines
fs/xfs/libxfs/xfs_defer.c xfs_defer_trans_roll( · line 255
Quoted 4 lines
fs/xfs/libxfs/xfs_defer.c +4 -0
+	/* Hold the (previously bjoin'd) buffer locked across the roll. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
+		xfs_trans_bhold(*tp, dop->dop_bufs[i]);
+
It seems more consistent to dirty the buffer in the tx here and bjoin+bhold it in the loop below.
Quoted 6 lines
I thought the purpose of calling bhold was to prevent the transaction commit (in xfs_trans_roll) from unlocking the buffer? Therefore you'd bhold it before the _roll and then bjoin/dirty the still-locked buffer afterwards to attach the buffer as a dirty buffer to the new transaction.
Yeah, but that wouldn't necessarily change.. I guess what I overlooked before is that xfs_defer_bjoin() doesn't actually hold the buffer in the current tp, so we'd have to start off with call to do that in the current transaction. All in all, what throws me off a bit is that I'd expect the same semantics/behavior for buffers in this situation as we have for inodes...
xfs_attr_set() joins the inode the transaction without transferring the lock (which is analogous to _bjoin() + _bhold()). It does some real work, defer_ijoin()'s the inode and finishes deferred ops. xfs_defer_finish() ultimately returns with a transaction that holds the inode with a clean log item descriptor.

Quoted 4 lines

The analogous behavior for buffers in my mind is for xfs_attr_set() to bhold the buffer to the current transaction, defer_bjoin() it and ultimately return from xfs_defer_finish() with the buffer held, but not yet dirtied, in the current transaction. Hm?

Ahhh, ok. In my head I had designed this as "I have this locked buffer, now do whatever you have to do to ensure that it doesn't get unlocked in defer_finish." whereas the strategy for inodes is "I have this locked inode that won't unlock until I tell it to, so do whatever you must to ensure that it gets relogged in defer_finish and is still locked-and-wont-unlock."

I can do the same with buffers -- "I have this locked buffer that won't unlock until I tell it to, so do what you have to do to ensure it gets relogged and is still locked-and-wont-unlock after defer_finish."

--D

Quoted 117 lines

Brian

Quoted 47 lines

Quoted 38 lines

fs/xfs/libxfs/xfs_defer.c +24 -0

 	trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
 
 	/* Roll the transaction. */

@@ -264,6 +268,12 @@ xfs_defer_trans_roll(
 	for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
 		xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
 
+	/* Rejoin the buffers and dirty them so the log moves forward. */
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
+		xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
+		xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
+	}
+
 	return error;
 }

@@ -299,6 +309,29 @@ xfs_defer_ijoin(
 }
 
 /*
+ * Add this buffer to the deferred op.  Each joined buffer is relogged
+ * each time we roll the transaction.
+ */
+int
+xfs_defer_bjoin(
+	struct xfs_defer_ops		*dop,
+	struct xfs_buf			*bp)
+{
+	int				i;
+
+	for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
+		if (dop->dop_bufs[i] == bp)
+			return 0;
+		else if (dop->dop_bufs[i] == NULL) {
+			dop->dop_bufs[i] = bp;
+			return 0;
+		}
+	}

Quoted 8 lines

fs/xfs/libxfs/xfs_defer.c xfs_defer_ijoin( · line 331
Quoted 2 lines
fs/xfs/libxfs/xfs_defer.c +2 -0
+
+	return -EFSCORRUPTED;
I notice that this looks exactly like xfs_defer_join(), but is -EFSCORRUPTED the right error here? It probably doesn't matter that much given that if we hit this we've already lost, but I wonder if an error that more reflects a programming error as opposed to inconsistent fs might be more appropriate..? -EINVAL, -EBUSY?

Quoted 64 lines

Yeah, I'm not sure what error code applies to "programmer messed up" :)

Perhaps we should add an ASSERT(0) at the bottom of both functions.

--D

Quoted 58 lines

Brian

Quoted 49 lines

fs/xfs/libxfs/xfs_defer.c +4 -3

+}
+
+/*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
  * one has even happened), rolling the transaction, and finishing the

@@ -493,9 +526,7 @@ xfs_defer_init(
 	struct xfs_defer_ops		*dop,
 	xfs_fsblock_t			*fbp)
 {
-	dop->dop_committed = false;
-	dop->dop_low = false;
-	memset(&dop->dop_inodes, 0, sizeof(dop->dop_inodes));
+	memset(dop, 0, sizeof(struct xfs_defer_ops));
 	*fbp = NULLFSBLOCK;
 	INIT_LIST_HEAD(&dop->dop_intake);
 	INIT_LIST_HEAD(&dop->dop_pending);

fs/xfs/libxfs/xfs_defer.h +4 -2

index d4f046d..045beac 100644
@@ -59,6 +59,7 @@ enum xfs_defer_ops_type {
 };
 
 #define XFS_DEFER_OPS_NR_INODES	2	/* join up to two inodes */
+#define XFS_DEFER_OPS_NR_BUFS	2	/* join up to two buffers */
 
 struct xfs_defer_ops {
 	bool			dop_committed;	/* did any trans commit? */

@@ -66,8 +67,9 @@ struct xfs_defer_ops {
 	struct list_head	dop_intake;	/* unlogged pending work */
 	struct list_head	dop_pending;	/* logged pending work */
 
-	/* relog these inodes with each roll */
+	/* relog these with each roll */
 	struct xfs_inode	*dop_inodes[XFS_DEFER_OPS_NR_INODES];
+	struct xfs_buf		*dop_bufs[XFS_DEFER_OPS_NR_BUFS];
 };
 
 void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,

@@ -77,6 +79,7 @@ void xfs_defer_cancel(struct xfs_defer_ops *dop);
 void xfs_defer_init(struct xfs_defer_ops *dop, xfs_fsblock_t *fbp);
 bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
 int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
+int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html