| Message ID | 20190805232512.50992-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] Make the NVMe tests more reliable | expand |
On 06/08/2019 01:25, Bart Van Assche wrote: [...] > + for ((i=0;i<10;i++)); do > + [ -e /sys/block/$dev/uuid ] && > + [ -e /sys/block/$dev/wwid ] && > + return 0 > + sleep .1 > + done > + return 1 > fi > done > + return 1 Hmmm, I don't really understand why you're adding the return {0,1} here. None of the callers of _find_nvme_loop_dev() does anything with the return value of the function. They expect either a nvme-device or an empty string and fail if the string is empty due to a non-empty diff in the golden output. Thanks, Johannes
On 8/6/19 1:11 AM, Johannes Thumshirn wrote: > On 06/08/2019 01:25, Bart Van Assche wrote: > [...] > >> + for ((i=0;i<10;i++)); do >> + [ -e /sys/block/$dev/uuid ] && >> + [ -e /sys/block/$dev/wwid ] && >> + return 0 >> + sleep .1 >> + done >> + return 1 >> fi >> done >> + return 1 > > Hmmm, I don't really understand why you're adding the return {0,1} here. > None of the callers of _find_nvme_loop_dev() does anything with the > return value of the function. > > They expect either a nvme-device or an empty string and fail if the > string is empty due to a non-empty diff in the golden output. Hi Johannes, The "return 0" statement has been added to break out of the two for-loops. The first "return 1" statement has been added to make sure that the echo "$dev" statement is executed at most once. The final "return 1" statement has been added to make the return value consistent. Do you perhaps want me to leave out {0,1} from the return statements? Bart.
On 2019-08-05 5:25 p.m., Bart Van Assche wrote: > When running blktests with kernel debugging enabled it can happen that > _find_nvme_loop_dev() returns before the NVMe block device has appeared > in sysfs. This patch avoids that the NVMe tests fail as follows: > > +cat: /sys/block/nvme1n1/uuid: No such file or directory > +cat: /sys/block/nvme1n1/wwid: No such file or directory > > Cc: Logan Gunthorpe <logang@deltatee.com> > Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Makes sense to me. Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > --- > tests/nvme/rc | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/tests/nvme/rc b/tests/nvme/rc > index 348b4a3c2cbc..05dfc5915a13 100644 > --- a/tests/nvme/rc > +++ b/tests/nvme/rc > @@ -169,8 +169,16 @@ _find_nvme_loop_dev() { > transport="$(cat "/sys/class/nvme/${dev}/transport")" > if [[ "$transport" == "loop" ]]; then > echo "$dev" > + for ((i=0;i<10;i++)); do > + [ -e /sys/block/$dev/uuid ] && > + [ -e /sys/block/$dev/wwid ] && > + return 0 > + sleep .1 > + done > + return 1 > fi > done > + return 1 > } > > _filter_discovery() { >
On Tue, Aug 06, 2019 at 08:11:02AM -0700, Bart Van Assche wrote: > On 8/6/19 1:11 AM, Johannes Thumshirn wrote: > > On 06/08/2019 01:25, Bart Van Assche wrote: > > [...] > > > > > + for ((i=0;i<10;i++)); do > > > + [ -e /sys/block/$dev/uuid ] && > > > + [ -e /sys/block/$dev/wwid ] && > > > + return 0 > > > + sleep .1 > > > + done > > > + return 1 > > > fi > > > done > > > + return 1 > > > > Hmmm, I don't really understand why you're adding the return {0,1} here. > > None of the callers of _find_nvme_loop_dev() does anything with the > > return value of the function. > > > > They expect either a nvme-device or an empty string and fail if the > > string is empty due to a non-empty diff in the golden output. > > Hi Johannes, > > The "return 0" statement has been added to break out of the two for-loops. > The first "return 1" statement has been added to make sure that the echo > "$dev" statement is executed at most once. The final "return 1" statement > has been added to make the return value consistent. > > Do you perhaps want me to leave out {0,1} from the return statements? Yes, I think this is less confusing for readers. With that, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Mon, Aug 05, 2019 at 04:25:12PM -0700, Bart Van Assche wrote: > When running blktests with kernel debugging enabled it can happen that > _find_nvme_loop_dev() returns before the NVMe block device has appeared > in sysfs. This patch avoids that the NVMe tests fail as follows: > > +cat: /sys/block/nvme1n1/uuid: No such file or directory > +cat: /sys/block/nvme1n1/wwid: No such file or directory > > Cc: Logan Gunthorpe <logang@deltatee.com> > Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > tests/nvme/rc | 8 ++++++++ > 1 file changed, 8 insertions(+) Thanks, Bart, I made Johannes' suggested change and applied.
diff --git a/tests/nvme/rc b/tests/nvme/rc index 348b4a3c2cbc..05dfc5915a13 100644 --- a/tests/nvme/rc +++ b/tests/nvme/rc @@ -169,8 +169,16 @@ _find_nvme_loop_dev() { transport="$(cat "/sys/class/nvme/${dev}/transport")" if [[ "$transport" == "loop" ]]; then echo "$dev" + for ((i=0;i<10;i++)); do + [ -e /sys/block/$dev/uuid ] && + [ -e /sys/block/$dev/wwid ] && + return 0 + sleep .1 + done + return 1 fi done + return 1 } _filter_discovery() {
When running blktests with kernel debugging enabled it can happen that _find_nvme_loop_dev() returns before the NVMe block device has appeared in sysfs. This patch avoids that the NVMe tests fail as follows: +cat: /sys/block/nvme1n1/uuid: No such file or directory +cat: /sys/block/nvme1n1/wwid: No such file or directory Cc: Logan Gunthorpe <logang@deltatee.com> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> Cc: Johannes Thumshirn <jthumshirn@suse.de> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- tests/nvme/rc | 8 ++++++++ 1 file changed, 8 insertions(+)