| Message ID | 20181122131528.19421-1-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: improve error handling of btrfs_add_link() | expand |
On Thu, Nov 22, 2018 at 02:15:28PM +0100, Johannes Thumshirn wrote: > err holds the return value of either btrfs_del_root_ref() or > btrfs_del_inode_ref() but it hasn't been checked since it's > introduction with commit fe66a05a0679 (Btrfs: improve error handling > for btrfs_insert_dir_item callers) in 2012. > > The first attempt in removing the variable was rejected, so instead of > removing 'err', start handling the errors of > btrfs_del_root_ref()/btrfs_del_inode_ref() by aborting the transaction in > btrfs_add_link() directly instead of the call-sites. ... which is an anti-pattern. https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort In case there are several conditons that can fail we want to see the first one for later analysis and debugging.
On 22/11/2018 14:35, David Sterba wrote: > On Thu, Nov 22, 2018 at 02:15:28PM +0100, Johannes Thumshirn wrote: >> err holds the return value of either btrfs_del_root_ref() or >> btrfs_del_inode_ref() but it hasn't been checked since it's >> introduction with commit fe66a05a0679 (Btrfs: improve error handling >> for btrfs_insert_dir_item callers) in 2012. >> >> The first attempt in removing the variable was rejected, so instead of >> removing 'err', start handling the errors of >> btrfs_del_root_ref()/btrfs_del_inode_ref() by aborting the transaction in >> btrfs_add_link() directly instead of the call-sites. > > ... which is an anti-pattern. > > https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort > > In case there are several conditons that can fail we want to see the > first one for later analysis and debugging. OK, but then handling the error of either btrfs_del_root_ref() or btrfs_del_inode_ref() will essentially only be printing an error message as anything else would hide the error condition which has led us into this path (i.e. btrfs_insert_dir_item() returning EEXISTS or EOVERFLOW), wouldn't it? Byte, Johannes
On Thu, Nov 22, 2018 at 02:44:35PM +0100, Johannes Thumshirn wrote: > On 22/11/2018 14:35, David Sterba wrote: > > On Thu, Nov 22, 2018 at 02:15:28PM +0100, Johannes Thumshirn wrote: > >> err holds the return value of either btrfs_del_root_ref() or > >> btrfs_del_inode_ref() but it hasn't been checked since it's > >> introduction with commit fe66a05a0679 (Btrfs: improve error handling > >> for btrfs_insert_dir_item callers) in 2012. > >> > >> The first attempt in removing the variable was rejected, so instead of > >> removing 'err', start handling the errors of > >> btrfs_del_root_ref()/btrfs_del_inode_ref() by aborting the transaction in > >> btrfs_add_link() directly instead of the call-sites. > > > > ... which is an anti-pattern. > > > > https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort > > > > In case there are several conditons that can fail we want to see the > > first one for later analysis and debugging. > > OK, but then handling the error of either btrfs_del_root_ref() or > btrfs_del_inode_ref() will essentially only be printing an error message > as anything else would hide the error condition which has led us into > this path (i.e. btrfs_insert_dir_item() returning EEXISTS or EOVERFLOW), > wouldn't it? So this seems to point out there are more things to resolve here: * how to recover from errors of btrfs_del_root_ref or btrfs_del_inode_ref * what to report back to the user When insert_dir_item fails, there are the inode references to be cleaned up, and error reported as EEXIST or EOVERFLOW. When btrfs_del_root_ref or btrfs_del_inode_ref fail, the cleanup is not possible and there's no other option than the transaction abort. As the original reason was the overflow, this should be reported to back to the user. The reason of the del_*_ref failures can be misleading, in case it's eg. ENOMEM. This would mean that the add-link operation is possible but there was not enough memory and restarting later would let it possibly succeed. So the missing part of error hanling is to add two "if (err) abort" and then still return 'ret'. As this is not all obvious why, a comment would be good there.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 9becf8543489..314142ea9d80 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -6351,6 +6351,7 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, struct btrfs_root *root = parent_inode->root; u64 ino = btrfs_ino(inode); u64 parent_ino = btrfs_ino(parent_inode); + int err = 0; if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) { memcpy(&key, &inode->root->root_key, sizeof(key)); @@ -6395,18 +6396,20 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, fail_dir_item: if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) { u64 local_index; - int err; + err = btrfs_del_root_ref(trans, key.objectid, root->root_key.objectid, parent_ino, &local_index, name, name_len); } else if (add_backref) { u64 local_index; - int err; err = btrfs_del_inode_ref(trans, root, name, name_len, ino, parent_ino, &local_index); } + + btrfs_abort_transaction(trans, err ? err : ret); + return ret; } @@ -9502,18 +9505,14 @@ static int btrfs_rename_exchange(struct inode *old_dir, ret = btrfs_add_link(trans, BTRFS_I(new_dir), BTRFS_I(old_inode), new_dentry->d_name.name, new_dentry->d_name.len, 0, old_idx); - if (ret) { - btrfs_abort_transaction(trans, ret); + if (ret) goto out_fail; - } ret = btrfs_add_link(trans, BTRFS_I(old_dir), BTRFS_I(new_inode), old_dentry->d_name.name, old_dentry->d_name.len, 0, new_idx); - if (ret) { - btrfs_abort_transaction(trans, ret); + if (ret) goto out_fail; - } if (old_inode->i_nlink == 1) BTRFS_I(old_inode)->dir_index = old_idx; @@ -9822,10 +9821,8 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, ret = btrfs_add_link(trans, BTRFS_I(new_dir), BTRFS_I(old_inode), new_dentry->d_name.name, new_dentry->d_name.len, 0, index); - if (ret) { - btrfs_abort_transaction(trans, ret); + if (ret) goto out_fail; - } if (old_inode->i_nlink == 1) BTRFS_I(old_inode)->dir_index = index;
err holds the return value of either btrfs_del_root_ref() or btrfs_del_inode_ref() but it hasn't been checked since it's introduction with commit fe66a05a0679 (Btrfs: improve error handling for btrfs_insert_dir_item callers) in 2012. The first attempt in removing the variable was rejected, so instead of removing 'err', start handling the errors of btrfs_del_root_ref()/btrfs_del_inode_ref() by aborting the transaction in btrfs_add_link() directly instead of the call-sites. Link: https://lore.kernel.org/linux-btrfs/20181119141323.GC24115@twin.jikos.cz/ Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- fs/btrfs/inode.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)