| Message ID | 20230817073021.3674602-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests,v2,1/2] nvme/rc: fix nvme device readiness check after _nvme_connect_subsys | expand |
On Thu, Aug 17, 2023 at 04:30:20PM +0900, Shin'ichiro Kawasaki wrote: > The helper function _nvme_connect_subsys() creates a nvme device. It may > take some time after the function call until the device gets ready for > I/O. So it is expected that the test cases call _find_nvme_dev() after > _nvme_connect_subsys() before I/O. _find_nvme_dev() returns the path of > the created device, and it also waits for uuid and wwid sysfs attributes > of the created device get ready. This wait works as the wait for the > device I/O readiness. > > However, this wait by _find_nvme_dev() has two problems. The first > problem is missing call of _find_nvme_dev(). The test case nvme/047 > calls _nvme_connect_subsys() twice, but _find_nvme_dev() is called only > for the first _nvme_connect_subsys() call. This causes too early I/O to > the device with tcp transport [1]. Fix this by moving the wait for the > device readiness from _find_nvme_dev() to _nvme_connect_subsys(). Also > add --wait-for option to _nvme_connect_subsys(). It allows to skip the > wait in _nvmet_passthru_target_connect() which has its own wait for > device readiness. > > The second problem is wrong paths for the sysfs attributes. The paths > do not include namespace index, so the check for the attributes always > fail. Still _find_nvme_dev() does 1 second wait and allows the device > get ready for I/O in most cases, but this is not intended behavior. > Fix the paths by adding the namespace index. > > On top of the checks for sysfs attributes, add 'udevadm settle' and a > check for the created device file. These ensures that the create device > is ready for I/O. > > [1] https://lore.kernel.org/linux-block/CAHj4cs9GNohGUjohNw93jrr8JGNcRYC-ienAZz+sa7az1RK77w@mail.gmail.com/ > > Fixes: c766fccf3aff ("Make the NVMe tests more reliable") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Thanks! Reviewed-by: Daniel Wagner <dwagner@suse.de>
On 8/17/23 10:30, Shin'ichiro Kawasaki wrote: > The helper function _nvme_connect_subsys() creates a nvme device. It may > take some time after the function call until the device gets ready for > I/O. So it is expected that the test cases call _find_nvme_dev() after > _nvme_connect_subsys() before I/O. _find_nvme_dev() returns the path of > the created device, and it also waits for uuid and wwid sysfs attributes > of the created device get ready. This wait works as the wait for the > device I/O readiness. > > However, this wait by _find_nvme_dev() has two problems. The first > problem is missing call of _find_nvme_dev(). The test case nvme/047 > calls _nvme_connect_subsys() twice, but _find_nvme_dev() is called only > for the first _nvme_connect_subsys() call. This causes too early I/O to > the device with tcp transport [1]. Fix this by moving the wait for the > device readiness from _find_nvme_dev() to _nvme_connect_subsys(). Also > add --wait-for option to _nvme_connect_subsys(). It allows to skip the > wait in _nvmet_passthru_target_connect() which has its own wait for > device readiness. > > The second problem is wrong paths for the sysfs attributes. The paths > do not include namespace index, so the check for the attributes always > fail. Still _find_nvme_dev() does 1 second wait and allows the device > get ready for I/O in most cases, but this is not intended behavior. > Fix the paths by adding the namespace index. > > On top of the checks for sysfs attributes, add 'udevadm settle' and a > check for the created device file. These ensures that the create device > is ready for I/O. > > [1] https://lore.kernel.org/linux-block/CAHj4cs9GNohGUjohNw93jrr8JGNcRYC-ienAZz+sa7az1RK77w@mail.gmail.com/ > > Fixes: c766fccf3aff ("Make the NVMe tests more reliable") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > Changes from v1: > * Added --wait-for option to _nvme_connect_subsys() > * Added 'udevadm settle' before readiness check > > tests/nvme/rc | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/tests/nvme/rc b/tests/nvme/rc > index 0b964e9..797483e 100644 > --- a/tests/nvme/rc > +++ b/tests/nvme/rc > @@ -428,6 +428,8 @@ _nvme_connect_subsys() { > local keep_alive_tmo="" > local reconnect_delay="" > local ctrl_loss_tmo="" > + local wait_for="ns" > + local dev i > > while [[ $# -gt 0 ]]; do > case $1 in > @@ -483,6 +485,10 @@ _nvme_connect_subsys() { > ctrl_loss_tmo="$2" > shift 2 > ;; > + --wait-for) > + wait_for="$2" > + shift 2 > + ;; I think that it would make better since to reverse the polarity here. connect always wait, and tests that actually want to do some fast stress will choose --nowait. > *) > positional_args+=("$1") > shift > @@ -532,6 +538,21 @@ _nvme_connect_subsys() { > fi > > nvme connect "${ARGS[@]}" 2> /dev/null > + > + # Wait until device file and uuid/wwid sysfs attributes get ready for > + # namespace 1. > + if [[ ${wait_for} == ns ]]; then > + udevadm settle > + dev=$(_find_nvme_dev "$subsysnqn") > + for ((i = 0; i < 10; i++)); do > + if [[ -b /dev/${dev}n1 && > + -e /sys/block/${dev}n1/uuid && > + -e /sys/block/${dev}n1/wwid ]]; then > + return What happens if the subsystem does not have any namespaces? Or what about other namesapces? Won't it make more sense to inspect the subsys for expected ns and wait for all?
On Aug 17, 2023 / 11:29, Sagi Grimberg wrote: > On 8/17/23 10:30, Shin'ichiro Kawasaki wrote: [...] > > diff --git a/tests/nvme/rc b/tests/nvme/rc > > index 0b964e9..797483e 100644 > > --- a/tests/nvme/rc > > +++ b/tests/nvme/rc > > @@ -428,6 +428,8 @@ _nvme_connect_subsys() { > > local keep_alive_tmo="" > > local reconnect_delay="" > > local ctrl_loss_tmo="" > > + local wait_for="ns" > > + local dev i > > while [[ $# -gt 0 ]]; do > > case $1 in > > @@ -483,6 +485,10 @@ _nvme_connect_subsys() { > > ctrl_loss_tmo="$2" > > shift 2 > > ;; > > + --wait-for) > > + wait_for="$2" > > + shift 2 > > + ;; > > I think that it would make better since to reverse the polarity > here. > > connect always wait, and tests that actually want to > do some fast stress will choose --nowait. Agreed. Will reflect in v3. > > > *) > > positional_args+=("$1") > > shift > > @@ -532,6 +538,21 @@ _nvme_connect_subsys() { > > fi > > nvme connect "${ARGS[@]}" 2> /dev/null > > + > > + # Wait until device file and uuid/wwid sysfs attributes get ready for > > + # namespace 1. > > + if [[ ${wait_for} == ns ]]; then > > + udevadm settle > > + dev=$(_find_nvme_dev "$subsysnqn") > > + for ((i = 0; i < 10; i++)); do > > + if [[ -b /dev/${dev}n1 && > > + -e /sys/block/${dev}n1/uuid && > > + -e /sys/block/${dev}n1/wwid ]]; then > > + return > > What happens if the subsystem does not have any namespaces? > Or what about other namesapces? > > Won't it make more sense to inspect the subsys for > expected ns and wait for all? I think such check is possible. I assume that we can refer /sys/kernel/config/nvmet/subsystems/namespaces to get the expected ns and check them all. Will cook something for v3.
diff --git a/tests/nvme/rc b/tests/nvme/rc index 0b964e9..797483e 100644 --- a/tests/nvme/rc +++ b/tests/nvme/rc @@ -428,6 +428,8 @@ _nvme_connect_subsys() { local keep_alive_tmo="" local reconnect_delay="" local ctrl_loss_tmo="" + local wait_for="ns" + local dev i while [[ $# -gt 0 ]]; do case $1 in @@ -483,6 +485,10 @@ _nvme_connect_subsys() { ctrl_loss_tmo="$2" shift 2 ;; + --wait-for) + wait_for="$2" + shift 2 + ;; *) positional_args+=("$1") shift @@ -532,6 +538,21 @@ _nvme_connect_subsys() { fi nvme connect "${ARGS[@]}" 2> /dev/null + + # Wait until device file and uuid/wwid sysfs attributes get ready for + # namespace 1. + if [[ ${wait_for} == ns ]]; then + udevadm settle + dev=$(_find_nvme_dev "$subsysnqn") + for ((i = 0; i < 10; i++)); do + if [[ -b /dev/${dev}n1 && + -e /sys/block/${dev}n1/uuid && + -e /sys/block/${dev}n1/wwid ]]; then + return + fi + sleep .1 + done + fi } _nvme_discover() { @@ -758,13 +779,6 @@ _find_nvme_dev() { subsysnqn="$(cat "/sys/class/nvme/${dev}/subsysnqn")" if [[ "$subsysnqn" == "$subsys" ]]; then echo "$dev" - for ((i = 0; i < 10; i++)); do - if [[ -e /sys/block/$dev/uuid && - -e /sys/block/$dev/wwid ]]; then - return - fi - sleep .1 - done fi done } @@ -794,7 +808,8 @@ _nvmet_passthru_target_connect() { local trtype=$1 local subsys_name=$2 - _nvme_connect_subsys "${trtype}" "${subsys_name}" || return + _nvme_connect_subsys "${trtype}" "${subsys_name}" --wait-for=none || + return nsdev=$(_find_nvme_passthru_loop_dev "${subsys_name}") # The following tests can race with the creation
The helper function _nvme_connect_subsys() creates a nvme device. It may take some time after the function call until the device gets ready for I/O. So it is expected that the test cases call _find_nvme_dev() after _nvme_connect_subsys() before I/O. _find_nvme_dev() returns the path of the created device, and it also waits for uuid and wwid sysfs attributes of the created device get ready. This wait works as the wait for the device I/O readiness. However, this wait by _find_nvme_dev() has two problems. The first problem is missing call of _find_nvme_dev(). The test case nvme/047 calls _nvme_connect_subsys() twice, but _find_nvme_dev() is called only for the first _nvme_connect_subsys() call. This causes too early I/O to the device with tcp transport [1]. Fix this by moving the wait for the device readiness from _find_nvme_dev() to _nvme_connect_subsys(). Also add --wait-for option to _nvme_connect_subsys(). It allows to skip the wait in _nvmet_passthru_target_connect() which has its own wait for device readiness. The second problem is wrong paths for the sysfs attributes. The paths do not include namespace index, so the check for the attributes always fail. Still _find_nvme_dev() does 1 second wait and allows the device get ready for I/O in most cases, but this is not intended behavior. Fix the paths by adding the namespace index. On top of the checks for sysfs attributes, add 'udevadm settle' and a check for the created device file. These ensures that the create device is ready for I/O. [1] https://lore.kernel.org/linux-block/CAHj4cs9GNohGUjohNw93jrr8JGNcRYC-ienAZz+sa7az1RK77w@mail.gmail.com/ Fixes: c766fccf3aff ("Make the NVMe tests more reliable") Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- Changes from v1: * Added --wait-for option to _nvme_connect_subsys() * Added 'udevadm settle' before readiness check tests/nvme/rc | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)