Re: [PATCH 1/2] generic: add a test for device removal with dirty data

[Zorro Lang <zlang@redhat.com>]

On Tue, Jul 11, 2023 at 12:09:30PM +0200, Christoph Hellwig wrote:
> Test the removal of the underlying device when the file system still
> has dirty data.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Thanks this new version! Sorry for the late review. Most of the cases look
good to me, just a bit picky reveiw points as below. (Same to patch 2/2).

>  tests/generic/730     | 57 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/730.out |  2 ++
>  2 files changed, 59 insertions(+)
>  create mode 100755 tests/generic/730
>  create mode 100644 tests/generic/730.out
> 
> diff --git a/tests/generic/730 b/tests/generic/730
> new file mode 100755
> index 00000000..6fba31fc
> --- /dev/null
> +++ b/tests/generic/730
> @@ -0,0 +1,57 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2015 Red Hat Inc. All Rights Reserved.
> +# Copyright (c) 2023 Christoph Hellwig
> +#
> +# Test proper file system shut down when the block device is removed underneath
> +# and there is dirty data.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +_cleanup()
> +{
> +	cd /
> +	$UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
> +	_put_scsi_debug_dev
> +	rm -f $tmp.*
> +}
> +
> +. ./common/filter
> +. ./common/scsi_debug
> +
> +_supported_fs generic
> +
> +# We don't actually use the test device, but we need a block based fs
> +_require_test
> +_require_block_device $TEST_DEV
> +_require_scsi_debug
> +
> +physical=`blockdev --getpbsz $SCRATCH_DEV`
> +logical=`blockdev --getss $SCRATCH_DEV`

There's not _require_scratch, so I doubt we should use SCRATCH_DEV directly.
How about use TEST_DEV (due to this case calls _require_test) ?

> +
> +SCSI_DEBUG_DEV=`_get_scsi_debug_dev ${physical:-512} ${logical:-512} 0 300`
> +test -b "$SCSI_DEBUG_DEV" || _notrun "Failed to initialize scsi debug device"
> +echo "SCSI debug device $SCSI_DEBUG_DEV" >>$seqres.full
> +
> +_mkfs_dev $SCSI_DEBUG_DEV

How about _fail if it fails?

> +
> +_mount $SCSI_DEBUG_DEV $SCRATCH_MNT

I don't know if mount can return non-zero without any output. If you hope to
check the return status, you can use run_check() in common/rc.

> +
> +# create a test file
> +$XFS_IO_PROG -f -c "pwrite 0 1M" $SCRATCH_MNT/testfile >>$seqres.full
> +
> +# open a file descriptor for reading the file
> +exec 3< $SCRATCH_MNT/testfile
> +
> +# delete the scsi debug device while it still has dirty data
> +echo 1 > /sys/block/`_short_dev $SCSI_DEBUG_DEV`/device/delete

Is this file "/sys/block/`_short_dev $SCSI_DEBUG_DEV`/device/delete" 100%
supported? Or we'd better to _notrun, not report pass.

(BTW, the `command` and $(command) are both work, but the $() is more recommended
in fstests (not mandatory:). If both formats are good, choose $(command).
http://mywiki.wooledge.org/BashFAQ/082)

Thanks,
Zorro

> +
> +# try to read from the file, which should give us -EIO
> +cat <&3 > /dev/null
> +
> +# close the file descriptor to not block unmount
> +exec 3<&-
> +
> +status=0
> +exit
> diff --git a/tests/generic/730.out b/tests/generic/730.out
> new file mode 100644
> index 00000000..79e96db8
> --- /dev/null
> +++ b/tests/generic/730.out
> @@ -0,0 +1,2 @@
> +QA output created by 730
> +cat: -: Input/output error
> -- 
> 2.39.2
>