Re: [PATCH 2/2] blktests: add test for ANA state transition

[Hannes Reinecke]

On 07/21/2018 11:29 PM, Chaitanya Kulkarni wrote:

From: linux-block-ow...@vger.kernel.org  on behalf 
of Hannes Reinecke 
Sent: Tuesday, July 17, 2018 6:31 AM
To: Omar Sandoval
Cc: Christoph Hellwig; Sagi Grimberg; Keith Busch; 
linux-n...@lists.infradead.org; linux-block@vger.kernel.org; Hannes Reinecke; 
Hannes Reinecke
Subject: [PATCH 2/2] blktests: add test for ANA state transition
   
  
Signed-off-by: Hannes Reinecke 

---
  tests/nvme/014 | 158 +
  tests/nvme/014.out |  17 ++
  2 files changed, 175 insertions(+)
  create mode 100755 tests/nvme/014
  create mode 100644 tests/nvme/014.out

diff --git a/tests/nvme/014 b/tests/nvme/014
new file mode 100755
index 000..4b57229
--- /dev/null
+++ b/tests/nvme/014
@@ -0,0 +1,158 @@
+#!/bin/bash
+#
+# Regression test for ANA base support
+#
+# Copyright (C) 2018 Hannes Reinecke
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .


Licenses - GNU Project - Free Software Foundation
www.gnu.org
Published software should be free software.To make it free software, you need 
to release it under
a free software license. We normally use the GNU General Public License (GNU 
GPL), specifying version 3
or any later version, but occasionally we use other free software licenses.


I don't mind, I just copied it over from testcase 10...


+
+. tests/nvme/rc
+
+DESCRIPTION="test ANA optimized/transitioning/inaccessible support"
+QUICK=1
+
+switch_nvmet_anagroup() {
+   local port1="$1"
+   local port2="$2"
+   local mode="$3"
+
+   echo "ANA state ${mode}"
+
+   if [ "${mode}" = "change" ] ; then
+   _set_nvmet_anagroup_state "${port1}" "1" "change"
+   _set_nvmet_anagroup_state "${port1}" "2" "change"
+   _set_nvmet_anagroup_state "${port2}" "1" "change"
+   _set_nvmet_anagroup_state "${port2}" "2" "change"
+   elif [ "${mode}" = "failover" ] ; then
+   _set_nvmet_anagroup_state "${port1}" "1" "inaccessible"
+   _set_nvmet_anagroup_state "${port1}" "2" "optimized"
+   _set_nvmet_anagroup_state "${port2}" "1" "optimized"
+   _set_nvmet_anagroup_state "${port2}" "2" "inaccessible"
+   else
+   _set_nvmet_anagroup_state "${port1}" "1" "optimized"
+   _set_nvmet_anagroup_state "${port1}" "2" "inaccessible"
+   _set_nvmet_anagroup_state "${port2}" "1" "inaccessible"
+   _set_nvmet_anagroup_state "${port2}" "2" "optimized"
+   fi
+}
+
+_display_ana_state() {
+   local grpid state
[CK] Newliine here ?


Okay


+   for nvme in /sys/class/nvme/* ; do
+   for c in ${nvme}/nvme* ; do
+   if [ ! -d ${c} ] ; then
+   echo "${nvme##*/}: ANA disabled"
+   continue
+   fi
+   grpid="$(cat "${c}/ana_grpid")"
+   state="$(cat "${c}/ana_state")"
+   echo "${c##*/}: grpid ${grpid} state ${state}"
+   done
+   done
+}

I think we need to move above functions to the  ${BLKTESTS_HOME}/tests/nvme/rc.


Okay, can do.


+
+_switch_ana_states() {
+   local port1=$1
+   local port2=$2
+
[CK] Please remove the extra line.
+}
[CK] I was not able to find a caller for above function, I'm I missing 
something ?
+

Yeah, that's an older function which got left over during refactoring.


+requires() {
+   _have_program nvme && _have_module nvme-loop && _have_module loop && \
+   _have_configfs && _have_fio
[CK] Missing nvmet module from the above list.
+}

Can we split following test function into small routines, it will be easier to 
review and
maintain?

+
+test() {
+   echo "Running ${TEST_NAME}"
+
+   modprobe nvmet
+   modprobe nvme-loop
+
+   local port1
+   port1="$(_create_nvmet_port "loop")"
[CK] Can we initialize variables at the time of declaration or after 
declaration of all the
variables ?


Sure. But we should do it consistently; the older tests also do not 
follow these rules...



+   ag1="$(_create_nvmet_anagroup "${port1}")"
[CK] Not sure if we need ag1 variable.


Yeah, it's more for symmetry than anything else.


+
+   local port2
[CK] Can we plese declare all the variable at the top please see 
tests/nvme/006-013
to 

Re: [PATCH 2/2] blktests: add test for ANA state transition

[Chaitanya Kulkarni]

From: linux-block-ow...@vger.kernel.org  on 
behalf of Hannes Reinecke 
Sent: Tuesday, July 17, 2018 6:31 AM
To: Omar Sandoval
Cc: Christoph Hellwig; Sagi Grimberg; Keith Busch; 
linux-n...@lists.infradead.org; linux-block@vger.kernel.org; Hannes Reinecke; 
Hannes Reinecke
Subject: [PATCH 2/2] blktests: add test for ANA state transition
  
 
Signed-off-by: Hannes Reinecke 
---
 tests/nvme/014 | 158 +
 tests/nvme/014.out |  17 ++
 2 files changed, 175 insertions(+)
 create mode 100755 tests/nvme/014
 create mode 100644 tests/nvme/014.out

diff --git a/tests/nvme/014 b/tests/nvme/014
new file mode 100755
index 000..4b57229
--- /dev/null
+++ b/tests/nvme/014
@@ -0,0 +1,158 @@
+#!/bin/bash
+#
+# Regression test for ANA base support
+#
+# Copyright (C) 2018 Hannes Reinecke
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .


Licenses - GNU Project - Free Software Foundation
www.gnu.org
Published software should be free software.To make it free software, you need 
to release it under a free software license. We normally use the GNU General 
Public License (GNU GPL), specifying version 3 or any later version, but 
occasionally we use other free software licenses.

+
+. tests/nvme/rc
+
+DESCRIPTION="test ANA optimized/transitioning/inaccessible support"
+QUICK=1
+
+switch_nvmet_anagroup() {
+   local port1="$1"
+   local port2="$2"
+   local mode="$3"
+
+   echo "ANA state ${mode}"
+
+   if [ "${mode}" = "change" ] ; then
+   _set_nvmet_anagroup_state "${port1}" "1" "change"
+   _set_nvmet_anagroup_state "${port1}" "2" "change"
+   _set_nvmet_anagroup_state "${port2}" "1" "change"
+   _set_nvmet_anagroup_state "${port2}" "2" "change"
+   elif [ "${mode}" = "failover" ] ; then
+   _set_nvmet_anagroup_state "${port1}" "1" "inaccessible"
+   _set_nvmet_anagroup_state "${port1}" "2" "optimized"
+   _set_nvmet_anagroup_state "${port2}" "1" "optimized"
+   _set_nvmet_anagroup_state "${port2}" "2" "inaccessible"
+   else
+   _set_nvmet_anagroup_state "${port1}" "1" "optimized"
+   _set_nvmet_anagroup_state "${port1}" "2" "inaccessible"
+   _set_nvmet_anagroup_state "${port2}" "1" "inaccessible"
+   _set_nvmet_anagroup_state "${port2}" "2" "optimized"
+   fi
+}
+
+_display_ana_state() {
+   local grpid state
[CK] Newliine here ?
+   for nvme in /sys/class/nvme/* ; do
+   for c in ${nvme}/nvme* ; do
+   if [ ! -d ${c} ] ; then
+   echo "${nvme##*/}: ANA disabled"
+   continue
+   fi
+   grpid="$(cat "${c}/ana_grpid")"
+   state="$(cat "${c}/ana_state")"
+   echo "${c##*/}: grpid ${grpid} state ${state}"
+   done
+   done
+}

I think we need to move above functions to the  ${BLKTESTS_HOME}/tests/nvme/rc.
+
+_switch_ana_states() {
+   local port1=$1
+   local port2=$2
+
[CK] Please remove the extra line.
+}
[CK] I was not able to find a caller for above function, I'm I missing 
something ?
+
+requires() {
+   _have_program nvme && _have_module nvme-loop && _have_module loop && \
+   _have_configfs && _have_fio
[CK] Missing nvmet module from the above list.
+}

Can we split following test function into small routines, it will be easier to 
review and
maintain?

+
+test() {
+   echo "Running ${TEST_NAME}"
+
+   modprobe nvmet
+   modprobe nvme-loop
+
+   local port1
+   port1="$(_create_nvmet_port "loop")"
[CK] Can we initialize variables at the time of declaration or after 
declaration of all the
variables ?
+   ag1="$(_create_nvmet_anagroup "${port1}")"
[CK] Not sure if we need ag1 variable.

+
+   local port2
[CK] Can we plese declare all the variable at the top please see 
tests/nvme/006-013
to maintain uniform style ?
+   port2="$(_create_nvmet_port "loop")"
+   ag2="$(_create_nvmet_anagroup "${port2}")"
+
+   truncate -s 1G "$TMPDIR/img1"
+
+   local loop_dev1
+   loop_dev1="$(losetup -f --show "$TMPDIR/img1")"
+
+   _create_nvmet_subsystem "blktests-subsystem-1" "${loop_dev1}" \
+   "91fdba0d-f87b-4c25-b80f-db7be1418b9e" 

Re: [PATCH 2/2] blktests: add test for ANA state transition

[Johannes Thumshirn]

On Tue, Jul 17, 2018 at 03:31:18PM +0200, Hannes Reinecke wrote:
> +requires() {
> + _have_program nvme && _have_module nvme-loop && _have_module loop && \
> + _have_configfs && _have_fio
> +}

this needs '_have_module_param nvme-core multipath' as well.


-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850