All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: linux-fsdevel@vger.kernel.org
Cc: ceph-devel@vger.kernel.org, idryomov@gmail.com,
	xiubli@redhat.com, Al Viro <viro@zeniv.linux.org.uk>,
	Dave Chinner <dchinner@redhat.com>
Subject: [PATCH v2] fs: change test in inode_insert5 for adding to the sb list
Date: Wed, 11 May 2022 12:53:39 -0400	[thread overview]
Message-ID: <20220511165339.85614-1-jlayton@kernel.org> (raw)

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@zeniv.linux.org.uk>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 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);
-- 
2.35.3


             reply	other threads:[~2022-05-11 16:53 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-11 16:53 Jeff Layton [this message]
2022-05-12  6:21 ` Christoph Hellwig
2022-05-12 14:21 ` Dave Chinner
2022-05-12 15:51   ` Jeff Layton
2022-06-27 11:15 ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220511165339.85614-1-jlayton@kernel.org \
    --to=jlayton@kernel.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=dchinner@redhat.com \
    --cc=idryomov@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=xiubli@redhat.com \
    --subject='Re: [PATCH v2] fs: change test in inode_insert5 for adding to the sb list' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.