diff mbox series

[2/3] fs: wait for partially frozen filesystems

Message ID 168688011838.860947.2073512011056060112.stgit@frogsfrogsfrogs (mailing list archive)
State New, archived
Headers show
Series fs: kernel and userspace filesystem freeze | expand

Commit Message

Darrick J. Wong June 16, 2023, 1:48 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Jan Kara suggested that when one thread is in the middle of freezing a
filesystem, another thread trying to freeze the same fs but with a
different freeze_holder should wait until the freezer reaches either end
state (UNFROZEN or COMPLETE) instead of returning EBUSY immediately.

Neither caller can do anything sensible with this race other than retry
but they cannot really distinguish EBUSY as in "someone other holder of
the same type has the sb already frozen" from "freezing raced with
holder of a different type".

Plumb in the extra coded needed to wait for the fs freezer to reach an
end state and try the freeze again.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/super.c |   34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

Comments

Dave Chinner June 16, 2023, 2:19 a.m. UTC | #1 
On Thu, Jun 15, 2023 at 06:48:38PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Jan Kara suggested that when one thread is in the middle of freezing a
> filesystem, another thread trying to freeze the same fs but with a
> different freeze_holder should wait until the freezer reaches either end
> state (UNFROZEN or COMPLETE) instead of returning EBUSY immediately.
> 
> Neither caller can do anything sensible with this race other than retry
> but they cannot really distinguish EBUSY as in "someone other holder of
> the same type has the sb already frozen" from "freezing raced with
> holder of a different type".
> 
> Plumb in the extra coded needed to wait for the fs freezer to reach an
> end state and try the freeze again.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/super.c |   34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)

Simple enough. I was going to comment about replacing wait_unfrozen
with a variant on wait_for_partially_frozen(), but then I looked at
the next patch....

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Christoph Hellwig June 16, 2023, 5:52 a.m. UTC | #2 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Jan Kara June 16, 2023, 1:24 p.m. UTC | #3 
On Thu 15-06-23 18:48:38, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Jan Kara suggested that when one thread is in the middle of freezing a
> filesystem, another thread trying to freeze the same fs but with a
> different freeze_holder should wait until the freezer reaches either end
> state (UNFROZEN or COMPLETE) instead of returning EBUSY immediately.
> 
> Neither caller can do anything sensible with this race other than retry
> but they cannot really distinguish EBUSY as in "someone other holder of
						  ^^^ some

> the same type has the sb already frozen" from "freezing raced with
> holder of a different type".
> 
> Plumb in the extra coded needed to wait for the fs freezer to reach an
		     ^^^ code

> end state and try the freeze again.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Besides these typo fixes the patch looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c |   34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/super.c b/fs/super.c
> index 81fb67157cba..1b8ea81788d4 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1635,6 +1635,24 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
>  		percpu_up_write(sb->s_writers.rw_sem + level);
>  }
>  
> +static int wait_for_partially_frozen(struct super_block *sb)
> +{
> +	int ret = 0;
> +
> +	do {
> +		unsigned short old = sb->s_writers.frozen;
> +
> +		up_write(&sb->s_umount);
> +		ret = wait_var_event_killable(&sb->s_writers.frozen,
> +					       sb->s_writers.frozen != old);
> +		down_write(&sb->s_umount);
> +	} while (ret == 0 &&
> +		 sb->s_writers.frozen != SB_UNFROZEN &&
> +		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);
> +
> +	return ret;
> +}
> +
>  /**
>   * freeze_super - lock the filesystem and force it into a consistent state
>   * @sb: the super to lock
> @@ -1686,6 +1704,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  	atomic_inc(&sb->s_active);
>  	down_write(&sb->s_umount);
>  
> +retry:
>  	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
>  		if (sb->s_writers.freeze_holders & who) {
>  			deactivate_locked_super(sb);
> @@ -1704,8 +1723,13 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  	}
>  
>  	if (sb->s_writers.frozen != SB_UNFROZEN) {
> -		deactivate_locked_super(sb);
> -		return -EBUSY;
> +		ret = wait_for_partially_frozen(sb);
> +		if (ret) {
> +			deactivate_locked_super(sb);
> +			return ret;
> +		}
> +
> +		goto retry;
>  	}
>  
>  	if (!(sb->s_flags & SB_BORN)) {
> @@ -1717,6 +1741,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  		/* Nothing to do really... */
>  		sb->s_writers.freeze_holders |= who;
>  		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> +		wake_up_var(&sb->s_writers.frozen);
>  		up_write(&sb->s_umount);
>  		return 0;
>  	}
> @@ -1737,6 +1762,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  		sb->s_writers.frozen = SB_UNFROZEN;
>  		sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
>  		wake_up(&sb->s_writers.wait_unfrozen);
> +		wake_up_var(&sb->s_writers.frozen);
>  		deactivate_locked_super(sb);
>  		return ret;
>  	}
> @@ -1753,6 +1779,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  			sb->s_writers.frozen = SB_UNFROZEN;
>  			sb_freeze_unlock(sb, SB_FREEZE_FS);
>  			wake_up(&sb->s_writers.wait_unfrozen);
> +			wake_up_var(&sb->s_writers.frozen);
>  			deactivate_locked_super(sb);
>  			return ret;
>  		}
> @@ -1763,6 +1790,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  	 */
>  	sb->s_writers.freeze_holders |= who;
>  	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> +	wake_up_var(&sb->s_writers.frozen);
>  	lockdep_sb_freeze_release(sb);
>  	up_write(&sb->s_umount);
>  	return 0;
> @@ -1803,6 +1831,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
>  	if (sb_rdonly(sb)) {
>  		sb->s_writers.freeze_holders &= ~who;
>  		sb->s_writers.frozen = SB_UNFROZEN;
> +		wake_up_var(&sb->s_writers.frozen);
>  		goto out;
>  	}
>  
> @@ -1821,6 +1850,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
>  
>  	sb->s_writers.freeze_holders &= ~who;
>  	sb->s_writers.frozen = SB_UNFROZEN;
> +	wake_up_var(&sb->s_writers.frozen);
>  	sb_freeze_unlock(sb, SB_FREEZE_FS);
>  out:
>  	wake_up(&sb->s_writers.wait_unfrozen);
>
diff mbox series

Patch

diff --git a/fs/super.c b/fs/super.c
index 81fb67157cba..1b8ea81788d4 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1635,6 +1635,24 @@  static void sb_freeze_unlock(struct super_block *sb, int level)
 		percpu_up_write(sb->s_writers.rw_sem + level);
 }
 
+static int wait_for_partially_frozen(struct super_block *sb)
+{
+	int ret = 0;
+
+	do {
+		unsigned short old = sb->s_writers.frozen;
+
+		up_write(&sb->s_umount);
+		ret = wait_var_event_killable(&sb->s_writers.frozen,
+					       sb->s_writers.frozen != old);
+		down_write(&sb->s_umount);
+	} while (ret == 0 &&
+		 sb->s_writers.frozen != SB_UNFROZEN &&
+		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);
+
+	return ret;
+}
+
 /**
  * freeze_super - lock the filesystem and force it into a consistent state
  * @sb: the super to lock
@@ -1686,6 +1704,7 @@  int freeze_super(struct super_block *sb, enum freeze_holder who)
 	atomic_inc(&sb->s_active);
 	down_write(&sb->s_umount);
 
+retry:
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
 		if (sb->s_writers.freeze_holders & who) {
 			deactivate_locked_super(sb);
@@ -1704,8 +1723,13 @@  int freeze_super(struct super_block *sb, enum freeze_holder who)
 	}
 
 	if (sb->s_writers.frozen != SB_UNFROZEN) {
-		deactivate_locked_super(sb);
-		return -EBUSY;
+		ret = wait_for_partially_frozen(sb);
+		if (ret) {
+			deactivate_locked_super(sb);
+			return ret;
+		}
+
+		goto retry;
 	}
 
 	if (!(sb->s_flags & SB_BORN)) {
@@ -1717,6 +1741,7 @@  int freeze_super(struct super_block *sb, enum freeze_holder who)
 		/* Nothing to do really... */
 		sb->s_writers.freeze_holders |= who;
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+		wake_up_var(&sb->s_writers.frozen);
 		up_write(&sb->s_umount);
 		return 0;
 	}
@@ -1737,6 +1762,7 @@  int freeze_super(struct super_block *sb, enum freeze_holder who)
 		sb->s_writers.frozen = SB_UNFROZEN;
 		sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
 		wake_up(&sb->s_writers.wait_unfrozen);
+		wake_up_var(&sb->s_writers.frozen);
 		deactivate_locked_super(sb);
 		return ret;
 	}
@@ -1753,6 +1779,7 @@  int freeze_super(struct super_block *sb, enum freeze_holder who)
 			sb->s_writers.frozen = SB_UNFROZEN;
 			sb_freeze_unlock(sb, SB_FREEZE_FS);
 			wake_up(&sb->s_writers.wait_unfrozen);
+			wake_up_var(&sb->s_writers.frozen);
 			deactivate_locked_super(sb);
 			return ret;
 		}
@@ -1763,6 +1790,7 @@  int freeze_super(struct super_block *sb, enum freeze_holder who)
 	 */
 	sb->s_writers.freeze_holders |= who;
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+	wake_up_var(&sb->s_writers.frozen);
 	lockdep_sb_freeze_release(sb);
 	up_write(&sb->s_umount);
 	return 0;
@@ -1803,6 +1831,7 @@  static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 	if (sb_rdonly(sb)) {
 		sb->s_writers.freeze_holders &= ~who;
 		sb->s_writers.frozen = SB_UNFROZEN;
+		wake_up_var(&sb->s_writers.frozen);
 		goto out;
 	}
 
@@ -1821,6 +1850,7 @@  static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 
 	sb->s_writers.freeze_holders &= ~who;
 	sb->s_writers.frozen = SB_UNFROZEN;
+	wake_up_var(&sb->s_writers.frozen);
 	sb_freeze_unlock(sb, SB_FREEZE_FS);
 out:
 	wake_up(&sb->s_writers.wait_unfrozen);