[PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log

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

On Mon, Aug 10, 2020 at 10:32 PM Josef Bacik [off-list ref] wrote:

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.”

On 8/11/20 6:14 AM, Filipe Manana wrote:

On Mon, Aug 10, 2020 at 10:32 PM Josef Bacik [off-list ref] wrote:

quoted

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

On Tue, Aug 11, 2020 at 3:27 PM Josef Bacik [off-list ref] wrote:

On 8/11/20 6:14 AM, Filipe Manana wrote:

quoted

On Mon, Aug 10, 2020 at 10:32 PM Josef Bacik [off-list ref] wrote:

quoted

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 <redacted>

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.”

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.

On Wed, Aug 19, 2020 at 5:25 PM David Sterba [off-list ref] wrote:

On Mon, Aug 10, 2020 at 05:31:16PM -0400, Josef Bacik wrote:

quoted

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.”

On Thu, Aug 20, 2020 at 11:29:11AM +0100, Filipe Manana wrote:

On Wed, Aug 19, 2020 at 5:25 PM David Sterba [off-list ref] wrote:

quoted

On Mon, Aug 10, 2020 at 05:31:16PM -0400, Josef Bacik wrote:

quoted

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.