Hi Eric,
Thanks for you can review this, as I know you're very busy recently:)
On Fri, May 27, 2016 at 01:05:15PM -0500, Eric Sandeen wrote:
> On 5/27/16 10:21 AM, Zorro Lang wrote:
> > XFS add more configurations in /sys/fs/xfs recently. For use
> > them, this patch add some common functions for:
> > 1. "require" a file/dir in /sys/fs.
> > 2. write a file in /sys/fs.
>
> More specifically, in /sys/fs/${FSTYP}
Sure, I should specify this:)
>
> > For common use, these functions can be used by other filesystems.
> >
> > Signed-off-by: Zorro Lang <zlang@xxxxxxxxxx>
> > ---
> > common/rc | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 81 insertions(+)
> >
> > diff --git a/common/rc b/common/rc
> > index 51092a0..9935f08 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3556,6 +3556,87 @@ run_fsx()
> > fi
> > }
> >
> > +_require_sys_fs()
> > +{
> > + local dev=$1
> > + local target=$2
> > + local dname=""
> > +
> > + if [ ! -b "$dev" -o -z "$target" ];then
> > + echo "Usage: _require_sys_fs <device> <target_need_exist>"
> > + exit 1
> > + fi
> > +
> > + dname=$(basename $(readlink -f $dev))
> > + _mkfs_dev $dev > /dev/null 2>&1
>
> we don't want to mkfs the test dev just for a _require test; that
> device is supposed to age, and this could possibly take a very long
> time, as well. And in general a mkfs should not be required to test
> the existence of a sysfs tunable, I think?
see below
>
> > + _mount -t $FSTYP `_common_dev_mount_options` $dev $SCRATCH_MNT
> > + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> > + umount $SCRATCH_MNT
> > + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> > + fi
> > + umount $SCRATCH_MNT
> > +}
> > +
> > +_require_scratch_sys_fs()
> > +{
> > + local target=$1
> > + local dname=""
> > +
> > + if [ ! -b "$SCRATCH_DEV" -o -z "$target" ];then
> > + echo "Usage: _require_scratch_sys_fs <target_need_exist>"
> > + exit 1
> > + fi
> > +
> > + dname=$(basename $(readlink -f $SCRATCH_DEV))
> > + _scratch_mkfs > /dev/null 2>&1
> > + _scratch_mount
> > + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> > + _scratch_unmount
> > + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> > + fi
> > + _scratch_unmount
> > +}
>
> This seems overly complex; do you need both of these functions? We are only
> testing one filesystem type at a time, and $TEST_DEV is always required.
> What is the reason for the two differing functions?
>
> Isn't _require_sys_fs $SCRATCH_DEV exactly the same as
> _require_scratch_sys_fs ?
>
Sorry for the "complex", let me explain:
1. At first, I write a function _require_sys_fs(), which used to check
all kinds of device's /sys/fs/fstype configurations. Include dmerror,
dmthinp and what ever other devices(not only TEST_DEV, SCRATCH_DEV).
2. Then I want to add a _require_scratch_sys_fs() function, which only
used for SCRATCH_DEV. Because I feel generally if SCRATCH_DEV support
a /sys/fs/fstype configurations, that means the working kernel support
it. And _require_sys_fs() can be used by other devices write their
_require_XXX_sys_fs() function(if needed... Looks like I thought too
much....).
3. When I wrote _require_scratch_sys_fs(), I want to use _require_sys_fs
$SCRATCH_DEV directly. But I saw _scratch_mkfs() use different options
with common mkfs_dev() function. So I use _scratch_mkfs and _scratch_mount
to write this _require_scratch_sys_fs().
4. About why I need mkfs before in this _require function, at first those
/sys/fs/fstype configurations only can be used after mount. The second
is I hope _require_sys_fs() can be used by other devices, which maybe
not mkfs... So....
> Also, because $SCRATCH_DEV is not required, $SCRATCH_MNT
> isn't either, so I don't think you can use $SCRATCH_MNT.
I did this according to _require_scratch_xfs_crc() function.
>
> Also :) We may one day want to require sysfs files which
> are not in the fs/ namespace, so it should probably
> be named according to the namespace.
>
> So maybe something like this (not tested)
>
> + /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
> +_require_fs_sys_fs()
> +{
> + local dev=$1
> + local target=$2
> + local dname=""
> + local tmp_mnt=`mktemp -d`
> +
> + if [ ! -b "$dev" -o -z "$target" ];then
> + echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
> + exit 1
> + fi
> +
> + dname=$(basename $(readlink -f $dev))
> + _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> + umount $tmp_mnt
> + rm -f $tmp_mnt
> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> + fi
> + umount $SCRATCH_MNT
I think you mean "umount $tmp_mnt" at here
> + rm -f $tmp_mnt
> +}
>
> and it would work for either SCRATCH or TEST?
hmm.. That's OK. I can't sure if mkfs before mount is better or not. If we
don't mkfs in this function, we ask the caller keep sure the $dev has been
mkfs. I think both are OK for me, I will follow your idea:)
>
> > +_set_sys_fs_param()
> > +{
> > + local dev=$1
> > + shift
> > + local target=$1
> > + shift
> > + local content="$*"
> > +
> > + if [ ! -b "$dev" -o -z "$target" -o -z "$content" ];then
> > + echo "Usage: _set_sys_fs_param <mounted_device> <target> <content>"
> > + exit 1
> > + fi
> > + local dname=$(basename $(readlink -f $dev))
> > + echo "$content" > /sys/fs/${FSTYP}/${dname}/$target
> > +}
>
> This seems fine. Normally I'd say this could just be open coded
> in the test, but the dname=$(basename $(readlink -f $dev)) is worth
> encapsulating in the helper.
Do you mean I should do?:
local dname=$(readlink -f $dev)
dname=`basename $dname`
>
> > +_enable_xfs_fail_at_unmount()
> > +{
> > + local dev=$1
> > +
> > + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
> > + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
> > + exit 1
> > + fi
> > +
> > + _set_sys_fs_param $dev error/fail_at_unmount 1
> > +}
> > +
> > +_disable_xfs_fail_at_unmount()
> > +{
> > + local dev=$1
> > +
> > + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
> > + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
> > + exit 1
> > + fi
> > +
> > + _set_sys_fs_param $dev error/fail_at_unmount 0
> > +}
>
> I don't think this is needed; it's easy enough to just call:
>
> _set_fs_sys_fs_param $dev error/fail_at_unmount 0
> _set_fs_sys_fs_param $dev error/fail_at_unmount 1
>
> directly from tests. There are a lot of sysfs tunables, writing wrappers
> for each one isn't really that helpful.
Yeah, except they can check if "$FSTYP" = "xfs", they don't do
more than use _set_fs_sys_fs_param directly.
Thanks,
Zorro
>
> Thanks,
> -Eric
>
> > init_rc
>
> >
> > ################################################################################
> >
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
