On Thu, Oct 18, 2018 at 5:37 AM zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
>
> On 2018/10/16 17:45, Amir Goldstein Wrote:
> > On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
[...]
> >
> > Please consider the name _overlay_repair_dirs() with reference to
> > _repair_scratch_fs(), which is used for roughly the same purpose.
> >
>
> _run_check_fsck() is helper used to test fsck and may expect to return
> "error" exit code when we do exception tests(see patch 4), but
> _repair_scratch_fs() always want to return "correct" exit code.
>
Right. so perhaps we need _overlay_repair_dirs() as a convenience
helper for things like the stress test and also related to another
comment about expecting return 0 is too fragile.
> > BTW, the tests generic/330 generic/332 when run with -overlay
> > over xfs with reflink support seem to call _repair_scratch_fs() which does:
> > # Let's hope fsck -y suffices...
> > fsck -t $FSTYP -y $SCRATCH_DEV 2>&1
> > local res=$?
> > case $res in
> > 0|1|2)
> > res=0
> > ;;
> > *)
> > _dump_err2 "fsck.$FSTYP failed, err=$res"
> >
> > How come fsck.overlay is not complaining about missing arguments??
> >
> > The rest of the generic tests that call _repair_scratch_fs() have
> > _require_dm_target() which _require_block_device(), so don't run with overlay.
> >
> > If you do sort this out and add overlay support to
> > _repair_scratch_fs() its probably
> > worth replacing 0|1|2) with FSCK_ constants.
> >
>
> Thanks for pointing this out, I think we do something like below can fix
> this problem, what do you think?
I am puzzled about why those tests do NOT fail when fsck.overlay is given
incorrect args??
>
> diff --git a/common/overlay b/common/overlay
> index 2896594..b9fe1ee 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -226,6 +226,14 @@ _run_check_fsck()
> echo "fsck return unexpected value:$expect_ret,$fsck_ret"
> }
>
> +_scratch_overlay_repair()
> +{
> + _overlay_fsck_dirs $OVL_BASE_SCRATCH_MNT/$OVL_LOWER \
> + $OVL_BASE_SCRATCH_MNT/$OVL_UPPER \
> + $OVL_BASE_SCRATCH_MNT/$OVL_WORK \
> + -y
> +}
> +
All overlay "global" helpers should be prefixed _overlay_XXX
unless there is a good reason to break that rule (even though we did
not always abide by this rule in the past).
But anyway, I think that a more specific _overlay_repair_scratch_fs()
should be used to hide the details of "correct" error code from common/rc.
And it would probably be useful if it used a helper _overlay_repair_dirs()
that can be used from overlay specific tests (like stress test).
Thanks,
Amir.
