[PATCH v2] btrfs: cleanup cow block on error

[PATCH v2] btrfs: cleanup cow block on error

[Josef Bacik]

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;
 			}
-- 
2.26.2

Re: [PATCH v2] btrfs: cleanup cow block on error

[Johannes Thumshirn]

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.

Re: [PATCH v2] btrfs: cleanup cow block on error

[Filipe Manana]

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
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: cleanup cow block on error

[David Sterba]

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

Re: [PATCH v2] btrfs: cleanup cow block on error

[Josef Bacik]

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

Re: [PATCH v2] btrfs: cleanup cow block on error

[Johannes Thumshirn]

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.

Re: [PATCH v2] btrfs: cleanup cow block on error

[Filipe Manana]

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



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: cleanup cow block on error

[David Sterba]

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.