| Message ID | 20200930185422.11494-4-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NVMe Target Passthru Block Tests | expand |
On 9/30/20 11:54, Logan Gunthorpe wrote: > Make a common helper from the code in tests nvme/012 and nvme/013 > to run an fio verify on a XFS file system backed by the > specified block device. > > While we are at it, all the output is redirected to $FULL instead of > /dev/null. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > common/xfs | 22 ++++++++++++++++++++++ > tests/nvme/012 | 14 +------------- > tests/nvme/013 | 14 +------------- > 3 files changed, 24 insertions(+), 26 deletions(-) The common namespace is getting cluttered. Can you please create a subdirectory common/fs/xfs ? > > diff --git a/common/xfs b/common/xfs > index d1a603b8c7b5..210c924cdd41 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -9,3 +9,25 @@ > _have_xfs() { > _have_fs xfs && _have_program mkfs.xfs > } > + > +_xfs_mkfs_and_mount() { > + local bdev=$1 > + local mount_dir=$2 > + > + mkdir -p "${mount_dir}" > + umount "${mount_dir}" > + mkfs.xfs -l size=32m -f "${bdev}" > + mount "${bdev}" "${mount_dir}" > +} > + > +_xfs_run_fio_verify_io() { > + local mount_dir="/mnt/blktests" The mount dir should be a parameter and not the hardcode value to make it reusable. > + local bdev=$1 > + > + _xfs_mkfs_and_mount "${bdev}" "${mount_dir}" >> "${FULL}" 2>&1 > + > + _run_fio_verify_io --size=950m --directory="${mount_dir}/" > + > + umount "${mount_dir}" >> "${FULL}" 2>&1 > + rm -fr "${mount_dir}" > +} > diff --git a/tests/nvme/012 b/tests/nvme/012 > index 1d8d8e3cc271..a13cd08ce6bf 100755 > --- a/tests/nvme/012 > +++ b/tests/nvme/012 > @@ -26,12 +26,9 @@ test() { > local port > local nvmedev > local loop_dev > - local mount_dir="/mnt/blktests" > local file_path="${TMPDIR}/img" > local subsys_name="blktests-subsystem-1" > > - mkdir -p "${mount_dir}" > /dev/null 2>&1 > - > truncate -s 1G "${file_path}" > > loop_dev="$(losetup -f --show "${file_path}")" > @@ -47,15 +44,7 @@ test() { > cat "/sys/block/${nvmedev}n1/uuid" > cat "/sys/block/${nvmedev}n1/wwid" > > - umount ${mount_dir} > /dev/null 2>&1 > - > - mkfs.xfs -l size=32m -f /dev/"${nvmedev}n1" > /dev/null 2>&1 > - > - mount /dev/"${nvmedev}n1" "${mount_dir}" > - > - _run_fio_verify_io --size=950m --directory="${mount_dir}/" > - > - umount "${mount_dir}" > /dev/null 2>&1 > + _xfs_run_fio_verify_io "/dev/${nvmedev}n1" > > _nvme_disconnect_subsys "${subsys_name}" > > @@ -66,7 +55,6 @@ test() { > losetup -d "${loop_dev}" > > rm "${file_path}" > - rm -fr "${mount_dir}" > > echo "Test complete" > } > diff --git a/tests/nvme/013 b/tests/nvme/013 > index 3819a2730d9b..1ac725ea83f2 100755 > --- a/tests/nvme/013 > +++ b/tests/nvme/013 > @@ -24,13 +24,10 @@ test() { > > local port > local nvmedev > - local mount_dir="/mnt/blktests/" > local file_path="${TMPDIR}/img" > > local subsys_name="blktests-subsystem-1" > > - mkdir -p "${mount_dir}" > /dev/null 2>&1 > - > truncate -s 1G "${file_path}" > > _create_nvmet_subsystem "${subsys_name}" "${file_path}" \ > @@ -44,15 +41,7 @@ test() { > cat "/sys/block/${nvmedev}n1/uuid" > cat "/sys/block/${nvmedev}n1/wwid" > > - umount ${mount_dir} > /dev/null 2>&1 > - > - mkfs.xfs -l size=32m -f /dev/"${nvmedev}n1" > /dev/null 2>&1 > - > - mount /dev/"${nvmedev}n1" "${mount_dir}" > - > - _run_fio_verify_io --size=800m --directory="${mount_dir}/" > - > - umount "${mount_dir}" > /dev/null 2>&1 > + _xfs_run_fio_verify_io "/dev/${nvmedev}n1" > > _nvme_disconnect_subsys "${subsys_name}" > > @@ -61,7 +50,6 @@ test() { > _remove_nvmet_port "${port}" > > rm "${file_path}" > - rm -fr "${mount_dir}" > > echo "Test complete" > } rest looks good
On 2020-10-06 5:50 p.m., Chaitanya Kulkarni wrote: > On 9/30/20 11:54, Logan Gunthorpe wrote: >> Make a common helper from the code in tests nvme/012 and nvme/013 >> to run an fio verify on a XFS file system backed by the >> specified block device. >> >> While we are at it, all the output is redirected to $FULL instead of >> /dev/null. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> common/xfs | 22 ++++++++++++++++++++++ >> tests/nvme/012 | 14 +------------- >> tests/nvme/013 | 14 +------------- >> 3 files changed, 24 insertions(+), 26 deletions(-) > > The common namespace is getting cluttered. Can you please create > > a subdirectory common/fs/xfs ? I disagree. The common directory only has 9 files. And given there appear to be no other files to add to the fs directory I don't think now is the time to create a directory. We can do so if and when a number of other fs helpers show up and there's a reason to group them together. >> >> diff --git a/common/xfs b/common/xfs >> index d1a603b8c7b5..210c924cdd41 100644 >> --- a/common/xfs >> +++ b/common/xfs >> @@ -9,3 +9,25 @@ >> _have_xfs() { >> _have_fs xfs && _have_program mkfs.xfs >> } >> + >> +_xfs_mkfs_and_mount() { >> + local bdev=$1 >> + local mount_dir=$2 >> + >> + mkdir -p "${mount_dir}" >> + umount "${mount_dir}" >> + mkfs.xfs -l size=32m -f "${bdev}" >> + mount "${bdev}" "${mount_dir}" >> +} >> + >> +_xfs_run_fio_verify_io() { >> + local mount_dir="/mnt/blktests" > > The mount dir should be a parameter and not the hardcode value > to make it reusable. I also disagree here. It is already reusable and is used in a number of places; none of those places require changing the mount directory. If and when a use comes up that requires a different directory (not sure what that would be), a parameter can be added. It is typically standard practice in the Linux community to not add features that have no users as it's confusing to people reading the code. Logan
On 2020-10-06 6:20 p.m., Chaitanya Kulkarni wrote: > On 10/6/20 16:59, Logan Gunthorpe wrote: >>> The mount dir should be a parameter and not the hardcode value >>> to make it reusable. >> I also disagree here. It is already reusable and is used in a number of >> places; none of those places require changing the mount directory. If >> and when a use comes up that requires a different directory (not sure >> what that would be), a parameter can be added. It is typically standard >> practice in the Linux community to not add features that have no users >> as it's confusing to people reading the code. >> >> Logan >> > Well if you are making a helper it should be generic if you have a usecase, "Generic" isn't a binary yes/no quality. Why add the mount option (that nobody is using) and not a size option as well that nobody uses? For that matter, fio has a ton of options we could expose. (think io-method, read/write pattern, etc, etc). The criteria we decide upon which options get exposed as arguments is what the code that's actually using it needs -- not what's available or what you imagine future use cases might be. If there are no users in the code it should not be exposed. If a use case comes along, an argument can easily be added when the new test is added to the code base. > mounted on different mount points not just one which is important testcase, > > that will require a prep patch. So? That's normal. > Why can't we do that right now when we have a clear usecase ? We don't have a clear use case that's being added to the code though... We have an imagined use case that hasn't been written. Add the feature when you add this use case. Logan
diff --git a/common/xfs b/common/xfs index d1a603b8c7b5..210c924cdd41 100644 --- a/common/xfs +++ b/common/xfs @@ -9,3 +9,25 @@ _have_xfs() { _have_fs xfs && _have_program mkfs.xfs } + +_xfs_mkfs_and_mount() { + local bdev=$1 + local mount_dir=$2 + + mkdir -p "${mount_dir}" + umount "${mount_dir}" + mkfs.xfs -l size=32m -f "${bdev}" + mount "${bdev}" "${mount_dir}" +} + +_xfs_run_fio_verify_io() { + local mount_dir="/mnt/blktests" + local bdev=$1 + + _xfs_mkfs_and_mount "${bdev}" "${mount_dir}" >> "${FULL}" 2>&1 + + _run_fio_verify_io --size=950m --directory="${mount_dir}/" + + umount "${mount_dir}" >> "${FULL}" 2>&1 + rm -fr "${mount_dir}" +} diff --git a/tests/nvme/012 b/tests/nvme/012 index 1d8d8e3cc271..a13cd08ce6bf 100755 --- a/tests/nvme/012 +++ b/tests/nvme/012 @@ -26,12 +26,9 @@ test() { local port local nvmedev local loop_dev - local mount_dir="/mnt/blktests" local file_path="${TMPDIR}/img" local subsys_name="blktests-subsystem-1" - mkdir -p "${mount_dir}" > /dev/null 2>&1 - truncate -s 1G "${file_path}" loop_dev="$(losetup -f --show "${file_path}")" @@ -47,15 +44,7 @@ test() { cat "/sys/block/${nvmedev}n1/uuid" cat "/sys/block/${nvmedev}n1/wwid" - umount ${mount_dir} > /dev/null 2>&1 - - mkfs.xfs -l size=32m -f /dev/"${nvmedev}n1" > /dev/null 2>&1 - - mount /dev/"${nvmedev}n1" "${mount_dir}" - - _run_fio_verify_io --size=950m --directory="${mount_dir}/" - - umount "${mount_dir}" > /dev/null 2>&1 + _xfs_run_fio_verify_io "/dev/${nvmedev}n1" _nvme_disconnect_subsys "${subsys_name}" @@ -66,7 +55,6 @@ test() { losetup -d "${loop_dev}" rm "${file_path}" - rm -fr "${mount_dir}" echo "Test complete" } diff --git a/tests/nvme/013 b/tests/nvme/013 index 3819a2730d9b..1ac725ea83f2 100755 --- a/tests/nvme/013 +++ b/tests/nvme/013 @@ -24,13 +24,10 @@ test() { local port local nvmedev - local mount_dir="/mnt/blktests/" local file_path="${TMPDIR}/img" local subsys_name="blktests-subsystem-1" - mkdir -p "${mount_dir}" > /dev/null 2>&1 - truncate -s 1G "${file_path}" _create_nvmet_subsystem "${subsys_name}" "${file_path}" \ @@ -44,15 +41,7 @@ test() { cat "/sys/block/${nvmedev}n1/uuid" cat "/sys/block/${nvmedev}n1/wwid" - umount ${mount_dir} > /dev/null 2>&1 - - mkfs.xfs -l size=32m -f /dev/"${nvmedev}n1" > /dev/null 2>&1 - - mount /dev/"${nvmedev}n1" "${mount_dir}" - - _run_fio_verify_io --size=800m --directory="${mount_dir}/" - - umount "${mount_dir}" > /dev/null 2>&1 + _xfs_run_fio_verify_io "/dev/${nvmedev}n1" _nvme_disconnect_subsys "${subsys_name}" @@ -61,7 +50,6 @@ test() { _remove_nvmet_port "${port}" rm "${file_path}" - rm -fr "${mount_dir}" echo "Test complete" }
Make a common helper from the code in tests nvme/012 and nvme/013 to run an fio verify on a XFS file system backed by the specified block device. While we are at it, all the output is redirected to $FULL instead of /dev/null. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- common/xfs | 22 ++++++++++++++++++++++ tests/nvme/012 | 14 +------------- tests/nvme/013 | 14 +------------- 3 files changed, 24 insertions(+), 26 deletions(-)