On Wed, 2022-05-11 at 12:53 -0400, Jeff Layton wrote:
> The inode_insert5 currently looks at I_CREATING to decide whether to
> insert the inode into the sb list. This test is a bit ambiguous though
> as I_CREATING state is not directly related to that list.
>
> This test is also problematic for some upcoming ceph changes to add
> fscrypt support. We need to be able to allocate an inode using new_inode
> and insert it into the hash later if we end up using it, and doing that
> now means that we double add it and corrupt the list.
>
> What we really want to know in this test is whether the inode is already
> in its superblock list, and then add it if it isn't. Have it test for
> list_empty instead and ensure that we always initialize the list by
> doing it in inode_init_once. It's only ever removed from the list with
> list_del_init, so that should be sufficient.
>
> There doesn't seem to be any need to hold the inode_hash_lock for this
> operation either, so drop that before adding to to the list.
>
> Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> ---
> fs/inode.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> A small revision to the patch that I sent as part of the ceph+fscrypt
> series. I didn't see any need to hold the inode_hash_lock when adding
> the inode to the sb list, so do that outside the lock. I also revised
> the comment to be more clear.
>
> Al, I'm planning to merge this via the ceph tree since I have other
> patches that depend on it. Let me know if you'd rather take this via
> your tree instead.
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..9d429247a4f0 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -422,6 +422,7 @@ void inode_init_once(struct inode *inode)
> INIT_LIST_HEAD(&inode->i_io_list);
> INIT_LIST_HEAD(&inode->i_wb_list);
> INIT_LIST_HEAD(&inode->i_lru);
> + INIT_LIST_HEAD(&inode->i_sb_list);
> __address_space_init_once(&inode->i_data);
> i_size_ordered_init(inode);
> }
> @@ -1021,7 +1022,6 @@ struct inode *new_inode_pseudo(struct super_block *sb)
> spin_lock(&inode->i_lock);
> inode->i_state = 0;
> spin_unlock(&inode->i_lock);
> - INIT_LIST_HEAD(&inode->i_sb_list);
> }
> return inode;
> }
> @@ -1165,7 +1165,6 @@ struct inode *inode_insert5(struct inode *inode, unsigned long hashval,
> {
> struct hlist_head *head = inode_hashtable + hash(inode->i_sb, hashval);
> struct inode *old;
> - bool creating = inode->i_state & I_CREATING;
>
> again:
> spin_lock(&inode_hash_lock);
> @@ -1199,11 +1198,17 @@ struct inode *inode_insert5(struct inode *inode, unsigned long hashval,
> inode->i_state |= I_NEW;
> hlist_add_head_rcu(&inode->i_hash, head);
> spin_unlock(&inode->i_lock);
> - if (!creating)
> - inode_sb_list_add(inode);
> unlock:
> spin_unlock(&inode_hash_lock);
>
> + /*
> + * Add it to the sb list if it's not already. If there is an inode,
> + * then it has I_NEW at this point, so it should be safe to test
> + * i_sb_list locklessly.
> + */
> + if (inode && list_empty(&inode->i_sb_list))
> + inode_sb_list_add(inode);
> +
> return inode;
> }
> EXPORT_SYMBOL(inode_insert5);
Hi Al,
Could I get your Acked-by or R-b on this patch? I'd like to take this in
via the ceph tree, and I'd prefer an explicit ack on this if possible.
Thanks!
--
Jeff Layton <jlayton@xxxxxxxxxx>
