xfs: fix transaction joining problems.
cover xfs: fix transaction joining problems.
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()
From: Dave Chinner <[email protected]>
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.
--- 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(-)
fs/xfs/xfs_symlink.c +2 -7
@@ -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));
fs/xfs/xfs_trans.c +5 -2
@@ -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 @@ voidxfs_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); } }
fs/xfs/xfs_trans.h +3 -1
@@ -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 *);
Signature · 1 line
2.16.1
Re: [PATCH 1/2] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
Quoted 17 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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.
Quoted 7 lines
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 <[email protected]>
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
Quoted 100 lines
--- 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(-)fs/xfs/xfs_symlink.c +2
-7@@ -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));fs/xfs/xfs_trans.c +5
-2@@ -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 @@ voidxfs_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); } }fs/xfs/xfs_trans.h +3
-1@@ -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 *);Signature · 6 lines
2.16.1 -- 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
PATCH 2/2 xfs: fix double ijoin in xfs_reflink_cancel_cow_range
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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.
--- fs/xfs/xfs_reflink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
fs/xfs/xfs_reflink.c +4 -2
@@ -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;
Signature · 1 line
2.16.1
Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
Quoted 5 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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 :)
Quoted 2 lines
[ 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
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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...
Quoted 4 lines
Quoted 1 line
[ 30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740That 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.
Quoted 3 lines
The other option would be to make
xfs_trans_ijoina 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...
Quoted 8 lines
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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
Quoted 4 lines · Dave Chinner
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 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
In reply to Dave ChinnerQuoted 50 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1a0Fix 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 <[email protected]>--- fs/xfs/xfs_reflink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)fs/xfs/xfs_reflink.c +3
-0@@ -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(
fs/xfs/xfs_reflink.c xfs_reflink_cancel_cow_blocks( · line 595
Quoted 4 lines
fs/xfs/xfs_reflink.c +0
-1@@ -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...
fs/xfs/xfs_reflink.c xfs_reflink_clear_inode_flag(
Quoted 4 lines
fs/xfs/xfs_reflink.c +0
-0xfs_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
Quoted 22 lines
fs/xfs/xfs_reflink.c +1
-1* 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;Signature · 6 lines
2.16.1 -- 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
Re: [PATCH 2/2] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
Quoted 65 lines · Darrick J. Wong
Quoted 50 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1a0Fix 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 <[email protected]>--- fs/xfs/xfs_reflink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)fs/xfs/xfs_reflink.c +3
-0@@ -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(fs/xfs/xfs_reflink.c xfs_reflink_cancel_cow_blocks( · line 595
Quoted 4 lines
fs/xfs/xfs_reflink.c +0
-1@@ -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...
fs/xfs/xfs_reflink.c xfs_reflink_clear_inode_flag(
Quoted 4 lines
fs/xfs/xfs_reflink.c +0
-0xfs_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_flaghere?
Quoted 5 lines
(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.
Signature · 2 lines
Dave Chinner [email protected]
[PATCH 3/2] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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.
--- fs/xfs/xfs_reflink.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
fs/xfs/xfs_reflink.c +6 -2
@@ -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;
Darrick J. Wong
Reviewed-byRe: [PATCH 3/2] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
Quoted 16 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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
Quoted 3 lines
Another fix.
Signed-off-by Dave Chinner <[email protected]>
Looks ok,
--D
Quoted 34 lines
--- fs/xfs/xfs_reflink.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)fs/xfs/xfs_reflink.c +6
-3@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html