From bddca171ee22000d88f79be5d5f13dbd02c8d62a Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Thu, 10 Mar 2022 14:52:51 -0800 Subject: [PATCH] Call iput outside cluster locked transactions The final iput of an inode can delete items in cluster locked transactions. It was never safe to call iput within locked transactions but we never saw the problem. Recent work on inode deletion raised the issue again. This makes sure that we always perform iput outside of locked transactions. The only interesting change is making scoutfs_new_inode() return the allocated inode on error so that the caller can put the inode after releasing the transaction. Signed-off-by: Zach Brown --- kmod/src/dir.c | 25 ++++++++++++++----------- kmod/src/inode.c | 23 +++++++++++++---------- kmod/src/inode.h | 5 ++--- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/kmod/src/dir.c b/kmod/src/dir.c index a4b6dc09..00734909 100644 --- a/kmod/src/dir.c +++ b/kmod/src/dir.c @@ -720,7 +720,7 @@ static struct inode *lock_hold_create(struct inode *dir, struct dentry *dentry, struct list_head *ind_locks) { struct super_block *sb = dir->i_sb; - struct inode *inode; + struct inode *inode = NULL; u64 ind_seq; int ret = 0; u64 ino; @@ -765,11 +765,9 @@ retry: if (ret) goto out_unlock; - inode = scoutfs_new_inode(sb, dir, mode, rdev, ino, *inode_lock); - if (IS_ERR(inode)) { - ret = PTR_ERR(inode); + ret = scoutfs_new_inode(sb, dir, mode, rdev, ino, *inode_lock, &inode); + if (ret < 0) goto out; - } ret = scoutfs_dirty_inode_item(dir, *dir_lock); out: @@ -787,6 +785,8 @@ out_unlock: *orph_lock = NULL; } + if (!IS_ERR_OR_NULL(inode)) + iput(inode); inode = ERR_PTR(ret); } @@ -1319,11 +1319,11 @@ static int scoutfs_symlink(struct inode *dir, struct dentry *dentry, insert_inode_hash(inode); /* XXX need to set i_op/fop before here for sec callbacks */ d_instantiate(dentry, inode); + inode = NULL; + ret = 0; out: if (ret < 0) { /* XXX remove inode items */ - if (!IS_ERR_OR_NULL(inode)) - iput(inode); symlink_item_ops(sb, SYM_DELETE, scoutfs_ino(inode), inode_lock, NULL, name_len); @@ -1334,6 +1334,9 @@ out: scoutfs_unlock(sb, dir_lock, SCOUTFS_LOCK_WRITE); scoutfs_unlock(sb, inode_lock, SCOUTFS_LOCK_WRITE); + if (!IS_ERR_OR_NULL(inode)) + iput(inode); + return ret; } @@ -1923,10 +1926,8 @@ static int scoutfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mod si = SCOUTFS_I(inode); ret = scoutfs_inode_orphan_create(sb, scoutfs_ino(inode), orph_lock); - if (ret < 0) { - iput(inode); + if (ret < 0) goto out; /* XXX returning error but items created */ - } inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME; si->crtime = inode->i_mtime; @@ -1939,7 +1940,6 @@ static int scoutfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mod scoutfs_update_inode_item(inode, inode_lock, &ind_locks); scoutfs_update_inode_item(dir, dir_lock, &ind_locks); scoutfs_inode_index_unlock(sb, &ind_locks); - iput(inode); out: scoutfs_release_trans(sb); @@ -1948,6 +1948,9 @@ out: scoutfs_unlock(sb, inode_lock, SCOUTFS_LOCK_WRITE); scoutfs_unlock(sb, orph_lock, SCOUTFS_LOCK_WRITE_ONLY); + if (!IS_ERR_OR_NULL(inode)) + iput(inode); + return ret; } diff --git a/kmod/src/inode.c b/kmod/src/inode.c index 737c8a6c..9d22f52a 100644 --- a/kmod/src/inode.c +++ b/kmod/src/inode.c @@ -1411,10 +1411,14 @@ out: /* * Allocate and initialize a new inode. The caller is responsible for * creating links to it and updating it. @dir can be null. + * + * This is called with locks and a transaction because it creates the + * inode item. We can't call iput on the new inode on error. We + * return the inode to the caller *including on error* for them to put + * once they've released the transaction. */ -struct inode *scoutfs_new_inode(struct super_block *sb, struct inode *dir, - umode_t mode, dev_t rdev, u64 ino, - struct scoutfs_lock *lock) +int scoutfs_new_inode(struct super_block *sb, struct inode *dir, umode_t mode, dev_t rdev, + u64 ino, struct scoutfs_lock *lock, struct inode **inode_ret) { struct scoutfs_inode_info *si; struct scoutfs_key key; @@ -1423,8 +1427,10 @@ struct inode *scoutfs_new_inode(struct super_block *sb, struct inode *dir, int ret; inode = new_inode(sb); - if (!inode) - return ERR_PTR(-ENOMEM); + if (!inode) { + ret = -ENOMEM; + goto out; + } si = SCOUTFS_I(inode); si->ino = ino; @@ -1460,12 +1466,9 @@ struct inode *scoutfs_new_inode(struct super_block *sb, struct inode *dir, if (ret < 0) scoutfs_omap_clear(sb, ino); out: - if (ret) { - iput(inode); - inode = ERR_PTR(ret); - } + *inode_ret = inode; - return inode; + return ret; } static void init_orphan_key(struct scoutfs_key *key, u64 ino) diff --git a/kmod/src/inode.h b/kmod/src/inode.h index 4e650007..88058117 100644 --- a/kmod/src/inode.h +++ b/kmod/src/inode.h @@ -106,9 +106,8 @@ void scoutfs_update_inode_item(struct inode *inode, struct scoutfs_lock *lock, struct list_head *ind_locks); int scoutfs_alloc_ino(struct super_block *sb, bool is_dir, u64 *ino_ret); -struct inode *scoutfs_new_inode(struct super_block *sb, struct inode *dir, - umode_t mode, dev_t rdev, u64 ino, - struct scoutfs_lock *lock); +int scoutfs_new_inode(struct super_block *sb, struct inode *dir, umode_t mode, dev_t rdev, + u64 ino, struct scoutfs_lock *lock, struct inode **inode_ret); void scoutfs_inode_set_meta_seq(struct inode *inode); void scoutfs_inode_set_data_seq(struct inode *inode);