| Message ID | 20200810213116.795789-1-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: check the right variable in btrfs_del_dir_entries_in_log | expand |
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 >
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
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
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 <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.”
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.
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);
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(-)