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

[Josef Bacik <josef@toxicpanda.com>]

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