[PATCH] fstests: generic/730: exclude btrfs for now

[PATCH] fstests: generic/730: exclude btrfs for now

[Qu Wenruo]

The test case always fail for btrfs:

  generic/730       - output mismatch (see /home/adam/xfstests-dev/results//generic/730.out.bad)
    --- tests/generic/730.out	2024-04-25 18:13:45.203549435 +0930
    +++ /home/adam/xfstests-dev/results//generic/730.out.bad	2025-06-04 15:10:39.062430952 +0930
    @@ -1,2 +1 @@
     QA output created by 730
    -cat: -: Input/output error
    ...

The root reasons are:

- Btrfs doesn't support shutdown callbacks

- The current shutdown callbacks are per-fs
  Meanwhile btrfs is a multi-device fs, it needs to know which block
  device is triggerring shutdown, and needs to do extra evaluation
  (e.g. can the remaining devices support RW operations) before
  triggering a full fs shutdown.

I'm trying to improve the situation, but until then just exlucde btrfs
from the test case for now.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/generic/730 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/generic/730 b/tests/generic/730
index 6251980e..cae6489f 100755
--- a/tests/generic/730
+++ b/tests/generic/730
@@ -26,6 +26,10 @@ _require_test
 _require_block_device $TEST_DEV
 _require_scsi_debug
 
+if [ "$FSTYP" = "btrfs" ]; then
+	_notrun "btrfs doesn't support per-fs shutdown yet"
+fi
+
 size=$(_small_fs_size_mb 256)
 SCSI_DEBUG_DEV=`_get_scsi_debug_dev 512 512 0 $size`
 test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
-- 
2.49.0

Re: [PATCH] fstests: generic/730: exclude btrfs for now

[Christoph Hellwig]

On Wed, Jun 04, 2025 at 03:55:09PM +0930, Qu Wenruo wrote:
> - Btrfs doesn't support shutdown callbacks
> 
> - The current shutdown callbacks are per-fs

The callsbacks are per-block device.

>   Meanwhile btrfs is a multi-device fs, it needs to know which block
>   device is triggerring shutdown, and needs to do extra evaluation
>   (e.g. can the remaining devices support RW operations) before
>   triggering a full fs shutdown.

Exactly for that reason.

> +++ b/tests/generic/730
> @@ -26,6 +26,10 @@ _require_test
>  _require_block_device $TEST_DEV
>  _require_scsi_debug
>  
> +if [ "$FSTYP" = "btrfs" ]; then
> +	_notrun "btrfs doesn't support per-fs shutdown yet"
> +fi

Please don't add these horrible fs excludes to tests.  Add a helper
in common to check if something is supported.

Re: [PATCH] fstests: generic/730: exclude btrfs for now

[Qu Wenruo]

在 2025/6/4 17:55, Christoph Hellwig 写道:
> On Wed, Jun 04, 2025 at 03:55:09PM +0930, Qu Wenruo wrote:
>> - Btrfs doesn't support shutdown callbacks
>>
>> - The current shutdown callbacks are per-fs
> 
> The callsbacks are per-block device.
> 
>>    Meanwhile btrfs is a multi-device fs, it needs to know which block
>>    device is triggerring shutdown, and needs to do extra evaluation
>>    (e.g. can the remaining devices support RW operations) before
>>    triggering a full fs shutdown.
> 
> Exactly for that reason.

Right, the ext4/xfs can go a superblock shutdown because they are single 
device fs (except the external journal device), and fs_holder_ops 
handles the single device failure by calling back into the super block 
shutdown call back.

For a multi-device fs, it should go through the blk_holder_ops, which 
btrfs doesn't provide when opening the devices.

> 
>> +++ b/tests/generic/730
>> @@ -26,6 +26,10 @@ _require_test
>>   _require_block_device $TEST_DEV
>>   _require_scsi_debug
>>   
>> +if [ "$FSTYP" = "btrfs" ]; then
>> +	_notrun "btrfs doesn't support per-fs shutdown yet"
>> +fi
> 
> Please don't add these horrible fs excludes to tests.  Add a helper
> in common to check if something is supported.
Although there is the _require_scratch_shutdown, it's only checking the 
full fs shutdown ioctl.

I guess it means, if a fs supports per-bdev shutdown, it won't hurt to 
also provide a full-fs shutdown ioctl?

Thanks,
Qu

Re: [PATCH] fstests: generic/730: exclude btrfs for now

[Christoph Hellwig]

On Wed, Jun 04, 2025 at 06:47:41PM +0930, Qu Wenruo wrote:
> Right, the ext4/xfs can go a superblock shutdown because they are single
> device fs (except the external journal device),

XFS also has a RT device in addition to that.

> and fs_holder_ops handles
> the single device failure by calling back into the super block shutdown call
> back.
> 
> For a multi-device fs, it should go through the blk_holder_ops, which btrfs
> doesn't provide when opening the devices.

Yes, btrfs would need it's own fs_ops.  Alternatively we could add a new
->devloss super_operations method and change fs_bdev_mark_dead to
something like:

	if (sb->s_op->devloss && sb->s_op->devloss(sb, bdev, surprise))
		goto done;

	<sync fs and stuff)
	if (sb->s_op->shutdown)
		sb->s_op->shutdown(sb);
done:
	super_unlock_shared(sb);

so that btrfs or other multi-device file systems don't have to duplicate
the logic.

> I guess it means, if a fs supports per-bdev shutdown, it won't hurt to also
> provide a full-fs shutdown ioctl?

Supporting it is a good idea in general as it enables a lot of good test
coverage in xfstests.

Re: [PATCH] fstests: generic/730: exclude btrfs for now

[Zorro Lang]

On Wed, Jun 04, 2025 at 03:55:09PM +0930, Qu Wenruo wrote:
> The test case always fail for btrfs:
> 
>   generic/730       - output mismatch (see /home/adam/xfstests-dev/results//generic/730.out.bad)
>     --- tests/generic/730.out	2024-04-25 18:13:45.203549435 +0930
>     +++ /home/adam/xfstests-dev/results//generic/730.out.bad	2025-06-04 15:10:39.062430952 +0930
>     @@ -1,2 +1 @@
>      QA output created by 730
>     -cat: -: Input/output error
>     ...
> 
> The root reasons are:
> 
> - Btrfs doesn't support shutdown callbacks
> 
> - The current shutdown callbacks are per-fs
>   Meanwhile btrfs is a multi-device fs, it needs to know which block
>   device is triggerring shutdown, and needs to do extra evaluation
>   (e.g. can the remaining devices support RW operations) before
>   triggering a full fs shutdown.
> 
> I'm trying to improve the situation, but until then just exlucde btrfs
> from the test case for now.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/generic/730 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/generic/730 b/tests/generic/730
> index 6251980e..cae6489f 100755
> --- a/tests/generic/730
> +++ b/tests/generic/730
> @@ -26,6 +26,10 @@ _require_test
>  _require_block_device $TEST_DEV
>  _require_scsi_debug
>  
> +if [ "$FSTYP" = "btrfs" ]; then
> +	_notrun "btrfs doesn't support per-fs shutdown yet"
> +fi
> +

Please use `_exclude_fs btrfs`, if you still hope to _notrun it on btrfs later :)

>  size=$(_small_fs_size_mb 256)
>  SCSI_DEBUG_DEV=`_get_scsi_debug_dev 512 512 0 $size`
>  test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
> -- 
> 2.49.0
> 
> 

Re: [PATCH] fstests: generic/730: exclude btrfs for now

[Qu Wenruo]

在 2025/6/4 22:54, Christoph Hellwig 写道:
> On Wed, Jun 04, 2025 at 06:47:41PM +0930, Qu Wenruo wrote:
>> Right, the ext4/xfs can go a superblock shutdown because they are single
>> device fs (except the external journal device),
> 
> XFS also has a RT device in addition to that.
> 
>> and fs_holder_ops handles
>> the single device failure by calling back into the super block shutdown call
>> back.
>>
>> For a multi-device fs, it should go through the blk_holder_ops, which btrfs
>> doesn't provide when opening the devices.
> 
> Yes, btrfs would need it's own fs_ops.  Alternatively we could add a new
> ->devloss super_operations method and change fs_bdev_mark_dead to
> something like:
> 
> 	if (sb->s_op->devloss && sb->s_op->devloss(sb, bdev, surprise))
> 		goto done;
> 
> 	<sync fs and stuff)
> 	if (sb->s_op->shutdown)
> 		sb->s_op->shutdown(sb);
> done:
> 	super_unlock_shared(sb);
> 
> so that btrfs or other multi-device file systems don't have to duplicate
> the logic.

I learnt this the hard way.

I tried to implement a btrfs specific blk_holder_ops, and just use 
btrfs_fs_info as the holder.

Test case generic/085 easily crash the kernel with freeze/thaw and 
mount/unmount races.


Bcachefs is doing its own blk_holder_ops but with extra protection with 
similar refcount and locks.

Considering btrfs doesn't got through the regular setup_bdev_super() 
patch, I guess for now we have to duplicate the logic for now.

Thanks,
Qu

> 
>> I guess it means, if a fs supports per-bdev shutdown, it won't hurt to also
>> provide a full-fs shutdown ioctl?
> 
> Supporting it is a good idea in general as it enables a lot of good test
> coverage in xfstests.
>