[PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[Josef Bacik]

With my new locking code dbench is so much faster that I tripped over a
transaction abort from ENOSPC.  This turned out to be because
btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
function sets err on error, and returns err.  So instead of properly
marking the inode as needing a full commit, we were returning -ENOSPC
and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
variable so that we return the correct thing in the case of ENOSPC.

Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-log.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index e0ab3c906119..bc9ed31502ec 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3449,11 +3449,11 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
 	btrfs_free_path(path);
 out_unlock:
 	mutex_unlock(&dir->log_mutex);
-	if (ret == -ENOSPC) {
+	if (err == -ENOSPC) {
 		btrfs_set_log_full_commit(trans);
-		ret = 0;
-	} else if (ret < 0)
-		btrfs_abort_transaction(trans, ret);
+		err = 0;
+	} else if (err < 0 && err != -ENOENT)
+		btrfs_abort_transaction(trans, err);
 
 	btrfs_end_log_trans(root);
 
-- 
2.24.1

Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[Filipe Manana]

On Mon, Aug 10, 2020 at 10:32 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> With my new locking code dbench is so much faster that I tripped over a
> transaction abort from ENOSPC.  This turned out to be because
> btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
> function sets err on error, and returns err.  So instead of properly
> marking the inode as needing a full commit, we were returning -ENOSPC
> and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
> variable so that we return the correct thing in the case of ENOSPC.
>
> Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/tree-log.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index e0ab3c906119..bc9ed31502ec 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3449,11 +3449,11 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
>         btrfs_free_path(path);
>  out_unlock:
>         mutex_unlock(&dir->log_mutex);
> -       if (ret == -ENOSPC) {
> +       if (err == -ENOSPC) {
>                 btrfs_set_log_full_commit(trans);
> -               ret = 0;
> -       } else if (ret < 0)
> -               btrfs_abort_transaction(trans, ret);
> +               err = 0;
> +       } else if (err < 0 && err != -ENOENT)

Why the check for ENOENT?
If any of the directory index items doesn't exist, the respective
functions return a NULL btrfs_dir_item pointer and we do nothing and
return 0.
I'm not seeing anything else that could return ENOENT either.

Other than that it looks good.


> +               btrfs_abort_transaction(trans, err);
>
>         btrfs_end_log_trans(root);
>
> --
> 2.24.1
>


-- 
Filipe David Manana,

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

Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[Josef Bacik]

On 8/11/20 6:14 AM, Filipe Manana wrote:
> On Mon, Aug 10, 2020 at 10:32 PM Josef Bacik <josef@toxicpanda.com> wrote:
>>
>> With my new locking code dbench is so much faster that I tripped over a
>> transaction abort from ENOSPC.  This turned out to be because
>> btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
>> function sets err on error, and returns err.  So instead of properly
>> marking the inode as needing a full commit, we were returning -ENOSPC
>> and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
>> variable so that we return the correct thing in the case of ENOSPC.
>>
>> Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/tree-log.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index e0ab3c906119..bc9ed31502ec 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -3449,11 +3449,11 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
>>          btrfs_free_path(path);
>>   out_unlock:
>>          mutex_unlock(&dir->log_mutex);
>> -       if (ret == -ENOSPC) {
>> +       if (err == -ENOSPC) {
>>                  btrfs_set_log_full_commit(trans);
>> -               ret = 0;
>> -       } else if (ret < 0)
>> -               btrfs_abort_transaction(trans, ret);
>> +               err = 0;
>> +       } else if (err < 0 && err != -ENOENT)
> 
> Why the check for ENOENT?
> If any of the directory index items doesn't exist, the respective
> functions return a NULL btrfs_dir_item pointer and we do nothing and
> return 0.
> I'm not seeing anything else that could return ENOENT either.
> 
> Other than that it looks good.

I missed this too until I tested it and things went wrong.  It's because 
btrfs_lookup_dir_item() can return -ENOENT if the dir item isn't in the tree log 
(which would happen if we hadn't fsync'ed this guy).  We actually handle that 
case in __btrfs_unlink_inode, so it's an expected error to get back.  Thanks,

Josef

Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[Filipe Manana]

On Tue, Aug 11, 2020 at 3:27 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On 8/11/20 6:14 AM, Filipe Manana wrote:
> > On Mon, Aug 10, 2020 at 10:32 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >>
> >> With my new locking code dbench is so much faster that I tripped over a
> >> transaction abort from ENOSPC.  This turned out to be because
> >> btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
> >> function sets err on error, and returns err.  So instead of properly
> >> marking the inode as needing a full commit, we were returning -ENOSPC
> >> and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
> >> variable so that we return the correct thing in the case of ENOSPC.
> >>
> >> Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >> ---
> >>   fs/btrfs/tree-log.c | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> >> index e0ab3c906119..bc9ed31502ec 100644
> >> --- a/fs/btrfs/tree-log.c
> >> +++ b/fs/btrfs/tree-log.c
> >> @@ -3449,11 +3449,11 @@ int btrfs_del_dir_entries_in_log(struct btrfs_trans_handle *trans,
> >>          btrfs_free_path(path);
> >>   out_unlock:
> >>          mutex_unlock(&dir->log_mutex);
> >> -       if (ret == -ENOSPC) {
> >> +       if (err == -ENOSPC) {
> >>                  btrfs_set_log_full_commit(trans);
> >> -               ret = 0;
> >> -       } else if (ret < 0)
> >> -               btrfs_abort_transaction(trans, ret);
> >> +               err = 0;
> >> +       } else if (err < 0 && err != -ENOENT)
> >
> > Why the check for ENOENT?
> > If any of the directory index items doesn't exist, the respective
> > functions return a NULL btrfs_dir_item pointer and we do nothing and
> > return 0.
> > I'm not seeing anything else that could return ENOENT either.
> >
> > Other than that it looks good.
>
> I missed this too until I tested it and things went wrong.  It's because
> btrfs_lookup_dir_item() can return -ENOENT if the dir item isn't in the tree log
> (which would happen if we hadn't fsync'ed this guy).

Hum, looking again I think you meant btrfs_lookup_dir_index_item() and
not btrfs_lookup_dir_item().

The log could have mentioned why we started to check ENOENT, since it
only mentions the bug where we check the wrong variable.

Now it makes sense.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.



> We actually handle that
> case in __btrfs_unlink_inode, so it's an expected error to get back.  Thanks,
>
> Josef



-- 
Filipe David Manana,

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

Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[David Sterba]

On Mon, Aug 10, 2020 at 05:31:16PM -0400, Josef Bacik wrote:
> With my new locking code dbench is so much faster that I tripped over a
> transaction abort from ENOSPC.  This turned out to be because
> btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
> function sets err on error, and returns err.  So instead of properly
> marking the inode as needing a full commit, we were returning -ENOSPC
> and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
> variable so that we return the correct thing in the case of ENOSPC.
> 
> Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Added to misc-next, with updated changelog and comment explaining the
ENOENT.

Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[Filipe Manana]

On Wed, Aug 19, 2020 at 5:25 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Aug 10, 2020 at 05:31:16PM -0400, Josef Bacik wrote:
> > With my new locking code dbench is so much faster that I tripped over a
> > transaction abort from ENOSPC.  This turned out to be because
> > btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
> > function sets err on error, and returns err.  So instead of properly
> > marking the inode as needing a full commit, we were returning -ENOSPC
> > and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
> > variable so that we return the correct thing in the case of ENOSPC.
> >
> > Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>
> Added to misc-next, with updated changelog and comment explaining the
> ENOENT.

Looking at the part added to the changelog:

"The ENOENT needs to be checked, because btrfs_lookup_dir_item() can
return -ENOENT if the dir item isn't in the tree log (which would happen
if we hadn't fsync'ed this guy).  We actually handle that case in
__btrfs_unlink_inode, so it's an expected error to get back."

btrfs_lookup_dir_item() returns NULL when the dir item does not exist
in the log.
What can return -ENOENT is btrfs_lookup_dir_index_item(), which we
call right after calling btrfs_lookup_dir_item().
The fact that one returns NULL and the other returns -ENOENT is what
made me question why the special handling for -ENOENT.

Other than the wrong function name, it looks good to me.

Thanks.






--
Filipe David Manana,

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

Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

[David Sterba]

On Thu, Aug 20, 2020 at 11:29:11AM +0100, Filipe Manana wrote:
> On Wed, Aug 19, 2020 at 5:25 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Mon, Aug 10, 2020 at 05:31:16PM -0400, Josef Bacik wrote:
> > > With my new locking code dbench is so much faster that I tripped over a
> > > transaction abort from ENOSPC.  This turned out to be because
> > > btrfs_del_dir_entries_in_log was checking for ret == -ENOSPC, but this
> > > function sets err on error, and returns err.  So instead of properly
> > > marking the inode as needing a full commit, we were returning -ENOSPC
> > > and aborting in __btrfs_unlink_inode.  Fix this by checking the proper
> > > variable so that we return the correct thing in the case of ENOSPC.
> > >
> > > Fixes: 4a500fd178c8 ("Btrfs: Metadata ENOSPC handling for tree log")
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >
> > Added to misc-next, with updated changelog and comment explaining the
> > ENOENT.
> 
> Looking at the part added to the changelog:
> 
> "The ENOENT needs to be checked, because btrfs_lookup_dir_item() can
> return -ENOENT if the dir item isn't in the tree log (which would happen
> if we hadn't fsync'ed this guy).  We actually handle that case in
> __btrfs_unlink_inode, so it's an expected error to get back."
> 
> btrfs_lookup_dir_item() returns NULL when the dir item does not exist
> in the log.
> What can return -ENOENT is btrfs_lookup_dir_index_item(), which we
> call right after calling btrfs_lookup_dir_item().
> The fact that one returns NULL and the other returns -ENOENT is what
> made me question why the special handling for -ENOENT.
> 
> Other than the wrong function name, it looks good to me.

Function name updated in the patch, thanks.