Re: [PATCH blktests] nvme/068: add a test for multipath delayed removal

[John Garry <john.g.garry@oracle.com>]

On 16/04/2026 12:18, Nilay Shroff wrote:
> 
>> diff --git a/tests/nvme/068 b/tests/nvme/068
>> new file mode 100644
>> index 0000000..e06fd6b
>> --- /dev/null
>> +++ b/tests/nvme/068
>> @@ -0,0 +1,118 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2026 John Garry
>> +#
>> +# Test NVMe multipath delayed removal works as expected
>> +
> Should we also reference the commit 62188639ec16 ("nvme-multipath:
> introduce delayed removal of the multipath head node") which introduced
> this feature in the kernel?
> 

Sure, if you think that it is useful.

We could also introduce at some point the _fixed_by_kernel_commit (like 
xfstests), but I am not sure if it has value for blktests

>> +. tests/nvme/rc
>> +. common/xfs
>> +
>> +DESCRIPTION="NVMe multipath delayed removal test"
>> +QUICK=1
>> +
>> +requires() {
>> +    _nvme_requires
>> +    _have_loop
>> +    _have_module_param_value nvme_core multipath Y
>> +    _require_nvme_trtype_is_fabrics
>> +}
>> +
>> +set_conditions() {
>> +    _set_nvme_trtype "$@"
>> +}
>> +
>> +_delayed_nvme_reconnect_ctrl() {
>> +    sleep 5
>> +    _nvme_connect_subsys
>> +}
>> +
>> +test() {
>> +    echo "Running ${TEST_NAME}"
>> +
>> +    _setup_nvmet
>> +
>> +    local nvmedev
>> +    local ns
>> +    local bytes_written
>> +    local refcnt_orig
>> +    local refcnt
>> +    _nvmet_target_setup
>> +
>> +    _nvme_connect_subsys
>> +
>> +    # Part a: Prove that writes fail when no path returns. Any reads or
>> +    #      writes are queued during the delayed removal period. If no
>> +    #      paths return before the timer expires, then those IOs should
>> +    #      fail.
>> +    #      During the delayed removal period, ensure that the module
>> +    #      refcnt is incremented, to prove that we cannot remove the
>> +    #      driver during this period.
>> +    nvmedev=$(_find_nvme_dev "${def_subsysnqn}")
>> +    ns=$(_find_nvme_ns "${def_subsys_uuid}")
>> +    refcnt=$(_module_use_count nvme_core)
> 
> Should the refcount be stored in $refcnt_orig instead of $refcnt?

Yeah, that's a small bug, as I needlessly store it twice:

+	refcnt=$(_module_use_count nvme_core)
+	echo 10 > "/sys/block/"$ns"/delayed_removal_secs"
+	refcnt_orig=$(_module_use_count nvme_core)

> 
>> +    echo 10 > "/sys/block/"$ns"/delayed_removal_secs"
>> +    refcnt_orig=$(_module_use_count nvme_core)
> 
> We may not need to again store refcnt here if we store it
> before enabling delayed_removal_sec above.
> 
> Other than above trivial comments, this looks good to me.
> 

Cheers,
John