[BUG REPORT] next-20231102: generic/311 fails on XFS with external log

[BUG REPORT] next-20231102: generic/311 fails on XFS with external log

[Chandan Babu R]

Hi,

generic/311 consistently fails when executing on a kernel built from
next-20231102.

The following is the fstests config file that was used during testing.

export FSTYP=xfs

export TEST_DEV=/dev/loop0
export TEST_DIR=/mnt/test
export TEST_LOGDEV=/dev/loop2

export SCRATCH_DEV=/dev/loop1
export SCRATCH_MNT=/mnt/scratch
export SCRATCH_LOGDEV=/dev/loop3

export USE_EXTERNAL=yes

export MKFS_OPTIONS="-f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g"


The following is the contents obtained from 311.out.bad.

QA output created by 311
Running test 1 buffered, normal suspend
Random seed is 1
ee6103415276cde95544b11b2675f132
device-mapper: suspend ioctl on flakey-logtest  failed: Device or resource busy
Command failed.
failed to suspend flakey-logtest


Git bisect revealed the following to be the first bad commit,

abcb2b94cce4fb7a8f84278e8da4d726837439d1
Author:     Christian Brauner <brauner@kernel.org>
AuthorDate: Wed Sep 27 15:21:16 2023 +0200
Commit:     Christian Brauner <brauner@kernel.org>
CommitDate: Sat Oct 28 13:29:24 2023 +0200

bdev: implement freeze and thaw holder operations

The old method of implementing block device freeze and thaw operations
required us to rely on get_active_super() to walk the list of all
superblocks on the system to find any superblock that might use the
block device. This is wasteful and not very pleasant overall.

Now that we can finally go straight from block device to owning
superblock things become way simpler.

Link: https://lore.kernel.org/r/20231024-vfs-super-freeze-v2-5-599c19f4faac@kernel.org
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>

--
Chandan

Re: [BUG REPORT] next-20231102: generic/311 fails on XFS with external log

[Christian Brauner]

On Thu, Nov 02, 2023 at 06:06:10PM +0530, Chandan Babu R wrote:
> Hi,
> 
> generic/311 consistently fails when executing on a kernel built from
> next-20231102.
> 
> The following is the fstests config file that was used during testing.
> 
> export FSTYP=xfs
> 
> export TEST_DEV=/dev/loop0
> export TEST_DIR=/mnt/test
> export TEST_LOGDEV=/dev/loop2
> 
> export SCRATCH_DEV=/dev/loop1
> export SCRATCH_MNT=/mnt/scratch
> export SCRATCH_LOGDEV=/dev/loop3

Thanks for the report. So dm flakey sets up:

/dev/dm-0 over /dev/loop0
/dev/dm-1 over /dev/loop2

and then we mount an xfs filesystem with:

/dev/loop2 as logdev and /dev/loop0 as the main device.

So on current kernels what happens is that if you freeze the main
device you end up:

bdev_freeze(dm-0)
-> get_super(dm-0) # finds xfs sb
   -> freeze_super(sb)

if you also freeze the log device afterwards via:

bdev_freeze(dm-1)
-> get_super(dm-1) # doesn't find xfs sb because freezing only works for
                   # main device

What's currently in -next allows you to roughly do the following:

bdev_freeze(dm-0)
-> fs_bdev_freeze(dm-0->sb)
   -> freeze_super(dm-0->sb) # returns 0

bdev_freeze(dm-1)
-> fs_bdev_freeze(dm-1->sb)
   -> freeze_super(dm-1->sb) # returns -EBUSY

So you'll see EBUSY because the superblock was already frozen when the
main block device was frozen. I was somewhat expecting that we may run
into such issues.

I think we just need to figure out what we want to do in cases the
superblock is frozen via multiple devices. It would probably be correct
to keep it frozen as long as any of the devices is frozen?

Re: [BUG REPORT] next-20231102: generic/311 fails on XFS with external log

[Dave Chinner]

On Thu, Nov 02, 2023 at 03:54:48PM +0100, Christian Brauner wrote:
> On Thu, Nov 02, 2023 at 06:06:10PM +0530, Chandan Babu R wrote:
> > Hi,
> > 
> > generic/311 consistently fails when executing on a kernel built from
> > next-20231102.
> > 
> > The following is the fstests config file that was used during testing.
> > 
> > export FSTYP=xfs
> > 
> > export TEST_DEV=/dev/loop0
> > export TEST_DIR=/mnt/test
> > export TEST_LOGDEV=/dev/loop2
> > 
> > export SCRATCH_DEV=/dev/loop1
> > export SCRATCH_MNT=/mnt/scratch
> > export SCRATCH_LOGDEV=/dev/loop3
> 
> Thanks for the report. So dm flakey sets up:
> 
> /dev/dm-0 over /dev/loop0
> /dev/dm-1 over /dev/loop2
> 
> and then we mount an xfs filesystem with:
> 
> /dev/loop2 as logdev and /dev/loop0 as the main device.
> 
> So on current kernels what happens is that if you freeze the main
> device you end up:
> 
> bdev_freeze(dm-0)
> -> get_super(dm-0) # finds xfs sb
>    -> freeze_super(sb)
> 
> if you also freeze the log device afterwards via:
> 
> bdev_freeze(dm-1)
> -> get_super(dm-1) # doesn't find xfs sb because freezing only works for
>                    # main device
> 
> What's currently in -next allows you to roughly do the following:
> 
> bdev_freeze(dm-0)
> -> fs_bdev_freeze(dm-0->sb)
>    -> freeze_super(dm-0->sb) # returns 0
> 
> bdev_freeze(dm-1)
> -> fs_bdev_freeze(dm-1->sb)
>    -> freeze_super(dm-1->sb) # returns -EBUSY
> 
> So you'll see EBUSY because the superblock was already frozen when the
> main block device was frozen. I was somewhat expecting that we may run
> into such issues.
> 
> I think we just need to figure out what we want to do in cases the
> superblock is frozen via multiple devices. It would probably be correct
> to keep it frozen as long as any of the devices is frozen?

So this series removed the blockdev freeze nesting code that dm
suspend/resume functionality used (i.e. it allowed concurrent bdev
freeze/thaw works and leaves the bdev frozen until the last thaw
occurs). Removing bd_fsfreeze_count essentially removed this nesting
ability.

IMO, bdev_freeze() should still nest freeze/thaw across all devices
in the filesystem like it used to on the main device. This implies
that freeze_super() needs to know that it is being called from
bdev_freeze() and needs a freeze counter to allow concurrent bdev
freezes and only thaw the fs when the last freeze goes away....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [BUG REPORT] next-20231102: generic/311 fails on XFS with external log

[Christoph Hellwig]

On Thu, Nov 02, 2023 at 03:54:48PM +0100, Christian Brauner wrote:
> So you'll see EBUSY because the superblock was already frozen when the
> main block device was frozen. I was somewhat expecting that we may run
> into such issues.
> 
> I think we just need to figure out what we want to do in cases the
> superblock is frozen via multiple devices. It would probably be correct
> to keep it frozen as long as any of the devices is frozen?

As dave pointed out I think we need to bring back / keep the freeze
count.

Re: [BUG REPORT] next-20231102: generic/311 fails on XFS with external log

[Christian Brauner]

On Fri, Nov 03, 2023 at 09:14:05AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 02, 2023 at 03:54:48PM +0100, Christian Brauner wrote:
> > So you'll see EBUSY because the superblock was already frozen when the
> > main block device was frozen. I was somewhat expecting that we may run
> > into such issues.
> > 
> > I think we just need to figure out what we want to do in cases the
> > superblock is frozen via multiple devices. It would probably be correct
> > to keep it frozen as long as any of the devices is frozen?
> 
> As dave pointed out I think we need to bring back / keep the freeze
> count.

The freeze count never want away. IOW, for each block device we still
have bd_fsfreeze_count otherwise we couldn't nest per-block device. What
we need is a freeze counter in sb_writers so we can nest superblock
freezes. IOW, we need to count the number of block devices that
requested/caused the superblock to be frozen. I think we're all in
agreement though. All of our suggestions should be the same.
I'm currently testing:

From c1849037227e5801f0b5e8acfa05aa5d90f4c9e4 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Fri, 3 Nov 2023 08:38:49 +0100
Subject: [PATCH] [DRAFT] fs: handle freezing from multiple devices

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 44 +++++++++++++++++++++++++++++++++++++++-----
 include/linux/fs.h |  2 ++
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 176c55abd9de..882c79366c70 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1476,9 +1476,11 @@ static int fs_bdev_freeze(struct block_device *bdev)
 		return -EINVAL;
 
 	if (sb->s_op->freeze_super)
-		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = sb->s_op->freeze_super(sb,
+				FREEZE_HOLDER_BLOCK | FREEZE_HOLDER_USERSPACE);
 	else
-		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = freeze_super(sb,
+				FREEZE_HOLDER_BLOCK | FREEZE_HOLDER_USERSPACE);
 	if (!error)
 		error = sync_blockdev(bdev);
 	deactivate_super(sb);
@@ -1497,9 +1499,11 @@ static int fs_bdev_thaw(struct block_device *bdev)
 		return -EINVAL;
 
 	if (sb->s_op->thaw_super)
-		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = sb->s_op->thaw_super(sb,
+				FREEZE_HOLDER_BLOCK | FREEZE_HOLDER_USERSPACE);
 	else
-		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = thaw_super(sb,
+				FREEZE_HOLDER_BLOCK | FREEZE_HOLDER_USERSPACE);
 	deactivate_super(sb);
 	return error;
 }
@@ -1923,6 +1927,7 @@ static int wait_for_partially_frozen(struct super_block *sb)
  * @who should be:
  * * %FREEZE_HOLDER_USERSPACE if userspace wants to freeze the fs;
  * * %FREEZE_HOLDER_KERNEL if the kernel wants to freeze the fs.
+ * * %FREEZE_HOLDER_BLOCK if freeze originated from the block layer.
  *
  * The @who argument distinguishes between the kernel and userspace trying to
  * freeze the filesystem.  Although there cannot be multiple kernel freezes or
@@ -1958,18 +1963,33 @@ static int wait_for_partially_frozen(struct super_block *sb)
 int freeze_super(struct super_block *sb, enum freeze_holder who)
 {
 	int ret;
+	bool bdev_initiated;
 
 	if (!super_lock_excl(sb)) {
 		WARN_ON_ONCE("Dying superblock while freezing!");
 		return -EINVAL;
 	}
 	atomic_inc(&sb->s_active);
+	bdev_initiated = (who & FREEZE_HOLDER_BLOCK);
+	who &= ~FREEZE_HOLDER_BLOCK;
 
 retry:
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
+		ret = -EBUSY;
+
+		/*
+		 * This is a freeze request from another block device
+		 * associated with the same superblock.
+		 */
+		if (bdev_initiated) {
+			sb->s_writers.bdev_count++;
+			pr_info("Freeze initiated from %d block devices\n", sb->s_writers.bdev_count);
+			ret = 0;
+		}
+
 		if (sb->s_writers.freeze_holders & who) {
 			deactivate_locked_super(sb);
-			return -EBUSY;
+			return ret;
 		}
 
 		WARN_ON(sb->s_writers.freeze_holders == 0);
@@ -2002,6 +2022,8 @@ 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;
+		if (bdev_initiated)
+			sb->s_writers.bdev_count++;
 		wake_up_var(&sb->s_writers.frozen);
 		super_unlock_excl(sb);
 		return 0;
@@ -2052,6 +2074,8 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	 */
 	sb->s_writers.freeze_holders |= who;
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+	if (bdev_initiated)
+		sb->s_writers.bdev_count++;
 	wake_up_var(&sb->s_writers.frozen);
 	lockdep_sb_freeze_release(sb);
 	super_unlock_excl(sb);
@@ -2068,12 +2092,22 @@ EXPORT_SYMBOL(freeze_super);
 static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 {
 	int error = -EINVAL;
+	bool bdev_initiated = (who & FREEZE_HOLDER_BLOCK);
+	who &= ~FREEZE_HOLDER_BLOCK;
 
 	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
 		goto out_unlock;
 	if (!(sb->s_writers.freeze_holders & who))
 		goto out_unlock;
 
+	if (bdev_initiated)
+		sb->s_writers.bdev_count--;
+	if (sb->s_writers.bdev_count) {
+		pr_info("Filesystems held frozen by %d block devices\n", sb->s_writers.bdev_count);
+		error = 0;
+		goto out_unlock;
+	}
+
 	/*
 	 * Freeze is shared with someone else.  Release our hold and drop the
 	 * active ref that freeze_super assigned to the freezer.
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 63ff88d20e46..edc9c071c199 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1186,6 +1186,7 @@ enum {
 struct sb_writers {
 	unsigned short			frozen;		/* Is sb frozen? */
 	unsigned short			freeze_holders;	/* Who froze fs? */
+	int				bdev_count;
 	struct percpu_rw_semaphore	rw_sem[SB_FREEZE_LEVELS];
 };
 
@@ -2054,6 +2055,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 enum freeze_holder {
 	FREEZE_HOLDER_KERNEL	= (1U << 0),
 	FREEZE_HOLDER_USERSPACE	= (1U << 1),
+	FREEZE_HOLDER_BLOCK	= (1U << 2),
 };
 
 struct super_operations {
-- 
2.34.1

[PATCH] fs: handle freezing from multiple devices

[Christian Brauner]

Before [1] freezing a filesystems through the block layer only worked
for the main block device as the owning superblock of additional block
devices could not be found. Any filesystem that made use of multiple
block devices would only be freezable via it's main block device.

For example, consider xfs over device mapper with /dev/dm-0 as main
block device and /dev/dm-1 as external log device. Two freeze requests
before [1]:

(1) dmsetup suspend /dev/dm-0 on the main block device

    bdev_freeze(dm-0)
    -> dm-0->bd_fsfreeze_count++
    -> freeze_super(xfs-sb)

    The owning superblock is found and the filesystem gets frozen.
    Returns 0.

(2) dmsetup suspend /dev/dm-1 on the log device

    bdev_freeze(dm-1)
    -> dm-1->bd_fsfreeze_count++

    The owning superblock isn't found and only the block device freeze
    count is incremented. Returns 0.

Two freeze requests after [1]:

(1') dmsetup suspend /dev/dm-0 on the main block device

    bdev_freeze(dm-0)
    -> dm-0->bd_fsfreeze_count++
    -> freeze_super(xfs-sb)

    The owning superblock is found and the filesystem gets frozen.
    Returns 0.

(2') dmsetup suspend /dev/dm-1 on the log device

    bdev_freeze(dm-0)
    -> dm-0->bd_fsfreeze_count++
    -> freeze_super(xfs-sb)

    The owning superblock is found and the filesystem gets frozen.
    Returns -EBUSY.

When (2') is called we initiate a freeze from another block device of
the same superblock. So we increment the bd_fsfreeze_count for that
additional block device. But we now also find the owning superblock for
additional block devices and call freeze_super() again which reports
-EBUSY.

This can be reproduced through xfstests via:

    mkfs.xfs -f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g,logdev=/dev/nvme1n1p4 /dev/nvme1n1p3
    mkfs.xfs -f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g,logdev=/dev/nvme1n1p6 /dev/nvme1n1p5

    FSTYP=xfs
    export TEST_DEV=/dev/nvme1n1p3
    export TEST_DIR=/mnt/test
    export TEST_LOGDEV=/dev/nvme1n1p4
    export SCRATCH_DEV=/dev/nvme1n1p5
    export SCRATCH_MNT=/mnt/scratch
    export SCRATCH_LOGDEV=/dev/nvme1n1p6
    export USE_EXTERNAL=yes

    sudo ./check generic/311

Fix this by counting the number of block devices that requested the
filesystem to be frozen in @bdev_count in struct sb_writers and only
unfreeze once the @bdev_count hits zero. Survives fstests and blktests
and makes the reproducer succeed.

Reported-by: Chandan Babu R <chandanbabu@kernel.org>
Link: https://lore.kernel.org/linux-block/87bkccnwxc.fsf@debian-BULLSEYE-live-builder-AMD64
Fixes: [1]: bfac4176f2c4 ("bdev: implement freeze and thaw holder operations") # no backport needed
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/fs.h |  2 ++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 176c55abd9de..c51eb669ed0b 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1476,9 +1476,11 @@ static int fs_bdev_freeze(struct block_device *bdev)
 		return -EINVAL;
 
 	if (sb->s_op->freeze_super)
-		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = sb->s_op->freeze_super(sb,
+				FREEZE_HOLDER_BDEV | FREEZE_HOLDER_USERSPACE);
 	else
-		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = freeze_super(sb,
+				FREEZE_HOLDER_BDEV | FREEZE_HOLDER_USERSPACE);
 	if (!error)
 		error = sync_blockdev(bdev);
 	deactivate_super(sb);
@@ -1497,9 +1499,11 @@ static int fs_bdev_thaw(struct block_device *bdev)
 		return -EINVAL;
 
 	if (sb->s_op->thaw_super)
-		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = sb->s_op->thaw_super(sb,
+				FREEZE_HOLDER_BDEV | FREEZE_HOLDER_USERSPACE);
 	else
-		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = thaw_super(sb,
+				FREEZE_HOLDER_BDEV | FREEZE_HOLDER_USERSPACE);
 	deactivate_super(sb);
 	return error;
 }
@@ -1923,6 +1927,7 @@ static int wait_for_partially_frozen(struct super_block *sb)
  * @who should be:
  * * %FREEZE_HOLDER_USERSPACE if userspace wants to freeze the fs;
  * * %FREEZE_HOLDER_KERNEL if the kernel wants to freeze the fs.
+ * * %FREEZE_HOLDER_BDEV if freeze originated from a block device.
  *
  * The @who argument distinguishes between the kernel and userspace trying to
  * freeze the filesystem.  Although there cannot be multiple kernel freezes or
@@ -1930,6 +1935,12 @@ static int wait_for_partially_frozen(struct super_block *sb)
  * userspace can both hold a filesystem frozen.  The filesystem remains frozen
  * until there are no kernel or userspace freezes in effect.
  *
+ * A filesystem may hold multiple devices and thus a filesystems may be
+ * frozen through the block layer via multiple block devices. In this
+ * case the request is as originating from the block layer by raising
+ * %FREEZE_HOLDER_BDEV. We count the number of block devices that
+ * requested a freeze in @bdev_count.
+ *
  * During this function, sb->s_writers.frozen goes through these values:
  *
  * SB_UNFROZEN: File system is normal, all writes progress as usual.
@@ -1958,18 +1969,29 @@ static int wait_for_partially_frozen(struct super_block *sb)
 int freeze_super(struct super_block *sb, enum freeze_holder who)
 {
 	int ret;
+	bool bdev_initiated;
 
 	if (!super_lock_excl(sb)) {
 		WARN_ON_ONCE("Dying superblock while freezing!");
 		return -EINVAL;
 	}
 	atomic_inc(&sb->s_active);
+	bdev_initiated = (who & FREEZE_HOLDER_BDEV);
+	who &= ~FREEZE_HOLDER_BDEV;
 
 retry:
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
+		ret = -EBUSY;
+
+		if (bdev_initiated) {
+			sb->s_writers.bdev_count++;
+			pr_info("VFS: Freeze initiated from %d block devices\n", sb->s_writers.bdev_count);
+			ret = 0;
+		}
+
 		if (sb->s_writers.freeze_holders & who) {
 			deactivate_locked_super(sb);
-			return -EBUSY;
+			return ret;
 		}
 
 		WARN_ON(sb->s_writers.freeze_holders == 0);
@@ -2002,6 +2024,8 @@ 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;
+		if (bdev_initiated)
+			sb->s_writers.bdev_count++;
 		wake_up_var(&sb->s_writers.frozen);
 		super_unlock_excl(sb);
 		return 0;
@@ -2052,6 +2076,8 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	 */
 	sb->s_writers.freeze_holders |= who;
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+	if (bdev_initiated)
+		sb->s_writers.bdev_count++;
 	wake_up_var(&sb->s_writers.frozen);
 	lockdep_sb_freeze_release(sb);
 	super_unlock_excl(sb);
@@ -2068,12 +2094,22 @@ EXPORT_SYMBOL(freeze_super);
 static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 {
 	int error = -EINVAL;
+	bool bdev_initiated = (who & FREEZE_HOLDER_BDEV);
+	who &= ~FREEZE_HOLDER_BDEV;
 
 	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
 		goto out_unlock;
 	if (!(sb->s_writers.freeze_holders & who))
 		goto out_unlock;
 
+	if (bdev_initiated)
+		sb->s_writers.bdev_count--;
+	if (sb->s_writers.bdev_count) {
+		pr_info("VFS: Filesystems held frozen by %d block devices\n", sb->s_writers.bdev_count);
+		error = 0;
+		goto out_unlock;
+	}
+
 	/*
 	 * Freeze is shared with someone else.  Release our hold and drop the
 	 * active ref that freeze_super assigned to the freezer.
@@ -2098,6 +2134,8 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 		if (error) {
 			printk(KERN_ERR "VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
+			if (bdev_initiated)
+				sb->s_writers.bdev_count++;
 			goto out_unlock;
 		}
 	}
@@ -2126,6 +2164,13 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
  * @who should be:
  * * %FREEZE_HOLDER_USERSPACE if userspace wants to thaw the fs;
  * * %FREEZE_HOLDER_KERNEL if the kernel wants to thaw the fs.
+ * * %FREEZE_HOLDER_BDEV if freeze originated from a block device.
+ *
+ * A filesystem may hold multiple devices and thus a filesystems may
+ * have been frozen through the block layer via multiple block devices.
+ * In this case the number of block devices that requested the
+ * filesystem to be frozen is stored in @bdev_count. We only unfreeze if
+ * @bdev_count is zero
  */
 int thaw_super(struct super_block *sb, enum freeze_holder who)
 {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 63ff88d20e46..dda7942e1f3e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1186,6 +1186,7 @@ enum {
 struct sb_writers {
 	unsigned short			frozen;		/* Is sb frozen? */
 	unsigned short			freeze_holders;	/* Who froze fs? */
+	int				bdev_count;	/* How many devices froze it? */
 	struct percpu_rw_semaphore	rw_sem[SB_FREEZE_LEVELS];
 };
 
@@ -2054,6 +2055,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 enum freeze_holder {
 	FREEZE_HOLDER_KERNEL	= (1U << 0),
 	FREEZE_HOLDER_USERSPACE	= (1U << 1),
+	FREEZE_HOLDER_BDEV	= (1U << 2),
 };
 
 struct super_operations {

---
base-commit: c6a4738de282fc95752e1f1c5573ab7b4020b55e
change-id: 20231103-vfs-multi-device-freeze-506e2c010473

Re: [PATCH] fs: handle freezing from multiple devices

[Christoph Hellwig]

On Fri, Nov 03, 2023 at 02:52:27PM +0100, Christian Brauner wrote:
> Fix this by counting the number of block devices that requested the
> filesystem to be frozen in @bdev_count in struct sb_writers and only
> unfreeze once the @bdev_count hits zero. Survives fstests and blktests
> and makes the reproducer succeed.

Is there a good reason to not just refcount the freezes in general?

Re: [PATCH] fs: handle freezing from multiple devices

[Christian Brauner]

On Fri, Nov 03, 2023 at 03:19:40PM +0100, Christoph Hellwig wrote:
> On Fri, Nov 03, 2023 at 02:52:27PM +0100, Christian Brauner wrote:
> > Fix this by counting the number of block devices that requested the
> > filesystem to be frozen in @bdev_count in struct sb_writers and only
> > unfreeze once the @bdev_count hits zero. Survives fstests and blktests
> > and makes the reproducer succeed.
> 
> Is there a good reason to not just refcount the freezes in general?

If we start counting freezes in general we break userspace as
freeze_super() is called from ioctl_fsfreeze() and that expects to
return EBUSY on an already frozen filesystem. xfs scrub might be another
user that might break if we change that.

Re: [PATCH] fs: handle freezing from multiple devices

[Jan Kara]

On Fri 03-11-23 16:10:10, Christian Brauner wrote:
> On Fri, Nov 03, 2023 at 03:19:40PM +0100, Christoph Hellwig wrote:
> > On Fri, Nov 03, 2023 at 02:52:27PM +0100, Christian Brauner wrote:
> > > Fix this by counting the number of block devices that requested the
> > > filesystem to be frozen in @bdev_count in struct sb_writers and only
> > > unfreeze once the @bdev_count hits zero. Survives fstests and blktests
> > > and makes the reproducer succeed.
> > 
> > Is there a good reason to not just refcount the freezes in general?
> 
> If we start counting freezes in general we break userspace as
> freeze_super() is called from ioctl_fsfreeze() and that expects to
> return EBUSY on an already frozen filesystem. xfs scrub might be another
> user that might break if we change that.

I guess Christoph meant that we'd count all the sb freezes into the
refcount (what is now bdev_count) but without HOLDER_BDEV flag we will
return EBUSY if the refcount is > 0 instead of incrementing it.

There would be a subtle behavioral difference that now if you freeze the fs
with ioctl_fsfreeze() and then try to freeze through the blockdev, you'll
get EBUSY while with the new method the bdev freeze will succeed but I
don't think that can do any harm. It even kind of makes more sense.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: handle freezing from multiple devices

[Christian Brauner]

On Fri, Nov 03, 2023 at 04:43:52PM +0100, Jan Kara wrote:
> On Fri 03-11-23 16:10:10, Christian Brauner wrote:
> > On Fri, Nov 03, 2023 at 03:19:40PM +0100, Christoph Hellwig wrote:
> > > On Fri, Nov 03, 2023 at 02:52:27PM +0100, Christian Brauner wrote:
> > > > Fix this by counting the number of block devices that requested the
> > > > filesystem to be frozen in @bdev_count in struct sb_writers and only
> > > > unfreeze once the @bdev_count hits zero. Survives fstests and blktests
> > > > and makes the reproducer succeed.
> > > 
> > > Is there a good reason to not just refcount the freezes in general?
> > 
> > If we start counting freezes in general we break userspace as
> > freeze_super() is called from ioctl_fsfreeze() and that expects to
> > return EBUSY on an already frozen filesystem. xfs scrub might be another
> > user that might break if we change that.
> 
> I guess Christoph meant that we'd count all the sb freezes into the
> refcount (what is now bdev_count) but without HOLDER_BDEV flag we will

Ah, sorry I didn't get that from the message.

> return EBUSY if the refcount is > 0 instead of incrementing it.

Yeah, that should work. I'm playing with this rn.

> There would be a subtle behavioral difference that now if you freeze the fs
> with ioctl_fsfreeze() and then try to freeze through the blockdev, you'll
> get EBUSY while with the new method the bdev freeze will succeed but I
> don't think that can do any harm. It even kind of makes more sense.

Yeah, though afaict that change is independent of whether we count all
freeze_super() calls or only such with HOLDER_BDEV. I've had the same
reaction as you.

[PATCH v2 0/2] Handle multi device freezing

[Christian Brauner]

Hey everyone,

Now that we can find the owning filesystem of any device if the
superblock and fs_holder_ops are used we need to handle multi-device
filesystem freezes. This series does that. Details in the main commit.

Thanks!
Christian

---
Christian Brauner (2):
      fs: remove dead check
      fs: handle freezing from multiple devices

 fs/super.c         | 140 +++++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |  17 ++++++-
 2 files changed, 124 insertions(+), 33 deletions(-)
---
base-commit: c6a4738de282fc95752e1f1c5573ab7b4020b55e
change-id: 20231103-vfs-multi-device-freeze-506e2c010473

[PATCH v2 1/2] fs: remove dead check

[Christian Brauner]

Above we call super_lock_excl() which waits until the superblock is
SB_BORN and since SB_BORN is never unset once set this check can never
fire. Plus, we also hold an active reference at this point already so
this superblock can't even be shutdown.

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

diff --git a/fs/super.c b/fs/super.c
index 176c55abd9de..2d32e60daef7 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1993,11 +1993,6 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 		goto retry;
 	}
 
-	if (!(sb->s_flags & SB_BORN)) {
-		super_unlock_excl(sb);
-		return 0;	/* sic - it's "nothing to do" */
-	}
-
 	if (sb_rdonly(sb)) {
 		/* Nothing to do really... */
 		sb->s_writers.freeze_holders |= who;

-- 
2.34.1

[PATCH v2 2/2] fs: handle freezing from multiple devices

[Christian Brauner]

Before [1] freezing a filesystems through the block layer only worked
for the main block device as the owning superblock of additional block
devices could not be found. Any filesystem that made use of multiple
block devices would only be freezable via it's main block device.

For example, consider xfs over device mapper with /dev/dm-0 as main
block device and /dev/dm-1 as external log device. Two freeze requests
before [1]:

(1) dmsetup suspend /dev/dm-0 on the main block device

    bdev_freeze(dm-0)
    -> dm-0->bd_fsfreeze_count++
    -> freeze_super(xfs-sb)

    The owning superblock is found and the filesystem gets frozen.
    Returns 0.

(2) dmsetup suspend /dev/dm-1 on the log device

    bdev_freeze(dm-1)
    -> dm-1->bd_fsfreeze_count++

    The owning superblock isn't found and only the block device freeze
    count is incremented. Returns 0.

Two freeze requests after [1]:

(1') dmsetup suspend /dev/dm-0 on the main block device

    bdev_freeze(dm-0)
    -> dm-0->bd_fsfreeze_count++
    -> freeze_super(xfs-sb)

    The owning superblock is found and the filesystem gets frozen.
    Returns 0.

(2') dmsetup suspend /dev/dm-1 on the log device

    bdev_freeze(dm-0)
    -> dm-0->bd_fsfreeze_count++
    -> freeze_super(xfs-sb)

    The owning superblock is found and the filesystem gets frozen.
    Returns -EBUSY.

When (2') is called we initiate a freeze from another block device of
the same superblock. So we increment the bd_fsfreeze_count for that
additional block device. But we now also find the owning superblock for
additional block devices and call freeze_super() again which reports
-EBUSY.

This can be reproduced through xfstests via:

    mkfs.xfs -f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g,logdev=/dev/nvme1n1p4 /dev/nvme1n1p3
    mkfs.xfs -f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g,logdev=/dev/nvme1n1p6 /dev/nvme1n1p5

    FSTYP=xfs
    export TEST_DEV=/dev/nvme1n1p3
    export TEST_DIR=/mnt/test
    export TEST_LOGDEV=/dev/nvme1n1p4
    export SCRATCH_DEV=/dev/nvme1n1p5
    export SCRATCH_MNT=/mnt/scratch
    export SCRATCH_LOGDEV=/dev/nvme1n1p6
    export USE_EXTERNAL=yes

    sudo ./check generic/311

Current semantics allow two concurrent freezers: one initiated from
userspace via FREEZE_HOLDER_USERSPACE and one initiated from the kernel
via FREEZE_HOLDER_KERNEL. If there are multiple concurrent freeze
requests from either FREEZE_HOLDER_USERSPACE or FREEZE_HOLDER_KERNEL
-EBUSY is returned.

We need to preserve these semantics because as they are uapi via
FIFREEZE and FITHAW ioctl()s. IOW, freezes don't nest for FIFREEZE and
FITHAW. Other kernels consumers rely on non-nesting freezes as well.

With freezes initiated from the block layer freezes need to nest if the
same superblock is frozen via multiple devices. So we need to start
counting the number of freeze requests.

If FREEZE_HOLDER_BDEV is passed alongside FREEZE_HOLDER_KERNEL or
FREEZE_HOLDER_USERSPACE we allow the caller to nest freeze calls.

To accommodate the old semantics we split the freeze counter into two
counting kernel initiated and userspace initiated freezes separately. We
can then also stop recording FREEZE_HOLDER_* in struct sb_writers.

We also simplify freezing by making all concurrent freezers share a
single active superblock reference count instead of having separate
references for kernel and userspace. I don't see why we would need two
active reference counts. Neither FREEZE_HOLDER_KERNEL nor
FREEZE_HOLDER_USERSPACE can put the active reference as long as they are
concurrent freezers anwyay. That was already true before we allowed
nesting freezes.

Survives various fstests runs with different options including the
reproducer, online scrub, and online repair, fsfreze, and so on. Also
survives blktests.

Reported-by: Chandan Babu R <chandanbabu@kernel.org>
Link: https://lore.kernel.org/linux-block/87bkccnwxc.fsf@debian-BULLSEYE-live-builder-AMD64
Fixes: [1]: bfac4176f2c4 ("bdev: implement freeze and thaw holder operations") # no backport needed
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 135 ++++++++++++++++++++++++++++++++++++++++++-----------
 include/linux/fs.h |  17 ++++++-
 2 files changed, 124 insertions(+), 28 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 2d32e60daef7..44273c553569 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1464,6 +1464,21 @@ static struct super_block *get_bdev_super(struct block_device *bdev)
 	return sb;
 }
 
+/**
+ * fs_bdev_freeze - freeze owning filesystem of block device
+ * @bdev: block device
+ *
+ * Freeze the filesystem that owns this block device if it is still
+ * active.
+ *
+ * A filesystem that owns multiple block devices may be frozen from each
+ * block device and won't be unfrozen until all block devices are
+ * unfrozen. Each block device can only freeze the filesystem once as we
+ * nest freezes for block devices in the block layer.
+ *
+ * Return: If the freeze was successful zero is returned. If the freeze
+ *         failed a negative error codeis returned.
+ */
 static int fs_bdev_freeze(struct block_device *bdev)
 {
 	struct super_block *sb;
@@ -1476,15 +1491,34 @@ static int fs_bdev_freeze(struct block_device *bdev)
 		return -EINVAL;
 
 	if (sb->s_op->freeze_super)
-		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = sb->s_op->freeze_super(sb,
+				FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
 	else
-		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = freeze_super(sb,
+				FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
 	if (!error)
 		error = sync_blockdev(bdev);
 	deactivate_super(sb);
 	return error;
 }
 
+/**
+ * fs_bdev_thaw - thaw owning filesystem of block device
+ * @bdev: block device
+ *
+ * Thaw the filesystem that owns this block device.
+ *
+ * A filesystem that owns multiple block devices may be frozen from each
+ * block device and won't be unfrozen until all block devices are
+ * unfrozen. Each block device can only freeze the filesystem once as we
+ * nest freezes for block devices in the block layer.
+ *
+ * Return: If the thaw was successful zero is returned. If the thaw
+ *         failed a negative error code is returned. If this function
+ *         returns zero it doesn't mean that the filesystem is unfrozen
+ *         as it may have been frozen multiple times (kernel may hold a
+ *         freeze or might be frozen from other block devices).
+ */
 static int fs_bdev_thaw(struct block_device *bdev)
 {
 	struct super_block *sb;
@@ -1497,9 +1531,11 @@ static int fs_bdev_thaw(struct block_device *bdev)
 		return -EINVAL;
 
 	if (sb->s_op->thaw_super)
-		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = sb->s_op->thaw_super(sb,
+				FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
 	else
-		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+		error = thaw_super(sb,
+				FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
 	deactivate_super(sb);
 	return error;
 }
@@ -1911,6 +1947,47 @@ static int wait_for_partially_frozen(struct super_block *sb)
 	return ret;
 }
 
+#define FREEZE_HOLDERS (FREEZE_HOLDER_KERNEL | FREEZE_HOLDER_USERSPACE)
+#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST)
+
+static inline int freeze_inc(struct super_block *sb, enum freeze_holder who)
+{
+	WARN_ON_ONCE((who & ~FREEZE_FLAGS));
+	WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+
+	if (who & FREEZE_HOLDER_KERNEL)
+		++sb->s_writers.freeze_kcount;
+	if (who & FREEZE_HOLDER_USERSPACE)
+		++sb->s_writers.freeze_ucount;
+	return sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount;
+}
+
+static inline int freeze_dec(struct super_block *sb, enum freeze_holder who)
+{
+	WARN_ON_ONCE((who & ~FREEZE_FLAGS));
+	WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+
+	if ((who & FREEZE_HOLDER_KERNEL) && sb->s_writers.freeze_kcount)
+		--sb->s_writers.freeze_kcount;
+	if ((who & FREEZE_HOLDER_USERSPACE) && sb->s_writers.freeze_ucount)
+		--sb->s_writers.freeze_ucount;
+	return sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount;
+}
+
+static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
+{
+	WARN_ON_ONCE((who & ~FREEZE_FLAGS));
+	WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+
+	if (who & FREEZE_HOLDER_KERNEL)
+		return (who & FREEZE_MAY_NEST) ||
+		       sb->s_writers.freeze_kcount == 0;
+	if (who & FREEZE_HOLDER_USERSPACE)
+		return (who & FREEZE_MAY_NEST) ||
+		       sb->s_writers.freeze_ucount == 0;
+	return false;
+}
+
 /**
  * freeze_super - lock the filesystem and force it into a consistent state
  * @sb: the super to lock
@@ -1923,6 +2000,7 @@ static int wait_for_partially_frozen(struct super_block *sb)
  * @who should be:
  * * %FREEZE_HOLDER_USERSPACE if userspace wants to freeze the fs;
  * * %FREEZE_HOLDER_KERNEL if the kernel wants to freeze the fs.
+ * * %FREEZE_MAY_NEST whether nesting freeze and thaw requests is allowed.
  *
  * The @who argument distinguishes between the kernel and userspace trying to
  * freeze the filesystem.  Although there cannot be multiple kernel freezes or
@@ -1930,6 +2008,13 @@ static int wait_for_partially_frozen(struct super_block *sb)
  * userspace can both hold a filesystem frozen.  The filesystem remains frozen
  * until there are no kernel or userspace freezes in effect.
  *
+ * A filesystem may hold multiple devices and thus a filesystems may be
+ * frozen through the block layer via multiple block devices. In this
+ * case the request is marked as being allowed to nest passig
+ * FREEZE_MAY_NEST. The filesystem remains frozen until all block
+ * devices are unfrozen. If multiple freezes are attempted without
+ * FREEZE_MAY_NEST -EBUSY will be returned.
+ *
  * During this function, sb->s_writers.frozen goes through these values:
  *
  * SB_UNFROZEN: File system is normal, all writes progress as usual.
@@ -1967,19 +2052,16 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 
 retry:
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
-		if (sb->s_writers.freeze_holders & who) {
+		if (!may_freeze(sb, who)) {
 			deactivate_locked_super(sb);
 			return -EBUSY;
 		}
 
-		WARN_ON(sb->s_writers.freeze_holders == 0);
-
-		/*
-		 * Someone else already holds this type of freeze; share the
-		 * freeze and assign the active ref to the freeze.
-		 */
-		sb->s_writers.freeze_holders |= who;
-		super_unlock_excl(sb);
+		/* all freezers share a single active reference */
+		if (freeze_inc(sb, who) > 1)
+			deactivate_locked_super(sb);
+		else
+			super_unlock_excl(sb);
 		return 0;
 	}
 
@@ -1995,7 +2077,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 
 	if (sb_rdonly(sb)) {
 		/* Nothing to do really... */
-		sb->s_writers.freeze_holders |= who;
+		WARN_ON_ONCE(freeze_inc(sb, who) > 1);
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 		wake_up_var(&sb->s_writers.frozen);
 		super_unlock_excl(sb);
@@ -2045,7 +2127,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	 * For debugging purposes so that fs can warn if it sees write activity
 	 * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
 	 */
-	sb->s_writers.freeze_holders |= who;
+	WARN_ON_ONCE(freeze_inc(sb, who) > 1);
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 	wake_up_var(&sb->s_writers.frozen);
 	lockdep_sb_freeze_release(sb);
@@ -2066,21 +2148,15 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 
 	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
 		goto out_unlock;
-	if (!(sb->s_writers.freeze_holders & who))
-		goto out_unlock;
 
 	/*
-	 * Freeze is shared with someone else.  Release our hold and drop the
-	 * active ref that freeze_super assigned to the freezer.
+	 * All freezers share a single active reference.
+	 * So just unlock in case there are any left.
 	 */
-	error = 0;
-	if (sb->s_writers.freeze_holders & ~who) {
-		sb->s_writers.freeze_holders &= ~who;
-		goto out_deactivate;
-	}
+	if (freeze_dec(sb, who))
+		goto out_unlock;
 
 	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_deactivate;
@@ -2091,13 +2167,13 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 	if (sb->s_op->unfreeze_fs) {
 		error = sb->s_op->unfreeze_fs(sb);
 		if (error) {
-			printk(KERN_ERR "VFS:Filesystem thaw failed\n");
+			pr_err("VFS: Filesystem thaw failed\n");
+			freeze_inc(sb, who);
 			lockdep_sb_freeze_release(sb);
 			goto out_unlock;
 		}
 	}
 
-	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);
@@ -2121,6 +2197,11 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
  * @who should be:
  * * %FREEZE_HOLDER_USERSPACE if userspace wants to thaw the fs;
  * * %FREEZE_HOLDER_KERNEL if the kernel wants to thaw the fs.
+ * * %FREEZE_MAY_NEST whether nesting freeze and thaw requests is allowed
+ *
+ * A filesystem may hold multiple devices and thus a filesystems may
+ * have been frozen through the block layer via multiple block devices.
+ * The filesystem remains frozen until all block devices are unfrozen.
  */
 int thaw_super(struct super_block *sb, enum freeze_holder who)
 {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 63ff88d20e46..fef481b2b4bc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1185,7 +1185,8 @@ enum {
 
 struct sb_writers {
 	unsigned short			frozen;		/* Is sb frozen? */
-	unsigned short			freeze_holders;	/* Who froze fs? */
+	int				freeze_kcount;	/* How many kernel freeze requests? */
+	int				freeze_ucount;	/* How many userspace freeze requests? */
 	struct percpu_rw_semaphore	rw_sem[SB_FREEZE_LEVELS];
 };
 
@@ -2051,9 +2052,23 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 					struct file *dst_file, loff_t dst_pos,
 					loff_t len, unsigned int remap_flags);
 
+/**
+ * enum freeze_holder - holder of the freeze
+ * @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
+ * @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
+ * @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
+ *
+ * Indicate who the owner of the freeze or thaw request is and whether the
+ * freeze needs to be exclusive or can nest.
+ * Without @FREEZE_MAY_NEST, multiple freeze and thaw requests from the same
+ * holder aren't allowed. It is however allowed to hold a single
+ * @FREEZE_HOLDER_USERSPACE and a single @FREEZE_HOLDER_USERSPACE freeze at the
+ * same time. This is used by some filesystems during online repair or similar.
+ */
 enum freeze_holder {
 	FREEZE_HOLDER_KERNEL	= (1U << 0),
 	FREEZE_HOLDER_USERSPACE	= (1U << 1),
+	FREEZE_MAY_NEST		= (1U << 2),
 };
 
 struct super_operations {

-- 
2.34.1

Re: [PATCH] fs: handle freezing from multiple devices

[Christoph Hellwig]

On Fri, Nov 03, 2023 at 05:49:17PM +0100, Christian Brauner wrote:
> > > > Is there a good reason to not just refcount the freezes in general?
> > > 
> > > If we start counting freezes in general we break userspace as
> > > freeze_super() is called from ioctl_fsfreeze() and that expects to
> > > return EBUSY on an already frozen filesystem. xfs scrub might be another
> > > user that might break if we change that.
> > 
> > I guess Christoph meant that we'd count all the sb freezes into the
> > refcount (what is now bdev_count) but without HOLDER_BDEV flag we will
> 
> Ah, sorry I didn't get that from the message.

That's also not what I meant :)  Jan's suggestion makes a little
more sense than mine, which I think would have been a good idea
when we started from scratch, but does indeed feel dangerous now.

Re: [PATCH v2 1/2] fs: remove dead check

[Christoph Hellwig]

On Sat, Nov 04, 2023 at 03:00:12PM +0100, Christian Brauner wrote:
> Above we call super_lock_excl() which waits until the superblock is
> SB_BORN and since SB_BORN is never unset once set this check can never
> fire. Plus, we also hold an active reference at this point already so
> this superblock can't even be shutdown.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] fs: handle freezing from multiple devices

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 0/2] Handle multi device freezing

[Chandan Babu R]

On Sat, Nov 04, 2023 at 03:00:11 PM +0100, Christian Brauner wrote:
> Hey everyone,
>
> Now that we can find the owning filesystem of any device if the
> superblock and fs_holder_ops are used we need to handle multi-device
> filesystem freezes. This series does that. Details in the main commit.

generic/311, xfs/006 and xfs/264 pass when executed against a kernel which has
the patches applied. Hence,

Tested-by: Chandan Babu R <chandanbabu@kernel.org>

>
> Thanks!
> Christian
>
> ---
> Christian Brauner (2):
>       fs: remove dead check
>       fs: handle freezing from multiple devices
>
>  fs/super.c         | 140 +++++++++++++++++++++++++++++++++++++++++------------
>  include/linux/fs.h |  17 ++++++-
>  2 files changed, 124 insertions(+), 33 deletions(-)
> ---
> base-commit: c6a4738de282fc95752e1f1c5573ab7b4020b55e
> change-id: 20231103-vfs-multi-device-freeze-506e2c010473


-- 
Chandan

Re: [PATCH v2 1/2] fs: remove dead check

[Jan Kara]

On Sat 04-11-23 15:00:12, Christian Brauner wrote:
> Above we call super_lock_excl() which waits until the superblock is
> SB_BORN and since SB_BORN is never unset once set this check can never
> fire. Plus, we also hold an active reference at this point already so
> this superblock can't even be shutdown.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/2] fs: handle freezing from multiple devices

[Jan Kara]

On Sat 04-11-23 15:00:13, Christian Brauner wrote:
> Before [1] freezing a filesystems through the block layer only worked
> for the main block device as the owning superblock of additional block
> devices could not be found. Any filesystem that made use of multiple
> block devices would only be freezable via it's main block device.
> 
> For example, consider xfs over device mapper with /dev/dm-0 as main
> block device and /dev/dm-1 as external log device. Two freeze requests
> before [1]:
> 
> (1) dmsetup suspend /dev/dm-0 on the main block device
> 
>     bdev_freeze(dm-0)
>     -> dm-0->bd_fsfreeze_count++
>     -> freeze_super(xfs-sb)
> 
>     The owning superblock is found and the filesystem gets frozen.
>     Returns 0.
> 
> (2) dmsetup suspend /dev/dm-1 on the log device
> 
>     bdev_freeze(dm-1)
>     -> dm-1->bd_fsfreeze_count++
> 
>     The owning superblock isn't found and only the block device freeze
>     count is incremented. Returns 0.
> 
> Two freeze requests after [1]:
> 
> (1') dmsetup suspend /dev/dm-0 on the main block device
> 
>     bdev_freeze(dm-0)
>     -> dm-0->bd_fsfreeze_count++
>     -> freeze_super(xfs-sb)
> 
>     The owning superblock is found and the filesystem gets frozen.
>     Returns 0.
> 
> (2') dmsetup suspend /dev/dm-1 on the log device
> 
>     bdev_freeze(dm-0)
>     -> dm-0->bd_fsfreeze_count++
>     -> freeze_super(xfs-sb)
> 
>     The owning superblock is found and the filesystem gets frozen.
>     Returns -EBUSY.
> 
> When (2') is called we initiate a freeze from another block device of
> the same superblock. So we increment the bd_fsfreeze_count for that
> additional block device. But we now also find the owning superblock for
> additional block devices and call freeze_super() again which reports
> -EBUSY.
> 
> This can be reproduced through xfstests via:
> 
>     mkfs.xfs -f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g,logdev=/dev/nvme1n1p4 /dev/nvme1n1p3
>     mkfs.xfs -f -m crc=1,reflink=1,rmapbt=1, -i sparse=1 -lsize=1g,logdev=/dev/nvme1n1p6 /dev/nvme1n1p5
> 
>     FSTYP=xfs
>     export TEST_DEV=/dev/nvme1n1p3
>     export TEST_DIR=/mnt/test
>     export TEST_LOGDEV=/dev/nvme1n1p4
>     export SCRATCH_DEV=/dev/nvme1n1p5
>     export SCRATCH_MNT=/mnt/scratch
>     export SCRATCH_LOGDEV=/dev/nvme1n1p6
>     export USE_EXTERNAL=yes
> 
>     sudo ./check generic/311
> 
> Current semantics allow two concurrent freezers: one initiated from
> userspace via FREEZE_HOLDER_USERSPACE and one initiated from the kernel
> via FREEZE_HOLDER_KERNEL. If there are multiple concurrent freeze
> requests from either FREEZE_HOLDER_USERSPACE or FREEZE_HOLDER_KERNEL
> -EBUSY is returned.
> 
> We need to preserve these semantics because as they are uapi via
> FIFREEZE and FITHAW ioctl()s. IOW, freezes don't nest for FIFREEZE and
> FITHAW. Other kernels consumers rely on non-nesting freezes as well.
> 
> With freezes initiated from the block layer freezes need to nest if the
> same superblock is frozen via multiple devices. So we need to start
> counting the number of freeze requests.
> 
> If FREEZE_HOLDER_BDEV is passed alongside FREEZE_HOLDER_KERNEL or
> FREEZE_HOLDER_USERSPACE we allow the caller to nest freeze calls.

FREEZE_HOLDER_BDEV should be FREEZE_MAY_NEST I guess.

> To accommodate the old semantics we split the freeze counter into two
> counting kernel initiated and userspace initiated freezes separately. We
> can then also stop recording FREEZE_HOLDER_* in struct sb_writers.
> 
> We also simplify freezing by making all concurrent freezers share a
> single active superblock reference count instead of having separate
> references for kernel and userspace. I don't see why we would need two
> active reference counts. Neither FREEZE_HOLDER_KERNEL nor
> FREEZE_HOLDER_USERSPACE can put the active reference as long as they are
> concurrent freezers anwyay. That was already true before we allowed
> nesting freezes.
> 
> Survives various fstests runs with different options including the
> reproducer, online scrub, and online repair, fsfreze, and so on. Also
> survives blktests.
> 
> Reported-by: Chandan Babu R <chandanbabu@kernel.org>
> Link: https://lore.kernel.org/linux-block/87bkccnwxc.fsf@debian-BULLSEYE-live-builder-AMD64
> Fixes: [1]: bfac4176f2c4 ("bdev: implement freeze and thaw holder operations") # no backport needed
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Just one more typo fix below. Feel free to add:

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

> @@ -1930,6 +2008,13 @@ static int wait_for_partially_frozen(struct super_block *sb)
>   * userspace can both hold a filesystem frozen.  The filesystem remains frozen
>   * until there are no kernel or userspace freezes in effect.
>   *
> + * A filesystem may hold multiple devices and thus a filesystems may be
> + * frozen through the block layer via multiple block devices. In this
> + * case the request is marked as being allowed to nest passig
							  ^^ passing

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/2] fs: handle freezing from multiple devices

[Christian Brauner]

> FREEZE_HOLDER_BDEV should be FREEZE_MAY_NEST I guess.

Yes, indeed.

> Just one more typo fix below. Feel free to add:

Thanks, I fixed both things.

Re: [PATCH v2 0/2] Handle multi device freezing

[Christian Brauner]

On Sat, 04 Nov 2023 15:00:12 +0100, Christian Brauner wrote:
> Above we call super_lock_excl() which waits until the superblock is
> SB_BORN and since SB_BORN is never unset once set this check can never
> fire. Plus, we also hold an active reference at this point already so
> this superblock can't even be shutdown.
> 
> 

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/2] fs: remove dead check
      https://git.kernel.org/vfs/vfs/c/9745915cc5e0
[2/2] fs: handle freezing from multiple devices
      https://git.kernel.org/vfs/vfs/c/03630940f622