[PATCH 0/2] super: small tweaks

[PATCH 0/2] super: small tweaks

[Christian Brauner]

Hey,

Just two minor hopefully uncontroversial simplifications.

Thanks!
Christian


---
base-commit: afde134b5bd02a5c719336ca1d0d3cb7e4def70e
change-id: 20231118-vfs-super-massage-wait-6ad126a1f315

[PATCH 1/2] super: massage wait event mechanism

[Christian Brauner]

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c | 51 ++++++++++++++-------------------------------------
 1 file changed, 14 insertions(+), 37 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index aa4e5c11ee32..f3227b22879d 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -81,16 +81,13 @@ static inline void super_unlock_shared(struct super_block *sb)
 	super_unlock(sb, false);
 }
 
-static inline bool wait_born(struct super_block *sb)
+static bool super_load_flags(const struct super_block *sb, unsigned int flags)
 {
-	unsigned int flags;
-
 	/*
 	 * Pairs with smp_store_release() in super_wake() and ensures
-	 * that we see SB_BORN or SB_DYING after we're woken.
+	 * that we see @flags after we're woken.
 	 */
-	flags = smp_load_acquire(&sb->s_flags);
-	return flags & (SB_BORN | SB_DYING);
+	return smp_load_acquire(&sb->s_flags) & flags;
 }
 
 /**
@@ -111,10 +108,15 @@ static inline bool wait_born(struct super_block *sb)
  */
 static __must_check bool super_lock(struct super_block *sb, bool excl)
 {
-
 	lockdep_assert_not_held(&sb->s_umount);
 
-relock:
+	/* wait until the superblock is ready or dying */
+	wait_var_event(&sb->s_flags, super_load_flags(sb, SB_BORN | SB_DYING));
+
+	/* Don't pointlessly acquire s_umount. */
+	if (super_load_flags(sb, SB_DYING))
+		return false;
+
 	__super_lock(sb, excl);
 
 	/*
@@ -127,20 +129,8 @@ static __must_check bool super_lock(struct super_block *sb, bool excl)
 		return false;
 	}
 
-	/* Has called ->get_tree() successfully. */
-	if (sb->s_flags & SB_BORN)
-		return true;
-
-	super_unlock(sb, excl);
-
-	/* wait until the superblock is ready or dying */
-	wait_var_event(&sb->s_flags, wait_born(sb));
-
-	/*
-	 * Neither SB_BORN nor SB_DYING are ever unset so we never loop.
-	 * Just reacquire @sb->s_umount for the caller.
-	 */
-	goto relock;
+	WARN_ON_ONCE(!(sb->s_flags & SB_BORN));
+	return true;
 }
 
 /* wait and try to acquire read-side of @sb->s_umount */
@@ -523,18 +513,6 @@ void deactivate_super(struct super_block *s)
 
 EXPORT_SYMBOL(deactivate_super);
 
-static inline bool wait_dead(struct super_block *sb)
-{
-	unsigned int flags;
-
-	/*
-	 * Pairs with memory barrier in super_wake() and ensures
-	 * that we see SB_DEAD after we're woken.
-	 */
-	flags = smp_load_acquire(&sb->s_flags);
-	return flags & SB_DEAD;
-}
-
 /**
  * grab_super - acquire an active reference to a superblock
  * @sb: superblock to acquire
@@ -561,7 +539,7 @@ static bool grab_super(struct super_block *sb)
 		}
 		super_unlock_excl(sb);
 	}
-	wait_var_event(&sb->s_flags, wait_dead(sb));
+	wait_var_event(&sb->s_flags, super_load_flags(sb, SB_DEAD));
 	put_super(sb);
 	return false;
 }
@@ -908,8 +886,7 @@ static void __iterate_supers(void (*f)(struct super_block *))
 
 	spin_lock(&sb_lock);
 	list_for_each_entry(sb, &super_blocks, s_list) {
-		/* Pairs with memory marrier in super_wake(). */
-		if (smp_load_acquire(&sb->s_flags) & SB_DYING)
+		if (super_load_flags(sb, SB_DYING))
 			continue;
 		sb->s_count++;
 		spin_unlock(&sb_lock);

-- 
2.42.0

[PATCH 2/2] super: don't bother with WARN_ON_ONCE()

[Christian Brauner]

We hold our own active reference and we've checked it above.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index f3227b22879d..844ca13e9d93 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -2067,10 +2067,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
 	super_unlock_excl(sb);
 	sb_wait_write(sb, SB_FREEZE_WRITE);
-	if (!super_lock_excl(sb)) {
-		WARN_ON_ONCE("Dying superblock while freezing!");
-		return -EINVAL;
-	}
+	__super_lock_excl(sb);
 
 	/* Now we go and block page faults... */
 	sb->s_writers.frozen = SB_FREEZE_PAGEFAULT;

-- 
2.42.0

Re: [PATCH 1/2] super: massage wait event mechanism

[Christoph Hellwig]

Can you explain why you're "massaging" things here?

Re: [PATCH 2/2] super: don't bother with WARN_ON_ONCE()

[Christoph Hellwig]

On Mon, Nov 27, 2023 at 12:51:31PM +0100, Christian Brauner wrote:
> diff --git a/fs/super.c b/fs/super.c
> index f3227b22879d..844ca13e9d93 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -2067,10 +2067,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
>  	super_unlock_excl(sb);
>  	sb_wait_write(sb, SB_FREEZE_WRITE);
> -	if (!super_lock_excl(sb)) {
> -		WARN_ON_ONCE("Dying superblock while freezing!");
> -		return -EINVAL;
> -	}
> +	__super_lock_excl(sb);

This looks ok, but I still find these locking helper so horrible to
follow..

Re: [PATCH 1/2] super: massage wait event mechanism

[Christian Brauner]

On Mon, Nov 27, 2023 at 02:59:00PM +0100, Christoph Hellwig wrote:
> Can you explain why you're "massaging" things here?

Ah, I didn't update my commit message before sending out:

"We're currently using two separate helpers wait_born() and wait_dead()
when we can just all do it in a single helper super_load_flags(). We're
also acquiring the lock before we check whether this superblock is even
a viable candidate. If it's already dying we don't even need to bother
with the lock."

Is that alright?

Re: [PATCH 2/2] super: don't bother with WARN_ON_ONCE()

[Christian Brauner]

On Mon, Nov 27, 2023 at 02:59:45PM +0100, Christoph Hellwig wrote:
> This looks ok, but I still find these locking helper so horrible to
> follow..

What do you still find objectionable?

Re: [PATCH 1/2] super: massage wait event mechanism

[Christoph Hellwig]

On Mon, Nov 27, 2023 at 03:52:56PM +0100, Christian Brauner wrote:
> On Mon, Nov 27, 2023 at 02:59:00PM +0100, Christoph Hellwig wrote:
> > Can you explain why you're "massaging" things here?
> 
> Ah, I didn't update my commit message before sending out:
> 
> "We're currently using two separate helpers wait_born() and wait_dead()
> when we can just all do it in a single helper super_load_flags(). We're
> also acquiring the lock before we check whether this superblock is even
> a viable candidate. If it's already dying we don't even need to bother
> with the lock."
> 
> Is that alright?

Sounds good, but now I need to go back and cross-reference it with
what actuall is in the patch :)

Re: [PATCH 1/2] super: massage wait event mechanism

[Christoph Hellwig]

On Mon, Nov 27, 2023 at 12:51:30PM +0100, Christian Brauner wrote:
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/super.c | 51 ++++++++++++++-------------------------------------
>  1 file changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index aa4e5c11ee32..f3227b22879d 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -81,16 +81,13 @@ static inline void super_unlock_shared(struct super_block *sb)
>  	super_unlock(sb, false);
>  }
>  
> +static bool super_load_flags(const struct super_block *sb, unsigned int flags)
>  {
>  	/*
>  	 * Pairs with smp_store_release() in super_wake() and ensures
> +	 * that we see @flags after we're woken.
>  	 */
> +	return smp_load_acquire(&sb->s_flags) & flags;

I find the name for this helper very confusing.  Yes, eventually it
is clear that the load maps to a load instruction with acquire semantics
here, but it's a really unusual naming I think.  Unfortunately I can't
offer a better one either.

Otherwise this looks good except for the fact that I really hate
code using smp_load_acquire / smp_store_release directly because of
all the mental load it causes.

Re: [PATCH 2/2] super: don't bother with WARN_ON_ONCE()

[Christoph Hellwig]

On Mon, Nov 27, 2023 at 03:53:36PM +0100, Christian Brauner wrote:
> On Mon, Nov 27, 2023 at 02:59:45PM +0100, Christoph Hellwig wrote:
> > This looks ok, but I still find these locking helper so horrible to
> > follow..
> 
> What do you still find objectionable?

Same thing as last time.  The __ helpers that take the share/exclusive
trip me off every single time I have to follow them.   Just open coding
the calls to the rw_semaphore helpers is a lot easier to read in
general, but for anything complex that actually needs an enum with
EXCL and SHARED in it would at least makes it clear what is happening.

Re: [PATCH 1/2] super: massage wait event mechanism

[Christian Brauner]

On Mon, Nov 27, 2023 at 05:46:37PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 27, 2023 at 12:51:30PM +0100, Christian Brauner wrote:
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/super.c | 51 ++++++++++++++-------------------------------------
> >  1 file changed, 14 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index aa4e5c11ee32..f3227b22879d 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -81,16 +81,13 @@ static inline void super_unlock_shared(struct super_block *sb)
> >  	super_unlock(sb, false);
> >  }
> >  
> > +static bool super_load_flags(const struct super_block *sb, unsigned int flags)
> >  {
> >  	/*
> >  	 * Pairs with smp_store_release() in super_wake() and ensures
> > +	 * that we see @flags after we're woken.
> >  	 */
> > +	return smp_load_acquire(&sb->s_flags) & flags;
> 
> I find the name for this helper very confusing.  Yes, eventually it
> is clear that the load maps to a load instruction with acquire semantics
> here, but it's a really unusual naming I think.  Unfortunately I can't
> offer a better one either.

I'll just drop the load from the middle then.

> 
> Otherwise this looks good except for the fact that I really hate
> code using smp_load_acquire / smp_store_release directly because of
> all the mental load it causes.

Hm, it's pretty common in our code in so many places...