| Message ID | 20220121185255.27601-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs: Pre-allocate superblock in sget_fc() if !test | expand |
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?)
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 >
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) {
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(+)