| Message ID | 1439566305-12460-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 14, 2015 at 11:31:43PM +0800, Anand Jain wrote: > From: Anand Jain <Anand.Jain@oracle.com> > > Controlled EIO from the device is achieved using the dm device. > Helper functions are at common/dmerror. > > Broadly steps will include calling _init_dmerror(). > _init_dmerror() will use SCRATCH_DEV to create dm linear device and assign > DMERROR_DEV to /dev/mapper/error-test. > > When test script is ready to get EIO, the test cases can call > _load_dmerror_table() which then it will load the dm error. > so that reading DMERROR_DEV will cause EIO. After the test case is > complete, cleanup must be done by calling _cleanup_dmerror(). > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Reviewed-by: Filipe Manana <fdmanana@suse.com> > --- > v5->v6: accepts Eryu's comments > v4->v5: No Change. keep up with the patch set > v3->v4: rebase on latest xfstests code > v2.1->v3: accepts Filipe Manana's review comments, thanks > v2->v2.1: fixed missed typo error fixup in the commit. > v1->v2: accepts Dave Chinner's review comments, thanks This is not a change log. A change log describes the changes that were made, not who asked for changes to be made. i.e. I have no idea what changes you actually made from the previous version in this patch set.... .... > +# common functions for setting up and tearing down a dmerror device > + > +_init_dmerror() All these function names shoul dbe prefixed _dmerror_<blah> so that it is clear the namespace we are working on. (similar to _scratch-<blah>, _test_<blah>, etc). > +{ > + $DMSETUP_PROG remove error-test > /dev/null 2>&1 > + > + local BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` I think I've said this before - local variables are lower case, global/exported variables are upper case. > + > + DMERROR_DEV='/dev/mapper/error-test' > + > + DMLINEAR_TABLE="0 $BLK_DEV_SIZE linear $SCRATCH_DEV 0" > + > + $DMSETUP_PROG create error-test --table "$DMLINEAR_TABLE" || \ > + _fatal "failed to create dm linear device" That should be _fail... _fatal() is local to common/config (as it can be included in scripts without including common/rc). Tests include common/rc (and not common/config directly), so they should all use _fail... > + DMERROR_TABLE="0 $BLK_DEV_SIZE error $SCRATCH_DEV 0" > +} > + > +_scratch_mkfs_dmerror() > +{ > + $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $DMERROR_DEV >> $seqres.full 2>&1 || \ > + _fatal "failed to create mkfs.btrfs $* $DMERROR_DEV" > +} This has nothing to do with the scratch device. The test should simply call '_mkfs_dev $DMERROR_DEV' as there's no need for a wrapper here. > + > +_mount_dmerror() > +{ > + $MOUNT_PROG -t $FSTYP $MOUNT_OPTIONS $DMERROR_DEV $SCRATCH_MNT > +} Should mirror _scratch_mount. _mount -t $FSTYP `_scratch_mount_options` $DMERROR_DEV $SCRATCH_MNT > + > +_unmount_dmerror() > +{ > + $UMOUNT_PROG $SCRATCH_MNT > +} Doesn't need a wrapper. > + > +_cleanup_dmerror() > +{ > + $UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1 > + $DMSETUP_PROG remove error-test > /dev/null 2>&1 > +} > + > +_load_dmerror_table() > +{ > + $DMSETUP_PROG suspend error-test > + [ $? -ne 0 ] && _fatal "failed to suspend error-test" Error message makes no sense when read as the reason for a test failing. '_fail "dmsetup suspend failed"' makes more sense, as that is the command that failed. > + > + $DMSETUP_PROG load error-test --table "$DMERROR_TABLE" > + [ $? -ne 0 ] && _fatal "failed to load error table error-test" _fail "dmsetup failed to load error table" > + > + $DMSETUP_PROG resume error-test > + [ $? -ne 0 ] && _fatal "failed to resume error-test" _fail "dmsetup resume failed" > +} > diff --git a/common/rc b/common/rc > index 70d2fa8..8d4da0e 100644 > --- a/common/rc > +++ b/common/rc > @@ -1337,6 +1337,15 @@ _require_sane_bdev_flush() > fi > } > > +# this test requires the device mapper error target > +# > +_require_dmerror() > +{ > + _require_command "$DMSETUP_PROG" dmsetup > + $DMSETUP_PROG targets | grep error >/dev/null 2>&1 > + [ $? -ne 0 ] && _notrun "This test requires dm error support" > +} Why isn't this in common/dmerror? Cheers, Dave.
Hi Dave, All comments accepted thanks. except for this. >> +_mount_dmerror() >> +{ >> + $MOUNT_PROG -t $FSTYP $MOUNT_OPTIONS $DMERROR_DEV $SCRATCH_MNT >> +} > > Should mirror _scratch_mount. > > _mount -t $FSTYP `_scratch_mount_options` $DMERROR_DEV $SCRATCH_MNT `_scratch_mount_options` also returns $SCRATCH_DEV. in case of tests involving dmerror module, dmerror_init would use $SCRATCH_DEV as backing device and provide $DMERROR_DEV to be used instead of $SCRATCH_DEV. So I am proposing.. + _mount -t $FSTYP $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $* $DMERROR_DEV $SCRATCH_MNT Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 18, 2015 at 12:26:54PM +0800, anand jain wrote: > > Hi Dave, > > All comments accepted thanks. except for this. > > > >>+_mount_dmerror() > >>+{ > >>+ $MOUNT_PROG -t $FSTYP $MOUNT_OPTIONS $DMERROR_DEV $SCRATCH_MNT > >>+} > > > >Should mirror _scratch_mount. > > > >_mount -t $FSTYP `_scratch_mount_options` $DMERROR_DEV $SCRATCH_MNT > > > `_scratch_mount_options` also returns $SCRATCH_DEV. > in case of tests involving dmerror module, dmerror_init would use > $SCRATCH_DEV as backing device and provide $DMERROR_DEV to be used > instead of $SCRATCH_DEV. So I am proposing.. > > + _mount -t $FSTYP $SCRATCH_OPTIONS $MOUNT_OPTIONS > $SELINUX_MOUNT_OPTIONS $* $DMERROR_DEV $SCRATCH_MNT Ok, SCRATCH_OPTIONS might not be the best idea here, so feel free to drop it. However, you've still missed the primary reason I suggested _scratch_mount_options in the first place: what do we do instead of copy'n'paste of random code fragments? Cheers, Dave.
> Ok, SCRATCH_OPTIONS might not be the best idea here, so feel free to > drop it. I have dropped $SCRATCH_OPTIONS. (waiting to submit v8). Thanks. > However, you've still missed the primary reason I suggested > _scratch_mount_options in the first place: I am shocked. What was missed ? Cheers, Anand -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 19, 2015 at 11:16:10AM +0800, anand jain wrote: > > > >Ok, SCRATCH_OPTIONS might not be the best idea here, so feel free to > >drop it. > > I have dropped $SCRATCH_OPTIONS. (waiting to submit v8). Thanks. > > >However, you've still missed the primary reason I suggested > >_scratch_mount_options in the first place: > > I am shocked. What was missed ? Answering the question I asked: Q: "what do we do instead of copy'n'paste of random code fragments?" Think on it, because the answer to that question (and it's not a hard one) should tell you exactly what you need to do. -Dave.
(thanks for the off-ML emails from the people who helped me
to understand).
Dave,
looks like you are suggesting something like..
-------
+_dmerror_mount_options()
+{
+ _scratch_options mount
+ echo $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS \
+ $* $DMERROR_DEV $SCRATCH_MNT
+}
+
+_dmerror_mount()
+{
+ _mount -t $FSTYP `_dmerror_mount_options $*`
+}
--------
Sorry that the word 'should mirror _scratch_mount()' confused me.
Pls clarify.
Thanks, Anand
>>> +_mount_dmerror()
>>> +{
>>> + $MOUNT_PROG -t $FSTYP $MOUNT_OPTIONS $DMERROR_DEV $SCRATCH_MNT
>>> +}
>> Should mirror _scratch_mount.
>>
>> _mount -t $FSTYP `_scratch_mount_options` $DMERROR_DEV $SCRATCH_MNT
> So I am proposing..
> + _mount -t $FSTYP $SCRATCH_OPTIONS $MOUNT_OPTIONS
> $SELINUX_MOUNT_OPTIONS $* $DMERROR_DEV $SCRATCH_MNT
However, you've still missed the primary reason I suggested
_scratch_mount_options in the first place: what do we do instead of
copy'n'paste of random code fragments?
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 20, 2015 at 03:09:05PM +0800, Anand Jain wrote: > > (thanks for the off-ML emails from the people who helped me > to understand). > > Dave, > > looks like you are suggesting something like.. > > ------- > +_dmerror_mount_options() > +{ > + _scratch_options mount > + echo $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS \ > + $* $DMERROR_DEV $SCRATCH_MNT > +} > + > +_dmerror_mount() > +{ > + _mount -t $FSTYP `_dmerror_mount_options $*` > +} > -------- Very close. :) You've got the structure right, but missed one more thing: the options are still cut'n'paste from _scratch_mount_options, and so should be /factored/ into a common function... _scratch_options is only useful to XFS to set external log/realtime devices, and we don't want to do that for the dmerror device. Everthing else is almost the same as _scratch_mount_options(), however. So: # Used for mounting non-scratch devices (e.g. loop, dm constructs) # with the safe set of scratch mount options (e.g. loop image may be # hosted on $SCRATCH_DEV, so can't use external scratch devices). _common_dev_mount_options() { echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $* } _scratch_mount_options() { _scratch_options mount echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \ $SCRATCH_DEV $SCRATCH_MNT } _dmerror_mount_options() { echo `_common_dev_mount_options $*` $DMERROR_DEV $SCRATCH_MNT } And to demonstrate why factoring like this is useful, this can also be applied to _mount_flakey(), and we could also replace most of the open-coded loop device mounts with a generic "mount device" function like: _mount_dev() { _mount `_common_dev_mount_options $*` } and call it like: _mount_dev $LOOP_DEV $MNT_POINT or even _mount_dev -o loop $MNT_POINT We can then take it further: if we have a _mount_dev() wrapper we can factor _common_dev_mount_options() from all the _*_mount_options() functions. i.e: _scratch_mount_options() { _scratch_options mount echo $SCRATCH_OPTIONS $SCRATCH_DEV $SCRATCH_MNT } _scratch_mount() { _mount_dev $* `_scratch_mount_options` } The result is we end up with a much cleaner set of mount functions that are more generic, more extensible and easier to use, as well as being more maintainable.... > Sorry that the word 'should mirror _scratch_mount()' confused me. Ok, I'll try to be more clear in future. Cheers, Dave.
Dave, On 08/21/2015 05:45 AM, Dave Chinner wrote: > On Thu, Aug 20, 2015 at 03:09:05PM +0800, Anand Jain wrote: >> >> (thanks for the off-ML emails from the people who helped me >> to understand). >> >> Dave, >> >> looks like you are suggesting something like.. >> >> ------- >> +_dmerror_mount_options() >> +{ >> + _scratch_options mount >> + echo $SCRATCH_OPTIONS $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS \ >> + $* $DMERROR_DEV $SCRATCH_MNT >> +} >> + >> +_dmerror_mount() >> +{ >> + _mount -t $FSTYP `_dmerror_mount_options $*` >> +} >> -------- > > Very close. :) > > You've got the structure right, but missed one more thing: the > options are still cut'n'paste from _scratch_mount_options, and so > should be /factored/ into a common function... > > _scratch_options is only useful to XFS to set external log/realtime > devices, and we don't want to do that for the dmerror device. > Everthing else is almost the same as _scratch_mount_options(), > however. So: > > # Used for mounting non-scratch devices (e.g. loop, dm constructs) > # with the safe set of scratch mount options (e.g. loop image may be > # hosted on $SCRATCH_DEV, so can't use external scratch devices). > _common_dev_mount_options() > { > echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $* > } > > _scratch_mount_options() > { > _scratch_options mount > > echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \ > $SCRATCH_DEV $SCRATCH_MNT > > } > _dmerror_mount_options() > { > echo `_common_dev_mount_options $*` $DMERROR_DEV $SCRATCH_MNT > } this would make test cases nice and sleek. Thanks. I have this in v8. > And to demonstrate why factoring like this is useful, this can also > be applied to _mount_flakey(), and we could also replace most of the > open-coded loop device mounts with a generic "mount device" function > like: yes indeed. I suggest if these could be handled in a separate patch as this would deviate from the intentions of this patch set ? Thanks, Anand > _mount_dev() > { > _mount `_common_dev_mount_options $*` > } > > and call it like: > > _mount_dev $LOOP_DEV $MNT_POINT > > or even > > _mount_dev -o loop $MNT_POINT > > We can then take it further: if we have a _mount_dev() wrapper we > can factor _common_dev_mount_options() from all the > _*_mount_options() functions. i.e: > > _scratch_mount_options() > { > _scratch_options mount > > echo $SCRATCH_OPTIONS $SCRATCH_DEV $SCRATCH_MNT > } > > _scratch_mount() > { > _mount_dev $* `_scratch_mount_options` > } > > The result is we end up with a much cleaner set of mount functions > that are more generic, more extensible and easier to use, as well as > being more maintainable.... > >> Sorry that the word 'should mirror _scratch_mount()' confused me. > > Ok, I'll try to be more clear in future. > > Cheers, > > Dave. > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/common/dmerror b/common/dmerror new file mode 100644 index 0000000..928e998 --- /dev/null +++ b/common/dmerror @@ -0,0 +1,69 @@ +##/bin/bash +# +# Copyright (c) 2015 Oracle. All Rights Reserved. +# +# 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. +# +# This program is distributed in the hope that it would 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, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +# +# +# common functions for setting up and tearing down a dmerror device + +_init_dmerror() +{ + $DMSETUP_PROG remove error-test > /dev/null 2>&1 + + local BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` + + DMERROR_DEV='/dev/mapper/error-test' + + DMLINEAR_TABLE="0 $BLK_DEV_SIZE linear $SCRATCH_DEV 0" + + $DMSETUP_PROG create error-test --table "$DMLINEAR_TABLE" || \ + _fatal "failed to create dm linear device" + + DMERROR_TABLE="0 $BLK_DEV_SIZE error $SCRATCH_DEV 0" +} + +_scratch_mkfs_dmerror() +{ + $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $DMERROR_DEV >> $seqres.full 2>&1 || \ + _fatal "failed to create mkfs.btrfs $* $DMERROR_DEV" +} + +_mount_dmerror() +{ + $MOUNT_PROG -t $FSTYP $MOUNT_OPTIONS $DMERROR_DEV $SCRATCH_MNT +} + +_unmount_dmerror() +{ + $UMOUNT_PROG $SCRATCH_MNT +} + +_cleanup_dmerror() +{ + $UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1 + $DMSETUP_PROG remove error-test > /dev/null 2>&1 +} + +_load_dmerror_table() +{ + $DMSETUP_PROG suspend error-test + [ $? -ne 0 ] && _fatal "failed to suspend error-test" + + $DMSETUP_PROG load error-test --table "$DMERROR_TABLE" + [ $? -ne 0 ] && _fatal "failed to load error table error-test" + + $DMSETUP_PROG resume error-test + [ $? -ne 0 ] && _fatal "failed to resume error-test" +} diff --git a/common/rc b/common/rc index 70d2fa8..8d4da0e 100644 --- a/common/rc +++ b/common/rc @@ -1337,6 +1337,15 @@ _require_sane_bdev_flush() fi } +# this test requires the device mapper error target +# +_require_dmerror() +{ + _require_command "$DMSETUP_PROG" dmsetup + $DMSETUP_PROG targets | grep error >/dev/null 2>&1 + [ $? -ne 0 ] && _notrun "This test requires dm error support" +} + # this test requires the device mapper flakey target # _require_dm_flakey()