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

[PATCH] 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index a6405d44d4ca..c2bd5c34a826 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)
+		s = alloc_super(fc->fs_type, fc->sb_flags, user_ns);
 retry:
 	spin_lock(&sb_lock);
 	if (test) {
-- 
2.27.0

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

[Christian Brauner]

On Fri, Jan 21, 2022 at 01:52:55PM -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 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/super.c b/fs/super.c
> index a6405d44d4ca..c2bd5c34a826 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)
> +		s = alloc_super(fc->fs_type, fc->sb_flags, user_ns);

Shouldn't we treat this allocation failure as "fatal" right away and not
bother taking locks, walking lists and so on? Seems strange to treat it
as fatal below but not here.

(The code-flow in here has always been a bit challenging to follow imho.
So not super keen to see more special-cases in there. Curious: do you
see any noticeable performance impact from that lock being taken and
dropped for the !test case?)

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

[Waiman Long]

On 1/24/22 06:37, Christian Brauner wrote:
> On Fri, Jan 21, 2022 at 01:52:55PM -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 | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/super.c b/fs/super.c
>> index a6405d44d4ca..c2bd5c34a826 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)
>> +		s = alloc_super(fc->fs_type, fc->sb_flags, user_ns);
> Shouldn't we treat this allocation failure as "fatal" right away and not
> bother taking locks, walking lists and so on? Seems strange to treat it
> as fatal below but not here.
I didn't add the null check because it was a rare case and the check is 
done later on anyway. I do agree that it may look a bit odd. Perhaps I 
should rearrange the code flow as suggested.
>
> (The code-flow in here has always been a bit challenging to follow imho.
> So not super keen to see more special-cases in there. Curious: do you
> see any noticeable performance impact from that lock being taken and
> dropped for the !test case?)

I don't believe there is noticeable performance impact with the !test 
case. The test case, however, can have some noticeable impact if the 
superblock list is long. I am wondering if we just always preallocate 
superblock with the risk that it may get unused and freed later on.

Cheers,
Longman

>