Re: [PATCH blktests] md: add regression test for "md/md-bitmap: fix writing non bitmap pages"

[Ofir Gal <ofir.gal@volumez.com>]

On 17/06/2024 14:32, Shinichiro Kawasaki wrote:
> On Jun 13, 2024 / 15:30, Ofir Gal wrote:
>> A bug in md-bitmap has been discovered by setting up a md raid on top of
>> nvme-tcp devices that has optimal io size larger than the allocated
>> bitmap.
>>
>> The following test reproduce the bug by setting up a md-raid on top of
>> nvme-tcp device over ram device that sets the optimal io size by using
>> dm-stripe.
>>
>> Signed-off-by: Ofir Gal <ofir.gal@volumez.com>
>> ---
>> This is my first contribution to blktests. The tests hangs when being
>> ran on a kernel without the bugfix. Should we wait for the bugfix to be
>> merged to upstream before merging the test?
> Thank you for the contribution :) If the bug does not cause any hang or failure
> of other following test case runs, I think it's ok to add this test case to the
> blktests before the fix gets merged to the upstream.
>
> Please find my in-line comments below. Later on, I will do trial runs.
Great!
>>  common/brd       | 28 +++++++++++++++++
>>  tests/md/001     | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/md/001.out |  2 ++
>>  tests/md/rc      | 12 ++++++++
>>  4 files changed, 122 insertions(+)
>>  create mode 100644 common/brd
>>  create mode 100755 tests/md/001
>>  create mode 100644 tests/md/001.out
>>  create mode 100644 tests/md/rc
>>
>> diff --git a/common/brd b/common/brd
>> new file mode 100644
>> index 0000000..b8cd4e3
>> --- /dev/null
>> +++ b/common/brd
>> @@ -0,0 +1,28 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Ofir Gal
>> +#
>> +# brd helper functions
>> +
>> +. common/shellcheck
>> +
>> +_have_brd() {
>> +	_have_driver brd
> This _have_driver() call checks brd driver is available, but it does not check
> brd driver is loadable. I think _init_brd() and _cleanup_brd() assume that brd
> driver is loadable. For that check, please do "_have_module brd" instead.
>
> One left work is to improve this case work with built-in brd driver, because
> some blktests users prefer running tests with built-in drivers. At this point, I
> think it's okay to go with loadable brd driver.
Ok, applied to v2

>> diff --git a/tests/md/001 b/tests/md/001
>> new file mode 100755
>> index 0000000..d5fb755
>> --- /dev/null
>> +++ b/tests/md/001
>> @@ -0,0 +1,80 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Ofir Gal
>> +#
>> +# Regression test for patch "md/md-bitmap: fix writing non bitmap pages" and
>> +# for patch "nvme-tcp: use sendpages_ok() instead of sendpage_ok()"
>> +
>> +. tests/md/rc
>> +. tests/nvme/rc
> I want to avoid cross references acoss test groups. So far, all test groups do
> not have any cross reference to keep them independent. How about to add this
> test case to the nvme test group?
I don't mind to add it to the nvme test group, just to clarify the test
checks a bug in md. The bug is "visible" only when the underlying device
of the raid is a network block device that utilize MSG_SPLICE_PAGES.

nvme-tcp is used as the network device, I'm not sure it's related to
the nvme test group. What do you think?

>> +. common/brd
>> +
>> +DESCRIPTION="Create a raid with bitmap on top of nvme device with
>> +optimal-io-size over bitmap size"
> This descrption is printed as blktests runs. All other blktests have single line
> description then the two lines description looks strange. Can we make it shorter
> to fit in one line?
Yes, does "Raid with bitmap on nvme device with opt-io-size over bitmap
size" sounds good?

>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	setup_underlying_device
>> +	setup_nvme_over_tcp
>> +
>> +	# Hangs here without the fix
>> +	mdadm -q --create /dev/md/blktests_md --level=1 --bitmap=internal \
> In the past, short options caused some trouble. I suggest to use the long option
> "--quiet" in place of the short option "-q".
Applied to v2.