Messages in this thread |  | | Subject | Re: [V2] btrfs: drop inode reference count on error path | From | Nikolay Borisov <> | Date | Thu, 18 Apr 2019 15:50:00 +0300 |
| |
On 18.04.19 г. 14:06 ч., Pan Bian wrote: > The reference count of inode is incremented by ihold. It should be > dropped if not used. However, the reference count is not dropped if > error occurs during updating the inode or deleting orphan items. This > patch fixes the bug. > > Signed-off-by: Pan Bian <bianpan2016@163.com> > --- > V2: move ihold just before device_initialize to make code clearer > --- > fs/btrfs/inode.c | 54 +++++++++++++++++++++++++----------------------------- > 1 file changed, 25 insertions(+), 29 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 82fdda8..d6630df 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir, > struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); > u64 index; > int err; > - int drop_inode = 0; > + int log_mode; > > /* do not allow sys_link's with other subvols of the same device */ > if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid) > @@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir, > err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode), > 1, index); > > - if (err) { > - drop_inode = 1; > - } else { > - struct dentry *parent = dentry->d_parent; > - int ret; > + if (err) > + goto err_link; > > - err = btrfs_update_inode(trans, root, inode); > + err = btrfs_update_inode(trans, root, inode); > + if (err) > + goto err_link; > + if (inode->i_nlink == 1) { > + /* > + * If new hard link count is 1, it's a file created > + * with open(2) O_TMPFILE flag. > + */ > + err = btrfs_orphan_del(trans, BTRFS_I(inode)); > if (err) > - goto fail; > - if (inode->i_nlink == 1) { > - /* > - * If new hard link count is 1, it's a file created > - * with open(2) O_TMPFILE flag. > - */ > - err = btrfs_orphan_del(trans, BTRFS_I(inode)); > - if (err) > - goto fail; > - } > - BTRFS_I(inode)->last_link_trans = trans->transid; > - d_instantiate(dentry, inode); > - ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent, > - true, NULL); > - if (ret == BTRFS_NEED_TRANS_COMMIT) { > - err = btrfs_commit_transaction(trans); > - trans = NULL; > - } > + goto err_link; > + } > + BTRFS_I(inode)->last_link_trans = trans->transid; > + ihold(inode); > + d_instantiate(dentry, inode); > + log_mode = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, > + dentry->d_parent, true, NULL); > + if (log_mode == BTRFS_NEED_TRANS_COMMIT) { > + err = btrfs_commit_transaction(trans); > + trans = NULL; > } > > +err_link: > + if (err) > + inode_dec_link_count(inode);
Any particular reason why you moved this before ending the transaction? It potentially has an effect during tyransaction commit since doing inode_dec_link_count does mark_inode_dirty which moves the inode on the dirty list. Have you explicitly thought about that ? Note, I'm not saying it's wrong but I want the rationale for the code move.
> fail: > if (trans) > btrfs_end_transaction(trans); > - if (drop_inode) { > - inode_dec_link_count(inode); > - iput(inode); > - } > btrfs_btree_balance_dirty(fs_info); > return err; > } >
|  |