* [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
@ 2020-08-10 21:31 Josef Bacik
2020-08-11 10:14 ` Filipe Manana
2020-08-19 16:22 ` David Sterba
0 siblings, 2 replies; 7+ messages in thread
From: Josef Bacik @ 2020-08-10 21:31 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
2020-08-10 21:31 [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log Josef Bacik
@ 2020-08-11 10:14 ` Filipe Manana
2020-08-11 14:27 ` Josef Bacik
2020-08-19 16:22 ` David Sterba
1 sibling, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2020-08-11 10:14 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
2020-08-11 10:14 ` Filipe Manana
@ 2020-08-11 14:27 ` Josef Bacik
2020-08-11 14:38 ` Filipe Manana
0 siblings, 1 reply; 7+ messages in thread
From: Josef Bacik @ 2020-08-11 14:27 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs, kernel-team
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
2020-08-11 14:27 ` Josef Bacik
@ 2020-08-11 14:38 ` Filipe Manana
0 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2020-08-11 14:38 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
2020-08-10 21:31 [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log Josef Bacik
2020-08-11 10:14 ` Filipe Manana
@ 2020-08-19 16:22 ` David Sterba
2020-08-20 10:29 ` Filipe Manana
1 sibling, 1 reply; 7+ messages in thread
From: David Sterba @ 2020-08-19 16:22 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
2020-08-19 16:22 ` David Sterba
@ 2020-08-20 10:29 ` Filipe Manana
2020-08-21 10:21 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2020-08-20 10:29 UTC (permalink / raw)
To: dsterba, Josef Bacik, linux-btrfs, kernel-team
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.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log
2020-08-20 10:29 ` Filipe Manana
@ 2020-08-21 10:21 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2020-08-21 10:21 UTC (permalink / raw)
To: Filipe Manana; +Cc: dsterba, Josef Bacik, linux-btrfs, kernel-team
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-08-21 10:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-10 21:31 [PATCH] btrfs: check the right variable in btrfs_del_dir_entries_in_log Josef Bacik
2020-08-11 10:14 ` Filipe Manana
2020-08-11 14:27 ` Josef Bacik
2020-08-11 14:38 ` Filipe Manana
2020-08-19 16:22 ` David Sterba
2020-08-20 10:29 ` Filipe Manana
2020-08-21 10:21 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.