On Tue, Dec 11, 2018 at 04:03:15PM +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. > > To quote David: > > "If the error handling in the error handling fails, there's not much left to do > and the abort either happened earlier in the callees or is necessary here." > > So if one of btrfs_del_root_ref() or btrfs_del_inode_ref() failed, abort the > transaction. > > Link: https://lore.kernel.org/linux-btrfs/20181126183759.GJ2842@xxxxxxxxxxxxx/ > Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx> > --- > fs/btrfs/inode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index c5c27d5457cc..c9457d2580c0 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -6311,6 +6311,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)); > @@ -6355,18 +6356,21 @@ 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); > } > + > + if (err) > + btrfs_abort_transaction(trans, err); > + > return ret; > } What I wanted is this: --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -6360,12 +6360,16 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, root->root_key.objectid, parent_ino, &local_index, name, name_len); + if (err) + btrrfs_abort_transaction(trans, err); } else if (add_backref) { u64 local_index; int err; err = btrfs_del_inode_ref(trans, root, name, name_len, ino, parent_ino, &local_index); + if (err) + btrrfs_abort_transaction(trans, err); } return ret; } Ie. each error has its own transaction abort that are not merged to one point. I believe I said this during the previous reviews.
