[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

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

[Christoph Hellwig]

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

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 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 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...

[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 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 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 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.