| Message ID | 20250201184021.278437-1-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [blktests] srp: skip test if scsi_transport_srp module is loaded and in use | expand |
CC+: Bart, On Feb 02, 2025 / 00:10, Nilay Shroff wrote: > The srp/* tests requires exclusive access to scsi_transport_srp > module. Running srp/* tests would definitely fail if the test can't > get exclusive access of scsi_transport_srp module as shown below: > > $ lsmod | grep scsi_transport_srp > scsi_transport_srp 327680 1 ibmvscsi > > $ ./check srp/001 > srp/001 (Create and remove LUNs) [failed] > runtime ... 0.249s > tests/srp/rc: line 263: /sys/class/srp_remote_ports/port-0:1/delete: Permission denied > tests/srp/rc: line 263: /sys/class/srp_remote_ports/port-0:1/delete: Permission denied > modprobe: FATAL: Module scsi_transport_srp is in use. > error: Invalid argument > error: Invalid argument > > So if the scsi_transport_srp module is loaded and in use then skip > running srp/* tests. Thanks. I think this is a good improvement. > > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> > --- > common/rc | 11 +++++++++++ > tests/srp/rc | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/common/rc b/common/rc > index bcb215d..73e0b9a 100644 > --- a/common/rc > +++ b/common/rc > @@ -78,6 +78,17 @@ _have_module() { > return 0 > } > > +_have_module_not_in_use() { > + _have_module "$1" || return > + > + if [ -d "/sys/module/$1" ]; then > + refcnt="$(cat /sys/module/$1/refcnt)" > + if [ "$refcnt" -ne "0" ]; then > + SKIP_REASONS+=("module $1 is in use") > + fi > + fi > +} Spaces are used for indents. Please use tabs instead. Nit: "refcnt" is not declared as a local variable. Let's declare it. Or, it would be the better to avoid "refcnt". Instead, $(< /sys/module/$1/refcnt) can be used as follows. Either way is fine for me. if [ -d "/sys/module/$1" ] && (($(</sys/module/"$1"/refcnt) != 0)); then SKIP_REASONS+=("module $1 is in use") fi
On 2/1/25 10:40 AM, Nilay Shroff wrote:
> +_have_module_not_in_use() {
The name "_have_module_not_in_use()" sounds weird to me. Wouldn't
_module_not_in_use() be a better name?
Thanks,
Bart.
On 2/4/25 5:25 PM, Shinichiro Kawasaki wrote: > CC+: Bart, > > On Feb 02, 2025 / 00:10, Nilay Shroff wrote: >> The srp/* tests requires exclusive access to scsi_transport_srp >> module. Running srp/* tests would definitely fail if the test can't >> get exclusive access of scsi_transport_srp module as shown below: >> >> $ lsmod | grep scsi_transport_srp >> scsi_transport_srp 327680 1 ibmvscsi >> >> $ ./check srp/001 >> srp/001 (Create and remove LUNs) [failed] >> runtime ... 0.249s >> tests/srp/rc: line 263: /sys/class/srp_remote_ports/port-0:1/delete: Permission denied >> tests/srp/rc: line 263: /sys/class/srp_remote_ports/port-0:1/delete: Permission denied >> modprobe: FATAL: Module scsi_transport_srp is in use. >> error: Invalid argument >> error: Invalid argument >> >> So if the scsi_transport_srp module is loaded and in use then skip >> running srp/* tests. > > Thanks. I think this is a good improvement. > >> >> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> >> --- >> common/rc | 11 +++++++++++ >> tests/srp/rc | 1 + >> 2 files changed, 12 insertions(+) >> >> diff --git a/common/rc b/common/rc >> index bcb215d..73e0b9a 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -78,6 +78,17 @@ _have_module() { >> return 0 >> } >> >> +_have_module_not_in_use() { >> + _have_module "$1" || return >> + >> + if [ -d "/sys/module/$1" ]; then >> + refcnt="$(cat /sys/module/$1/refcnt)" >> + if [ "$refcnt" -ne "0" ]; then >> + SKIP_REASONS+=("module $1 is in use") >> + fi >> + fi >> +} > > Spaces are used for indents. Please use tabs instead. > Grrr.. I think my editor inserted spaces instead of tabs, I would fix this in the next version of the patch. > Nit: "refcnt" is not declared as a local variable. Let's declare it. Or, it > would be the better to avoid "refcnt". Instead, $(< /sys/module/$1/refcnt) > can be used as follows. Either way is fine for me. > > if [ -d "/sys/module/$1" ] && (($(</sys/module/"$1"/refcnt) != 0)); then > SKIP_REASONS+=("module $1 is in use") > fi Okay, I'd make "refcnt" local and update. Thanks, --Nilay
On 2/4/25 11:30 PM, Bart Van Assche wrote: > On 2/1/25 10:40 AM, Nilay Shroff wrote: >> +_have_module_not_in_use() { > > The name "_have_module_not_in_use()" sounds weird to me. Wouldn't > _module_not_in_use() be a better name? > Yeah _module_not_in_use() sounds better. I will incorporate this in the next patch. Thanks, --Nilay
diff --git a/common/rc b/common/rc index bcb215d..73e0b9a 100644 --- a/common/rc +++ b/common/rc @@ -78,6 +78,17 @@ _have_module() { return 0 } +_have_module_not_in_use() { + _have_module "$1" || return + + if [ -d "/sys/module/$1" ]; then + refcnt="$(cat /sys/module/$1/refcnt)" + if [ "$refcnt" -ne "0" ]; then + SKIP_REASONS+=("module $1 is in use") + fi + fi +} + _have_module_param() { _have_driver "$1" || return diff --git a/tests/srp/rc b/tests/srp/rc index 85bd1dd..1bc7b20 100755 --- a/tests/srp/rc +++ b/tests/srp/rc @@ -61,6 +61,7 @@ group_requires() { _have_module scsi_debug _have_module target_core_iblock _have_module target_core_mod + _have_module_not_in_use scsi_transport_srp for p in mkfs.ext4 mkfs.xfs multipath multipathd pidof rdma \ sg_reset fio; do
The srp/* tests requires exclusive access to scsi_transport_srp module. Running srp/* tests would definitely fail if the test can't get exclusive access of scsi_transport_srp module as shown below: $ lsmod | grep scsi_transport_srp scsi_transport_srp 327680 1 ibmvscsi $ ./check srp/001 srp/001 (Create and remove LUNs) [failed] runtime ... 0.249s tests/srp/rc: line 263: /sys/class/srp_remote_ports/port-0:1/delete: Permission denied tests/srp/rc: line 263: /sys/class/srp_remote_ports/port-0:1/delete: Permission denied modprobe: FATAL: Module scsi_transport_srp is in use. error: Invalid argument error: Invalid argument So if the scsi_transport_srp module is loaded and in use then skip running srp/* tests. Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> --- common/rc | 11 +++++++++++ tests/srp/rc | 1 + 2 files changed, 12 insertions(+)