[PATCH 0/2] xfs: fix transaction joining problems.

[PATCH 0/2] xfs: fix transaction joining problems.

[Dave Chinner]

Hi folks,

I found these when tracking down a hang in some new code I wrote
this afternoon. Joining a log item mulitple times to the same
transaction violates the relationship assumptions between
transactions and log items. It broke the code I wrote this afternoon
so subtly it took me several hours to track this down....

I don't know of any issues that can be attributed to these bugs, but
it's incorrect behaviour that needs fixing.

Cheers,

Dave.

[PATCH 1/2] xfs: fix double ijoin in xfs_inactive_symlink_rmt()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

xfs_inactive_symlink_rmt() does something nasty - it joins an inode
into a transaction it is already joined to. This means the inode can
have multiple log item descriptors attached to the transaction for
it. This breaks teh 1:1 mapping that is supposed to exist
between the log item and log item descriptor.

This results in the log item being processed twice during
transaction commit and CIL formatting, and there are lots of other
potential issues tha arise from double processing of log items in
the transaction commit state machine.

In this case, the inode is already held by the rolling transaction
returned from xfs_defer_finish(), so there's no need to join it
again.

Also add a log item flag to say the item has been joined to a
transaction and assert that it's not set when adding a log item to a
transaction. Clear it when the log item is disconnected from the log
item descriptor. This will tell us if there are other log items that
transactions are referencing multiple times.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_symlink.c | 9 ++-------
 fs/xfs/xfs_trans.c   | 7 +++++--
 fs/xfs/xfs_trans.h   | 4 +++-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 2e9e793a8f9d..b0502027070e 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
 	error = xfs_defer_finish(&tp, &dfops);
 	if (error)
 		goto error_bmap_cancel;
-	/*
-	 * The first xact was committed, so add the inode to the new one.
-	 * Mark it dirty so it will be logged and moved forward in the log as
-	 * part of every commit.
-	 */
-	xfs_trans_ijoin(tp, ip, 0);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 	/*
 	 * Commit the transaction containing extent freeing and EFDs.
 	 */
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	error = xfs_trans_commit(tp);
 	if (error) {
 		ASSERT(XFS_FORCED_SHUTDOWN(mp));
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 86f92df32c42..2aeeb2ad276a 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -737,12 +737,14 @@ xfs_trans_add_item(
 
 	ASSERT(lip->li_mountp == tp->t_mountp);
 	ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
+	ASSERT(!(lip->li_flags & XFS_LI_TRANS));
 
 	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
 
 	lidp->lid_item = lip;
 	lidp->lid_flags = 0;
 	list_add_tail(&lidp->lid_trans, &tp->t_items);
+	lip->li_flags |= XFS_LI_TRANS;
 
 	lip->li_desc = lidp;
 }
@@ -762,6 +764,7 @@ void
 xfs_trans_del_item(
 	struct xfs_log_item	*lip)
 {
+	lip->li_flags &= ~XFS_LI_TRANS;
 	xfs_trans_free_item_desc(lip->li_desc);
 	lip->li_desc = NULL;
 }
@@ -781,15 +784,15 @@ xfs_trans_free_items(
 	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
 		struct xfs_log_item	*lip = lidp->lid_item;
 
-		lip->li_desc = NULL;
+		xfs_trans_del_item(lip);
 
 		if (commit_lsn != NULLCOMMITLSN)
 			lip->li_ops->iop_committing(lip, commit_lsn);
 		if (abort)
 			lip->li_flags |= XFS_LI_ABORTED;
+
 		lip->li_ops->iop_unlock(lip);
 
-		xfs_trans_free_item_desc(lidp);
 	}
 }
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 9d542dfe0052..c514486ba2a0 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -67,11 +67,13 @@ typedef struct xfs_log_item {
 #define	XFS_LI_IN_AIL	0x1
 #define	XFS_LI_ABORTED	0x2
 #define	XFS_LI_FAILED	0x4
+#define	XFS_LI_TRANS	0x8	/* attached to an active transaction */
 
 #define XFS_LI_FLAGS \
 	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
 	{ XFS_LI_ABORTED,	"ABORTED" }, \
-	{ XFS_LI_FAILED,	"FAILED" }
+	{ XFS_LI_FAILED,	"FAILED" }, \
+	{ XFS_LI_TRANS,		"TRANS" }
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);
-- 
2.16.1

[PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

AN inode is joined to teh same transaction twice in
xfs_reflink_cancel_cow_range() resulting in the following assert
failure:

[   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
[   30.183435] ------------[ cut here ]------------
......
[   30.209264] Call Trace:
[   30.209935]  xfs_trans_add_item+0xcc/0xe0
[   30.210968]  xfs_reflink_cancel_cow_blocks+0xab/0x290
[   30.212249]  ? xfs_trans_reserve+0x1b4/0x2b0
[   30.213320]  ? kmem_zone_alloc+0x61/0xe0
[   30.214321]  xfs_reflink_cancel_cow_range+0xb2/0x1f0
[   30.215616]  xfs_fs_destroy_inode+0x1bd/0x280
[   30.216757]  dispose_list+0x35/0x40
[   30.217656]  evict_inodes+0x132/0x160
[   30.218620]  generic_shutdown_super+0x3a/0x110
[   30.219771]  kill_block_super+0x21/0x50
[   30.220762]  deactivate_locked_super+0x39/0x70
[   30.221909]  cleanup_mnt+0x3b/0x70
[   30.222819]  task_work_run+0x7f/0xa0
[   30.223762]  exit_to_usermode_loop+0x9b/0xa0
[   30.224884]  do_syscall_64+0x18f/0x1a0

Fix it and document that the callers of
xfs_reflink_cancel_cow_blocks() must have already joined the inode
to the permanent transaction passed in.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 8c16177b33d4..6225d1ea3fdb 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
  *
  * If cancel_real is true this function cancels all COW fork extents for the
  * inode; if cancel_real is false, real extents are not cleared.
+ *
+ * Caller must have already joined the inode to the current transaction. The
+ * inode will be joined to the transaction returned to the caller.
  */
 int
 xfs_reflink_cancel_cow_blocks(
@@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
 			if (error)
 				break;
 		} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
-			xfs_trans_ijoin(*tpp, ip, 0);
 			xfs_defer_init(&dfops, &firstfsb);
 
 			/* Free the CoW orphan record. */
@@ -1571,6 +1573,7 @@ xfs_reflink_clear_inode_flag(
 	 * We didn't find any shared blocks so turn off the reflink flag.
 	 * First, get rid of any leftover CoW mappings.
 	 */
+	xfs_trans_ijoin(*tpp, ip, 0);
 	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
 	if (error)
 		return error;
@@ -1579,7 +1582,6 @@ xfs_reflink_clear_inode_flag(
 	trace_xfs_reflink_unset_inode_flag(ip);
 	ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
 	xfs_inode_clear_cowblocks_tag(ip);
-	xfs_trans_ijoin(*tpp, ip, 0);
 	xfs_trans_log_inode(*tpp, ip, XFS_ILOG_CORE);
 
 	return error;
-- 
2.16.1

[PATCH 3/2] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Another assert failure:

XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
....
xfs_trans_add_item+0xcc/0xe0
xfs_reflink_clear_inode_flag+0x53/0x120
xfs_reflink_try_clear_inode_flag+0x5b/0xa0
? filemap_write_and_wait+0x4f/0x70
xfs_reflink_unshare+0x18e/0x19d
xfs_file_fallocate+0x241/0x310
? selinux_file_permission+0xd4/0x140
vfs_fallocate+0x13d/0x260
SyS_fallocate+0x43/0x80

Another fix.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 6225d1ea3fdb..5cf60a042e8f 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1554,7 +1554,12 @@ xfs_reflink_inode_has_shared_extents(
 	return 0;
 }
 
-/* Clear the inode reflink flag if there are no shared extents. */
+/*
+ * Clear the inode reflink flag if there are no shared extents.
+ *
+ * The caller is responsible for joining the inode to the transaction passed in.
+ * The inode will be joined to the transaction that is returned to the caller.
+ */
 int
 xfs_reflink_clear_inode_flag(
 	struct xfs_inode	*ip,
@@ -1573,7 +1578,6 @@ xfs_reflink_clear_inode_flag(
 	 * We didn't find any shared blocks so turn off the reflink flag.
 	 * First, get rid of any leftover CoW mappings.
 	 */
-	xfs_trans_ijoin(*tpp, ip, 0);
 	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
 	if (error)
 		return error;

Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Christoph Hellwig]

On Wed, Mar 07, 2018 at 08:10:20PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> AN inode is joined to teh same transaction twice in
> xfs_reflink_cancel_cow_range() resulting in the following assert
> failure:

Needs some major spelling love :)

> 
> [   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740

That assertations seems like something that only exists locally in your
tree.  Any chance to send it out with this series?

The other option would be to make xfs_trans_ijoin a no-op if the inode
is already joined, except that this wouldn't work with the magic unlock
on commit.  Which is a feature I find horribly confusing, so we should
get rid of it, for which we'd need to get rid of the concept of
synchronous transactions in favour of leaving the log force to the
caller, which again would be more logical.

Guess I need to look into doing these cleanups as I don't want to force
them on anyone else.  Just need to finish all the other more important
bits on the todo list first :)

Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Dave Chinner]

On Wed, Mar 07, 2018 at 05:17:25AM -0800, Christoph Hellwig wrote:
> On Wed, Mar 07, 2018 at 08:10:20PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > AN inode is joined to teh same transaction twice in
> > xfs_reflink_cancel_cow_range() resulting in the following assert
> > failure:
> 
> Needs some major spelling love :)

Yeah, I sent it out as soon as I realised it was more than just an
isolated occurrence. Needs updating...

> > [   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> 
> That assertations seems like something that only exists locally in your
> tree.  Any chance to send it out with this series?

It's in the first patch. I've got to revise it, anyway, because I
missed the xfs_trans_brelease() case that removes the log item from
the transaction and that throws a false positive in the rmap
finishing code.

> The other option would be to make xfs_trans_ijoin a no-op if the inode
> is already joined, except that this wouldn't work with the magic unlock
> on commit. 

I'd much prefer we catch all the places where we are not handling
permanent transaction state correctly as we pass it between
different functions. Especially as we need to do that to get rid of
the log item descriptor abstraction...

> Which is a feature I find horribly confusing, so we should
> get rid of it, for which we'd need to get rid of the concept of
> synchronous transactions in favour of leaving the log force to the
> caller, which again would be more logical.
> 
> Guess I need to look into doing these cleanups as I don't want to force
> them on anyone else.  Just need to finish all the other more important
> bits on the todo list first :)

As always :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Christoph Hellwig]

On Thu, Mar 08, 2018 at 08:07:07AM +1100, Dave Chinner wrote:
> It's in the first patch. I've got to revise it, anyway, because I
> missed the xfs_trans_brelease() case that removes the log item from
> the transaction and that throws a false positive in the rmap
> finishing code.

Please split the infrastructure into a new patch.

Re: [PATCH 1/2] xfs: fix double ijoin in xfs_inactive_symlink_rmt()

[Darrick J. Wong]

On Wed, Mar 07, 2018 at 08:10:19PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> into a transaction it is already joined to. This means the inode can
> have multiple log item descriptors attached to the transaction for
> it. This breaks teh 1:1 mapping that is supposed to exist
> between the log item and log item descriptor.
> 
> This results in the log item being processed twice during
> transaction commit and CIL formatting, and there are lots of other
> potential issues tha arise from double processing of log items in
> the transaction commit state machine.
> 
> In this case, the inode is already held by the rolling transaction
> returned from xfs_defer_finish(), so there's no need to join it
> again.
> 
> Also add a log item flag to say the item has been joined to a
> transaction and assert that it's not set when adding a log item to a
> transaction. Clear it when the log item is disconnected from the log
> item descriptor. This will tell us if there are other log items that
> transactions are referencing multiple times.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>

Looks ok, but please split this patch into one piece that adds more
explicit tracking (and ASSERTing) of the log items' attachment to
transactions and a second patch for the xfs_inactive_symlink_rmt
correction.  Christoph stumbled over the new assert being here, and I
nearly did too.

--D

> ---
>  fs/xfs/xfs_symlink.c | 9 ++-------
>  fs/xfs/xfs_trans.c   | 7 +++++--
>  fs/xfs/xfs_trans.h   | 4 +++-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 2e9e793a8f9d..b0502027070e 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
>  	error = xfs_defer_finish(&tp, &dfops);
>  	if (error)
>  		goto error_bmap_cancel;
> -	/*
> -	 * The first xact was committed, so add the inode to the new one.
> -	 * Mark it dirty so it will be logged and moved forward in the log as
> -	 * part of every commit.
> -	 */
> -	xfs_trans_ijoin(tp, ip, 0);
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +
>  	/*
>  	 * Commit the transaction containing extent freeing and EFDs.
>  	 */
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  	error = xfs_trans_commit(tp);
>  	if (error) {
>  		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 86f92df32c42..2aeeb2ad276a 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -737,12 +737,14 @@ xfs_trans_add_item(
>  
>  	ASSERT(lip->li_mountp == tp->t_mountp);
>  	ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
> +	ASSERT(!(lip->li_flags & XFS_LI_TRANS));
>  
>  	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
>  
>  	lidp->lid_item = lip;
>  	lidp->lid_flags = 0;
>  	list_add_tail(&lidp->lid_trans, &tp->t_items);
> +	lip->li_flags |= XFS_LI_TRANS;
>  
>  	lip->li_desc = lidp;
>  }
> @@ -762,6 +764,7 @@ void
>  xfs_trans_del_item(
>  	struct xfs_log_item	*lip)
>  {
> +	lip->li_flags &= ~XFS_LI_TRANS;
>  	xfs_trans_free_item_desc(lip->li_desc);
>  	lip->li_desc = NULL;
>  }
> @@ -781,15 +784,15 @@ xfs_trans_free_items(
>  	list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
>  		struct xfs_log_item	*lip = lidp->lid_item;
>  
> -		lip->li_desc = NULL;
> +		xfs_trans_del_item(lip);
>  
>  		if (commit_lsn != NULLCOMMITLSN)
>  			lip->li_ops->iop_committing(lip, commit_lsn);
>  		if (abort)
>  			lip->li_flags |= XFS_LI_ABORTED;
> +
>  		lip->li_ops->iop_unlock(lip);
>  
> -		xfs_trans_free_item_desc(lidp);
>  	}
>  }
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 9d542dfe0052..c514486ba2a0 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -67,11 +67,13 @@ typedef struct xfs_log_item {
>  #define	XFS_LI_IN_AIL	0x1
>  #define	XFS_LI_ABORTED	0x2
>  #define	XFS_LI_FAILED	0x4
> +#define	XFS_LI_TRANS	0x8	/* attached to an active transaction */
>  
>  #define XFS_LI_FLAGS \
>  	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
>  	{ XFS_LI_ABORTED,	"ABORTED" }, \
> -	{ XFS_LI_FAILED,	"FAILED" }
> +	{ XFS_LI_FAILED,	"FAILED" }, \
> +	{ XFS_LI_TRANS,		"TRANS" }
>  
>  struct xfs_item_ops {
>  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> -- 
> 2.16.1
> 
> --
> 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

Re: [PATCH 3/2] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()

[Darrick J. Wong]

On Wed, Mar 07, 2018 at 08:19:42PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Another assert failure:
> 
> XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> ....
> xfs_trans_add_item+0xcc/0xe0
> xfs_reflink_clear_inode_flag+0x53/0x120
> xfs_reflink_try_clear_inode_flag+0x5b/0xa0
> ? filemap_write_and_wait+0x4f/0x70
> xfs_reflink_unshare+0x18e/0x19d
> xfs_file_fallocate+0x241/0x310
> ? selinux_file_permission+0xd4/0x140
> vfs_fallocate+0x13d/0x260
> SyS_fallocate+0x43/0x80
> 
> Another fix.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>

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

--D

> ---
>  fs/xfs/xfs_reflink.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 6225d1ea3fdb..5cf60a042e8f 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1554,7 +1554,12 @@ xfs_reflink_inode_has_shared_extents(
>  	return 0;
>  }
>  
> -/* Clear the inode reflink flag if there are no shared extents. */
> +/*
> + * Clear the inode reflink flag if there are no shared extents.
> + *
> + * The caller is responsible for joining the inode to the transaction passed in.
> + * The inode will be joined to the transaction that is returned to the caller.
> + */
>  int
>  xfs_reflink_clear_inode_flag(
>  	struct xfs_inode	*ip,
> @@ -1573,7 +1578,6 @@ xfs_reflink_clear_inode_flag(
>  	 * We didn't find any shared blocks so turn off the reflink flag.
>  	 * First, get rid of any leftover CoW mappings.
>  	 */
> -	xfs_trans_ijoin(*tpp, ip, 0);
>  	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
>  	if (error)
>  		return error;
> --
> 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

Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Darrick J. Wong]

On Wed, Mar 07, 2018 at 08:10:20PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> AN inode is joined to teh same transaction twice in
> xfs_reflink_cancel_cow_range() resulting in the following assert
> failure:
> 
> [   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> [   30.183435] ------------[ cut here ]------------
> ......
> [   30.209264] Call Trace:
> [   30.209935]  xfs_trans_add_item+0xcc/0xe0
> [   30.210968]  xfs_reflink_cancel_cow_blocks+0xab/0x290
> [   30.212249]  ? xfs_trans_reserve+0x1b4/0x2b0
> [   30.213320]  ? kmem_zone_alloc+0x61/0xe0
> [   30.214321]  xfs_reflink_cancel_cow_range+0xb2/0x1f0
> [   30.215616]  xfs_fs_destroy_inode+0x1bd/0x280
> [   30.216757]  dispose_list+0x35/0x40
> [   30.217656]  evict_inodes+0x132/0x160
> [   30.218620]  generic_shutdown_super+0x3a/0x110
> [   30.219771]  kill_block_super+0x21/0x50
> [   30.220762]  deactivate_locked_super+0x39/0x70
> [   30.221909]  cleanup_mnt+0x3b/0x70
> [   30.222819]  task_work_run+0x7f/0xa0
> [   30.223762]  exit_to_usermode_loop+0x9b/0xa0
> [   30.224884]  do_syscall_64+0x18f/0x1a0
> 
> Fix it and document that the callers of
> xfs_reflink_cancel_cow_blocks() must have already joined the inode
> to the permanent transaction passed in.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_reflink.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 8c16177b33d4..6225d1ea3fdb 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
>   *
>   * If cancel_real is true this function cancels all COW fork extents for the
>   * inode; if cancel_real is false, real extents are not cleared.
> + *
> + * Caller must have already joined the inode to the current transaction. The
> + * inode will be joined to the transaction returned to the caller.
>   */
>  int
>  xfs_reflink_cancel_cow_blocks(
> @@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
>  			if (error)
>  				break;
>  		} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
> -			xfs_trans_ijoin(*tpp, ip, 0);

Looks ok, but...

>  			xfs_defer_init(&dfops, &firstfsb);
>  
>  			/* Free the CoW orphan record. */
> @@ -1571,6 +1573,7 @@ xfs_reflink_clear_inode_flag(

Wait, what?  Why are we messing with xfs_reflink_clear_inode_flag here?

(Shame on me for looking at patch 3 before patch 2.)

The comment update in patch 3 is fine (caller must ijoin, function will
ijoin if returning new transaction) but ... didn't this function already
do all this before this churn below?

--D

>  	 * We didn't find any shared blocks so turn off the reflink flag.
>  	 * First, get rid of any leftover CoW mappings.
>  	 */
> +	xfs_trans_ijoin(*tpp, ip, 0);
>  	error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
>  	if (error)
>  		return error;
> @@ -1579,7 +1582,6 @@ xfs_reflink_clear_inode_flag(
>  	trace_xfs_reflink_unset_inode_flag(ip);
>  	ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
>  	xfs_inode_clear_cowblocks_tag(ip);
> -	xfs_trans_ijoin(*tpp, ip, 0);
>  	xfs_trans_log_inode(*tpp, ip, XFS_ILOG_CORE);
>  
>  	return error;
> -- 
> 2.16.1
> 
> --
> 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

Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range

[Dave Chinner]

On Fri, Mar 09, 2018 at 10:43:35AM -0800, Darrick J. Wong wrote:
> On Wed, Mar 07, 2018 at 08:10:20PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > AN inode is joined to teh same transaction twice in
> > xfs_reflink_cancel_cow_range() resulting in the following assert
> > failure:
> > 
> > [   30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> > [   30.183435] ------------[ cut here ]------------
> > ......
> > [   30.209264] Call Trace:
> > [   30.209935]  xfs_trans_add_item+0xcc/0xe0
> > [   30.210968]  xfs_reflink_cancel_cow_blocks+0xab/0x290
> > [   30.212249]  ? xfs_trans_reserve+0x1b4/0x2b0
> > [   30.213320]  ? kmem_zone_alloc+0x61/0xe0
> > [   30.214321]  xfs_reflink_cancel_cow_range+0xb2/0x1f0
> > [   30.215616]  xfs_fs_destroy_inode+0x1bd/0x280
> > [   30.216757]  dispose_list+0x35/0x40
> > [   30.217656]  evict_inodes+0x132/0x160
> > [   30.218620]  generic_shutdown_super+0x3a/0x110
> > [   30.219771]  kill_block_super+0x21/0x50
> > [   30.220762]  deactivate_locked_super+0x39/0x70
> > [   30.221909]  cleanup_mnt+0x3b/0x70
> > [   30.222819]  task_work_run+0x7f/0xa0
> > [   30.223762]  exit_to_usermode_loop+0x9b/0xa0
> > [   30.224884]  do_syscall_64+0x18f/0x1a0
> > 
> > Fix it and document that the callers of
> > xfs_reflink_cancel_cow_blocks() must have already joined the inode
> > to the permanent transaction passed in.
> > 
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_reflink.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 8c16177b33d4..6225d1ea3fdb 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
> >   *
> >   * If cancel_real is true this function cancels all COW fork extents for the
> >   * inode; if cancel_real is false, real extents are not cleared.
> > + *
> > + * Caller must have already joined the inode to the current transaction. The
> > + * inode will be joined to the transaction returned to the caller.
> >   */
> >  int
> >  xfs_reflink_cancel_cow_blocks(
> > @@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
> >  			if (error)
> >  				break;
> >  		} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
> > -			xfs_trans_ijoin(*tpp, ip, 0);
> 
> Looks ok, but...
> 
> >  			xfs_defer_init(&dfops, &firstfsb);
> >  
> >  			/* Free the CoW orphan record. */
> > @@ -1571,6 +1573,7 @@ xfs_reflink_clear_inode_flag(
> 
> Wait, what?  Why are we messing with xfs_reflink_clear_inode_flag here?
> 
> (Shame on me for looking at patch 3 before patch 2.)
> 
> The comment update in patch 3 is fine (caller must ijoin, function will
> ijoin if returning new transaction) but ... didn't this function already
> do all this before this churn below?

Peeling the onion from the inside out. First I fixed
xfs_reflink_cancel_cow_blocks(), then discovered that
xfs_reflink_clear_inode_flag() also joined the inode to the
transaction. Basically, patch 2 was a fix that triggered earlier in
a fstests run, patch three triggered later one after patch 2 was
done.

I've reworked the series - still testing because now I'm hitting
transaction block overruns again - and I'll make sure that it's all
sorted in that series.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com