Re: [PATCH v2 3/7] nvme: make tests transport type agnostic

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

On 8/7/20 10:19 AM, Sagi Grimberg wrote:
> 
>>> diff --git a/tests/nvme/002 b/tests/nvme/002
>>> index 999e222705bf..8540623497c7 100755
>>> --- a/tests/nvme/002
>>> +++ b/tests/nvme/002
>>> @@ -21,7 +21,7 @@ test() {
>>>     
>>>     	local iterations=1000
>>>     	local port
>>> -	port="$(_create_nvmet_port "loop")"
>>> +	port="$(_create_nvmet_port ${nvme_trtype})"
>> Is there a way to directly use nvme_trtype especially in rc ?
>> if not disregard this comment.
> 
> I didn't want to do this, because a test can create multiple ports.
> But maybe it could have a default value?
> 
>>> @@ -33,10 +33,10 @@ test() {
>>>     		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
>>>     	_add_nvmet_subsys_to_port "${port}" "blktests-subsystem-1"
>>>     
>>> -	_nvme_connect_subsys "loop" "blktests-subsystem-1"
>>> +	_nvme_connect_subsys ${nvme_trtype} "blktests-subsystem-1"
>>>     
>>>     	local nvmedev
>>> -	nvmedev="$(_find_nvme_loop_dev)"
>>> +	nvmedev="$(_find_nvme_dev)"
>>>     	cat "/sys/block/${nvmedev}n1/uuid"
>>>     	cat "/sys/block/${nvmedev}n1/wwid"
>>
>> Since we are touching nvmedev can we move above uuid and wwid to
>> a wrapper something like _nvme_show_uuid_wwid ${nvmedev}n1 ?
> 
> Doesn't help the patch set cause, so it can be added incrementally.
Okay.
> 
>>
>>>
>>> @@ -36,12 +36,12 @@ test() {
>>>     
>>>     	loop_dev="$(losetup -f --show "$TMPDIR/img")"
>>>     
>>> -	port="$(_create_nvmet_port "loop")"
>>> +	port="$(_create_nvmet_port ${nvme_trtype})"
>>>     
>>>     	for ((i = 0; i < iterations; i++)); do
>>>     		_create_nvmet_subsystem "${subsys}$i" "${loop_dev}"
>>>     		_add_nvmet_subsys_to_port "${port}" "${subsys}$i"
>>> -		_nvme_connect_subsys "loop" "${subsys}$i"
>>> +		_nvme_connect_subsys ${nvme_trtype} "${subsys}$i"
>> Same here for nvme_trtype as first comment.
>>>     		_nvme_disconnect_subsys "${subsys}$i" >> "${FULL}" 2>&1
>>>     		_remove_nvmet_subsystem_from_port "${port}" "${subsys}$i"
>>>     		_remove_nvmet_subsystem "${subsys}$i"
>>> diff --git a/tests/nvme/rc b/tests/nvme/rc
>>> index 6d57cf591300..191f0497416a 100644
>>> --- a/tests/nvme/rc
>>> +++ b/tests/nvme/rc
>>> @@ -6,6 +6,9 @@
>>>     
>>>     . common/rc
>>>     
>>> +def_traddr="127.0.0.1"
>>> +def_adrfam="ipv4"
>>> +def_trsvcid="4420"
>>>     nvme_trtype=${nvme_trtype:-"loop"}
>>>     
>>>     _nvme_requires() {
>>> @@ -62,8 +65,8 @@ _cleanup_nvmet() {
>>>     	for dev in /sys/class/nvme/nvme*; do
>>>     		dev="$(basename "$dev")"
>>>     		transport="$(cat "/sys/class/nvme/${dev}/transport")"
>>> -		if [[ "$transport" == "loop" ]]; then
>>> -			echo "WARNING: Test did not clean up loop device: ${dev}"
>>> +		if [[ "$transport" == "${nvme_trtype}" ]]; then
>>> +			echo "WARNING: Test did not clean up ${nvme_trtype} device: ${dev}"
>>>     			_nvme_disconnect_ctrl "${dev}"
>>>     		fi
>>>     	done
>>> @@ -87,14 +90,20 @@ _cleanup_nvmet() {
>>>     	shopt -u nullglob
>>>     	trap SIGINT
>>>     
>>> -	modprobe -r nvme-loop 2>/dev/null
>>> +	modprobe -r nvme-${nvme_trtype} 2>/dev/null
>>> +	if [[ "${nvme_trtype}" != "loop" ]]; then
>>> +		modprobe -r nvmet-${nvme_trtype} 2>/dev/null
>> This is not from your patch but I'd keep the error message it has
>> turned out to be useful for me when debugging refcount problem
>> especially unload and load scenario.
> 
> Again, I'd like to avoid doing things that are outside the scope
> of what this is trying to achieve because it is not a small change.
> 
> We can add it incrementally.
That make sense, we can do it incrementally.
>