| Message ID | 1f84722853326611d5d0d6c74e7af75be7b5928d.1601384009.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: cleanup cow block on error | expand |
On 29/09/2020 14:54, Josef Bacik wrote: > With my automated fstest runs I noticed one of my workers didn't report > results. Turns out it was hung trying to write back inodes, and the > write back thread was stuck trying to lock an extent buffer > > [root@xfstests2 xfstests-dev]# cat /proc/2143497/stack > [<0>] __btrfs_tree_lock+0x108/0x250 > [<0>] lock_extent_buffer_for_io+0x35e/0x3a0 > [<0>] btree_write_cache_pages+0x15a/0x3b0 > [<0>] do_writepages+0x28/0xb0 > [<0>] __writeback_single_inode+0x54/0x5c0 > [<0>] writeback_sb_inodes+0x1e8/0x510 > [<0>] wb_writeback+0xcc/0x440 > [<0>] wb_workfn+0xd7/0x650 > [<0>] process_one_work+0x236/0x560 > [<0>] worker_thread+0x55/0x3c0 > [<0>] kthread+0x13a/0x150 > [<0>] ret_from_fork+0x1f/0x30 > > This is because we got an error while cow'ing a block, specifically here > > if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > ret = btrfs_reloc_cow_block(trans, root, buf, cow); > if (ret) { > btrfs_abort_transaction(trans, ret); > return ret; > } > } > > The problem here is that as soon as we allocate the new block it is > locked and marked dirty in the btree inode. This means that we could > attempt to writeback this block and need to lock the extent buffer. > However we're not unlocking it here and thus we deadlock. > > Fix this by unlocking the cow block if we have any errors inside of > __btrfs_cow_block, and also free it so we do not leak it. > > Fixes: 65b51a009e29 ("btrfs_search_slot: reduce lock contention by cowing in two stages") > Reviewed-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/ctree.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index a165093739c4..113da62dc17f 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1064,6 +1064,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > > ret = update_ref_for_cow(trans, root, buf, cow, &last_ref); > if (ret) { > + btrfs_tree_unlock(cow); > + free_extent_buffer(cow); > btrfs_abort_transaction(trans, ret); > return ret; > } > @@ -1071,6 +1073,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > ret = btrfs_reloc_cow_block(trans, root, buf, cow); > if (ret) { > + btrfs_tree_unlock(cow); > + free_extent_buffer(cow); > btrfs_abort_transaction(trans, ret); > return ret; > } > @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > if (last_ref) { > ret = tree_mod_log_free_eb(buf); > if (ret) { > + btrfs_tree_unlock(cow); > + free_extent_buffer(cow); > btrfs_abort_transaction(trans, ret); > return ret; > } > I don't want to be a party pooper here but, now you have this pattern: if (ret) { btrfs_tree_unlock(cow); free_extent_buffer(cow); btrfs_abort_transaction(trans, ret); return ret; } repeated three times. I think this should be consolidated in a 'goto err' or something.
On Tue, Sep 29, 2020 at 1:55 PM Josef Bacik <josef@toxicpanda.com> wrote: > > With my automated fstest runs I noticed one of my workers didn't report > results. Turns out it was hung trying to write back inodes, and the > write back thread was stuck trying to lock an extent buffer > > [root@xfstests2 xfstests-dev]# cat /proc/2143497/stack > [<0>] __btrfs_tree_lock+0x108/0x250 > [<0>] lock_extent_buffer_for_io+0x35e/0x3a0 > [<0>] btree_write_cache_pages+0x15a/0x3b0 > [<0>] do_writepages+0x28/0xb0 > [<0>] __writeback_single_inode+0x54/0x5c0 > [<0>] writeback_sb_inodes+0x1e8/0x510 > [<0>] wb_writeback+0xcc/0x440 > [<0>] wb_workfn+0xd7/0x650 > [<0>] process_one_work+0x236/0x560 > [<0>] worker_thread+0x55/0x3c0 > [<0>] kthread+0x13a/0x150 > [<0>] ret_from_fork+0x1f/0x30 > > This is because we got an error while cow'ing a block, specifically here > > if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > ret = btrfs_reloc_cow_block(trans, root, buf, cow); > if (ret) { > btrfs_abort_transaction(trans, ret); > return ret; > } > } > > The problem here is that as soon as we allocate the new block it is > locked and marked dirty in the btree inode. This means that we could > attempt to writeback this block and need to lock the extent buffer. > However we're not unlocking it here and thus we deadlock. > > Fix this by unlocking the cow block if we have any errors inside of > __btrfs_cow_block, and also free it so we do not leak it. > > Fixes: 65b51a009e29 ("btrfs_search_slot: reduce lock contention by cowing in two stages") > Reviewed-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/ctree.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index a165093739c4..113da62dc17f 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1064,6 +1064,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > > ret = update_ref_for_cow(trans, root, buf, cow, &last_ref); > if (ret) { > + btrfs_tree_unlock(cow); > + free_extent_buffer(cow); > btrfs_abort_transaction(trans, ret); > return ret; > } > @@ -1071,6 +1073,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > ret = btrfs_reloc_cow_block(trans, root, buf, cow); > if (ret) { > + btrfs_tree_unlock(cow); > + free_extent_buffer(cow); > btrfs_abort_transaction(trans, ret); > return ret; > } > @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > if (last_ref) { > ret = tree_mod_log_free_eb(buf); > if (ret) { > + btrfs_tree_unlock(cow); > + free_extent_buffer(cow); The tree here already has a node pointing to the new buffer ("cow"), so we shouldn't call free_extent_buffer() against it. For all the previous places it's fine. Thanks. > btrfs_abort_transaction(trans, ret); > return ret; > } > -- > 2.26.2 >
On Tue, Sep 29, 2020 at 01:16:50PM +0000, Johannes Thumshirn wrote: > On 29/09/2020 14:54, Josef Bacik wrote: > > @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > > if (last_ref) { > > ret = tree_mod_log_free_eb(buf); > > if (ret) { > > + btrfs_tree_unlock(cow); > > + free_extent_buffer(cow); > > btrfs_abort_transaction(trans, ret); > > return ret; > > } > > > > > I don't want to be a party pooper here but, now you have this pattern: > > if (ret) { > btrfs_tree_unlock(cow); > free_extent_buffer(cow); > btrfs_abort_transaction(trans, ret); > return ret; > } > > repeated three times. I think this should be consolidated in a 'goto err' or something. Hah, you think _you_ are the party pooper? https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort
On 9/29/20 9:19 AM, Filipe Manana wrote: > On Tue, Sep 29, 2020 at 1:55 PM Josef Bacik <josef@toxicpanda.com> wrote: >> >> With my automated fstest runs I noticed one of my workers didn't report >> results. Turns out it was hung trying to write back inodes, and the >> write back thread was stuck trying to lock an extent buffer >> >> [root@xfstests2 xfstests-dev]# cat /proc/2143497/stack >> [<0>] __btrfs_tree_lock+0x108/0x250 >> [<0>] lock_extent_buffer_for_io+0x35e/0x3a0 >> [<0>] btree_write_cache_pages+0x15a/0x3b0 >> [<0>] do_writepages+0x28/0xb0 >> [<0>] __writeback_single_inode+0x54/0x5c0 >> [<0>] writeback_sb_inodes+0x1e8/0x510 >> [<0>] wb_writeback+0xcc/0x440 >> [<0>] wb_workfn+0xd7/0x650 >> [<0>] process_one_work+0x236/0x560 >> [<0>] worker_thread+0x55/0x3c0 >> [<0>] kthread+0x13a/0x150 >> [<0>] ret_from_fork+0x1f/0x30 >> >> This is because we got an error while cow'ing a block, specifically here >> >> if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { >> ret = btrfs_reloc_cow_block(trans, root, buf, cow); >> if (ret) { >> btrfs_abort_transaction(trans, ret); >> return ret; >> } >> } >> >> The problem here is that as soon as we allocate the new block it is >> locked and marked dirty in the btree inode. This means that we could >> attempt to writeback this block and need to lock the extent buffer. >> However we're not unlocking it here and thus we deadlock. >> >> Fix this by unlocking the cow block if we have any errors inside of >> __btrfs_cow_block, and also free it so we do not leak it. >> >> Fixes: 65b51a009e29 ("btrfs_search_slot: reduce lock contention by cowing in two stages") >> Reviewed-by: Filipe Manana <fdmanana@suse.com> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> >> --- >> fs/btrfs/ctree.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c >> index a165093739c4..113da62dc17f 100644 >> --- a/fs/btrfs/ctree.c >> +++ b/fs/btrfs/ctree.c >> @@ -1064,6 +1064,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, >> >> ret = update_ref_for_cow(trans, root, buf, cow, &last_ref); >> if (ret) { >> + btrfs_tree_unlock(cow); >> + free_extent_buffer(cow); >> btrfs_abort_transaction(trans, ret); >> return ret; >> } >> @@ -1071,6 +1073,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, >> if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { >> ret = btrfs_reloc_cow_block(trans, root, buf, cow); >> if (ret) { >> + btrfs_tree_unlock(cow); >> + free_extent_buffer(cow); >> btrfs_abort_transaction(trans, ret); >> return ret; >> } >> @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, >> if (last_ref) { >> ret = tree_mod_log_free_eb(buf); >> if (ret) { >> + btrfs_tree_unlock(cow); >> + free_extent_buffer(cow); > > The tree here already has a node pointing to the new buffer ("cow"), > so we shouldn't call free_extent_buffer() against it. > For all the previous places it's fine. > We still need to drop our ref for it, just because the tree is pointing to it doesn't mean we hold the ref for it forever. Thanks, Josef
On 29/09/2020 15:22, David Sterba wrote: > On Tue, Sep 29, 2020 at 01:16:50PM +0000, Johannes Thumshirn wrote: >> On 29/09/2020 14:54, Josef Bacik wrote: >>> @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, >>> if (last_ref) { >>> ret = tree_mod_log_free_eb(buf); >>> if (ret) { >>> + btrfs_tree_unlock(cow); >>> + free_extent_buffer(cow); >>> btrfs_abort_transaction(trans, ret); >>> return ret; >>> } >>> >> >> >> I don't want to be a party pooper here but, now you have this pattern: >> >> if (ret) { >> btrfs_tree_unlock(cow); >> free_extent_buffer(cow); >> btrfs_abort_transaction(trans, ret); >> return ret; >> } >> >> repeated three times. I think this should be consolidated in a 'goto err' or something. > > Hah, you think _you_ are the party pooper? > > https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort > Args, you're right.
On Tue, Sep 29, 2020 at 2:23 PM Josef Bacik <josef@toxicpanda.com> wrote: > > On 9/29/20 9:19 AM, Filipe Manana wrote: > > On Tue, Sep 29, 2020 at 1:55 PM Josef Bacik <josef@toxicpanda.com> wrote: > >> > >> With my automated fstest runs I noticed one of my workers didn't report > >> results. Turns out it was hung trying to write back inodes, and the > >> write back thread was stuck trying to lock an extent buffer > >> > >> [root@xfstests2 xfstests-dev]# cat /proc/2143497/stack > >> [<0>] __btrfs_tree_lock+0x108/0x250 > >> [<0>] lock_extent_buffer_for_io+0x35e/0x3a0 > >> [<0>] btree_write_cache_pages+0x15a/0x3b0 > >> [<0>] do_writepages+0x28/0xb0 > >> [<0>] __writeback_single_inode+0x54/0x5c0 > >> [<0>] writeback_sb_inodes+0x1e8/0x510 > >> [<0>] wb_writeback+0xcc/0x440 > >> [<0>] wb_workfn+0xd7/0x650 > >> [<0>] process_one_work+0x236/0x560 > >> [<0>] worker_thread+0x55/0x3c0 > >> [<0>] kthread+0x13a/0x150 > >> [<0>] ret_from_fork+0x1f/0x30 > >> > >> This is because we got an error while cow'ing a block, specifically here > >> > >> if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > >> ret = btrfs_reloc_cow_block(trans, root, buf, cow); > >> if (ret) { > >> btrfs_abort_transaction(trans, ret); > >> return ret; > >> } > >> } > >> > >> The problem here is that as soon as we allocate the new block it is > >> locked and marked dirty in the btree inode. This means that we could > >> attempt to writeback this block and need to lock the extent buffer. > >> However we're not unlocking it here and thus we deadlock. > >> > >> Fix this by unlocking the cow block if we have any errors inside of > >> __btrfs_cow_block, and also free it so we do not leak it. > >> > >> Fixes: 65b51a009e29 ("btrfs_search_slot: reduce lock contention by cowing in two stages") > >> Reviewed-by: Filipe Manana <fdmanana@suse.com> > >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > >> --- > >> fs/btrfs/ctree.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > >> index a165093739c4..113da62dc17f 100644 > >> --- a/fs/btrfs/ctree.c > >> +++ b/fs/btrfs/ctree.c > >> @@ -1064,6 +1064,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > >> > >> ret = update_ref_for_cow(trans, root, buf, cow, &last_ref); > >> if (ret) { > >> + btrfs_tree_unlock(cow); > >> + free_extent_buffer(cow); > >> btrfs_abort_transaction(trans, ret); > >> return ret; > >> } > >> @@ -1071,6 +1073,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > >> if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > >> ret = btrfs_reloc_cow_block(trans, root, buf, cow); > >> if (ret) { > >> + btrfs_tree_unlock(cow); > >> + free_extent_buffer(cow); > >> btrfs_abort_transaction(trans, ret); > >> return ret; > >> } > >> @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, > >> if (last_ref) { > >> ret = tree_mod_log_free_eb(buf); > >> if (ret) { > >> + btrfs_tree_unlock(cow); > >> + free_extent_buffer(cow); > > > > The tree here already has a node pointing to the new buffer ("cow"), > > so we shouldn't call free_extent_buffer() against it. > > For all the previous places it's fine. > > > > We still need to drop our ref for it, just because the tree is pointing to it > doesn't mean we hold the ref for it forever. Thanks, Nevermind, it's a block number and not a memory pointer as I was thinking before. It's fine indeed. > > Josef
On Tue, Sep 29, 2020 at 08:53:54AM -0400, Josef Bacik wrote: > With my automated fstest runs I noticed one of my workers didn't report > results. Turns out it was hung trying to write back inodes, and the > write back thread was stuck trying to lock an extent buffer Which test was that? > [root@xfstests2 xfstests-dev]# cat /proc/2143497/stack > [<0>] __btrfs_tree_lock+0x108/0x250 > [<0>] lock_extent_buffer_for_io+0x35e/0x3a0 > [<0>] btree_write_cache_pages+0x15a/0x3b0 > [<0>] do_writepages+0x28/0xb0 > [<0>] __writeback_single_inode+0x54/0x5c0 > [<0>] writeback_sb_inodes+0x1e8/0x510 > [<0>] wb_writeback+0xcc/0x440 > [<0>] wb_workfn+0xd7/0x650 > [<0>] process_one_work+0x236/0x560 > [<0>] worker_thread+0x55/0x3c0 > [<0>] kthread+0x13a/0x150 > [<0>] ret_from_fork+0x1f/0x30 > > This is because we got an error while cow'ing a block, specifically here > > if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { > ret = btrfs_reloc_cow_block(trans, root, buf, cow); > if (ret) { > btrfs_abort_transaction(trans, ret); > return ret; > } > } > > The problem here is that as soon as we allocate the new block it is > locked and marked dirty in the btree inode. This means that we could > attempt to writeback this block and need to lock the extent buffer. > However we're not unlocking it here and thus we deadlock. > > Fix this by unlocking the cow block if we have any errors inside of > __btrfs_cow_block, and also free it so we do not leak it. > > Fixes: 65b51a009e29 ("btrfs_search_slot: reduce lock contention by cowing in two stages") This is a commit from 2008, though it has some overlap with the fixed code, there have been many changes meanwhile and I don't see a clear logic that would point to that. The functions with missing free after failure are nowhere in that commit. The patch is applicable to all the stable trees so that's not a problem, but the Fixes: tag should be a close match, though it's not always easy and in this case I'd rather leave it out.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index a165093739c4..113da62dc17f 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1064,6 +1064,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, ret = update_ref_for_cow(trans, root, buf, cow, &last_ref); if (ret) { + btrfs_tree_unlock(cow); + free_extent_buffer(cow); btrfs_abort_transaction(trans, ret); return ret; } @@ -1071,6 +1073,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { ret = btrfs_reloc_cow_block(trans, root, buf, cow); if (ret) { + btrfs_tree_unlock(cow); + free_extent_buffer(cow); btrfs_abort_transaction(trans, ret); return ret; } @@ -1103,6 +1107,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans, if (last_ref) { ret = tree_mod_log_free_eb(buf); if (ret) { + btrfs_tree_unlock(cow); + free_extent_buffer(cow); btrfs_abort_transaction(trans, ret); return ret; }
With my automated fstest runs I noticed one of my workers didn't report results. Turns out it was hung trying to write back inodes, and the write back thread was stuck trying to lock an extent buffer [root@xfstests2 xfstests-dev]# cat /proc/2143497/stack [<0>] __btrfs_tree_lock+0x108/0x250 [<0>] lock_extent_buffer_for_io+0x35e/0x3a0 [<0>] btree_write_cache_pages+0x15a/0x3b0 [<0>] do_writepages+0x28/0xb0 [<0>] __writeback_single_inode+0x54/0x5c0 [<0>] writeback_sb_inodes+0x1e8/0x510 [<0>] wb_writeback+0xcc/0x440 [<0>] wb_workfn+0xd7/0x650 [<0>] process_one_work+0x236/0x560 [<0>] worker_thread+0x55/0x3c0 [<0>] kthread+0x13a/0x150 [<0>] ret_from_fork+0x1f/0x30 This is because we got an error while cow'ing a block, specifically here if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) { ret = btrfs_reloc_cow_block(trans, root, buf, cow); if (ret) { btrfs_abort_transaction(trans, ret); return ret; } } The problem here is that as soon as we allocate the new block it is locked and marked dirty in the btree inode. This means that we could attempt to writeback this block and need to lock the extent buffer. However we're not unlocking it here and thus we deadlock. Fix this by unlocking the cow block if we have any errors inside of __btrfs_cow_block, and also free it so we do not leak it. Fixes: 65b51a009e29 ("btrfs_search_slot: reduce lock contention by cowing in two stages") Reviewed-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/ctree.c | 6 ++++++ 1 file changed, 6 insertions(+)