[PATCH v2] vfs: Pre-allocate superblock in sget_fc() if !test

[PATCH v2] vfs: Pre-allocate superblock in sget_fc() if !test

[Waiman Long]

When the test function is not defined in sget_fc(), we always need
to allocate a new superblock. So there is no point in acquiring the
sb_lock twice in this case. Optimize the !test case by pre-allocating
the superblock first before acquring the lock.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 fs/super.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index a6405d44d4ca..6601267f6fe0 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -520,6 +520,8 @@ struct super_block *sget_fc(struct fs_context *fc,
 	struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns;
 	int err;
 
+	if (!test)
+		goto prealloc;
 retry:
 	spin_lock(&sb_lock);
 	if (test) {
@@ -530,6 +532,7 @@ struct super_block *sget_fc(struct fs_context *fc,
 	}
 	if (!s) {
 		spin_unlock(&sb_lock);
+prealloc:
 		s = alloc_super(fc->fs_type, fc->sb_flags, user_ns);
 		if (!s)
 			return ERR_PTR(-ENOMEM);
-- 
2.27.0

Re: [PATCH v2] vfs: Pre-allocate superblock in sget_fc() if !test

[Christian Brauner]

On Mon, Jan 24, 2022 at 11:10:06AM -0500, Waiman Long wrote:
> When the test function is not defined in sget_fc(), we always need
> to allocate a new superblock. So there is no point in acquiring the
> sb_lock twice in this case. Optimize the !test case by pre-allocating
> the superblock first before acquring the lock.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  fs/super.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/super.c b/fs/super.c
> index a6405d44d4ca..6601267f6fe0 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -520,6 +520,8 @@ struct super_block *sget_fc(struct fs_context *fc,
>  	struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns;
>  	int err;
>  
> +	if (!test)
> +		goto prealloc;

I mean, now its another goto in there that adds to the confusion.
sget_fc() could _probably_ benefit if it were split into
__sget_test_fc() and __sget_independent_fc() with both ending up being
called from sget_fc() dending on whether or not test is NULL ofc.

This way the test and non-test cases would stop being mangled together
possibly making the logic easier to follow. Would probably also require
to move the common initialization part into a helper callable from both
__sget_independent_fc() and __sget_test_fc() sm like idk:

static struct super_block *__finish_init_super(........)
{
	s->s_fs_info = fc->s_fs_info;
	err = set(s, fc);
	if (err) {
		s->s_fs_info = NULL;
		spin_unlock(&sb_lock);
		destroy_unused_super(s);
		return ERR_PTR(err);
	}
	fc->s_fs_info = NULL;
	s->s_type = fc->fs_type;
	s->s_iflags |= fc->s_iflags;
	strlcpy(s->s_id, s->s_type->name, sizeof(s->s_id));
	list_add_tail(&s->s_list, &super_blocks);
	hlist_add_head(&s->s_instances, &s->s_type->fs_supers);
	spin_unlock(&sb_lock);
	get_filesystem(s->s_type);
	register_shrinker_prepared(&s->s_shrink);
	return s;
}

Re: [PATCH v2] vfs: Pre-allocate superblock in sget_fc() if !test

[Al Viro]

On Mon, Jan 24, 2022 at 11:10:06AM -0500, Waiman Long wrote:
> When the test function is not defined in sget_fc(), we always need
> to allocate a new superblock. So there is no point in acquiring the
> sb_lock twice in this case. Optimize the !test case by pre-allocating
> the superblock first before acquring the lock.

Umm...  Do you really see the contention on that sucker?  Because if you
do, I'd expect sget_fc() to be the least of your problems.

TBH, I'd be very surprised if that had caused measurable improvement in
any setup.  Seriously, take a look at alloc_super().  If spin_lock() +
spin_unlock() + branch is noticable among all that...  And it's not
as if you were not going to dirty that cacheline shortly afterwards
anyway, so cacheline pingpong is not an issue either...

I'm not saying that the patch is broken, I'm just wondering if it's worth
bothering with.