| Message ID | 20181016074559.24728-3-yi.zhang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | overlay: enhance fsck.overlay test cases | expand |
On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote: > > Some valid test cases about fsck.overlay may be not valid enough now, > they lose the impure xattr on the parent directory of the simluated > redirect directory, and lose the whiteout which use to cover the origin > lower object. Then fsck.overlay will fix these two inconsistency which > are not those test cases want to cover, thus it will lead to > fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by > complement the missing overlay related features. > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > --- Ok. I think it's fine if we merge this fix now, but this way it is going to be quite hard to maintain this test. Imagine every time that you add another feature to fsck.overlay, say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT and break this test. Perhaps it would have been better to construct the test cases by: - mount overlay - create some copied up/ redirected dirs and whiteouts - umount overlay - make minor modifications to upper/lower layer - run fsck Then you wouldn't need to worry about things like impure parent dir and future overlay features. I will leave it to you to decide if you want to fix this now or the next time around... Thanks, Amir.
On 2018/10/16 17:26, Amir Goldstein Wrote: > On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote: >> >> Some valid test cases about fsck.overlay may be not valid enough now, >> they lose the impure xattr on the parent directory of the simluated >> redirect directory, and lose the whiteout which use to cover the origin >> lower object. Then fsck.overlay will fix these two inconsistency which >> are not those test cases want to cover, thus it will lead to >> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by >> complement the missing overlay related features. >> >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> >> --- > > Ok. I think it's fine if we merge this fix now, but this way it is going > to be quite hard to maintain this test. > > Imagine every time that you add another feature to fsck.overlay, > say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT > and break this test. > > Perhaps it would have been better to construct the test cases by: > - mount overlay > - create some copied up/ redirected dirs and whiteouts > - umount overlay > - make minor modifications to upper/lower layer > - run fsck > > Then you wouldn't need to worry about things like impure parent dir > and future overlay features. > > I will leave it to you to decide if you want to fix this now or the > next time around... > Indeed, I thought about this choice. If we create overlay on-disk features (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes non-independent. It will depends on the kernel (overlayfs module) user are running the test. Imaging if user want to test the latest fsck.overlay on the old kernel which contains a compatible feature xattr fsck.overlay know but the kernel don't, we will get the unexpected result. Or maybe we can add some _require_xxx_feature() helper to enforce user doing test on the kernel which supports the specified feature? Thanks, Yi.
On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote: > > On 2018/10/16 17:26, Amir Goldstein Wrote: > > On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote: > >> > >> Some valid test cases about fsck.overlay may be not valid enough now, > >> they lose the impure xattr on the parent directory of the simluated > >> redirect directory, and lose the whiteout which use to cover the origin > >> lower object. Then fsck.overlay will fix these two inconsistency which > >> are not those test cases want to cover, thus it will lead to > >> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by > >> complement the missing overlay related features. > >> > >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > >> --- > > > > Ok. I think it's fine if we merge this fix now, but this way it is going > > to be quite hard to maintain this test. > > > > Imagine every time that you add another feature to fsck.overlay, > > say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT > > and break this test. > > > > Perhaps it would have been better to construct the test cases by: > > - mount overlay > > - create some copied up/ redirected dirs and whiteouts > > - umount overlay > > - make minor modifications to upper/lower layer > > - run fsck > > > > Then you wouldn't need to worry about things like impure parent dir > > and future overlay features. > > > > I will leave it to you to decide if you want to fix this now or the > > next time around... > > > > Indeed, I thought about this choice. If we create overlay on-disk features > (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes > non-independent. It will depends on the kernel (overlayfs module) user are > running the test. Imaging if user want to test the latest fsck.overlay > on the old kernel which contains a compatible feature xattr fsck.overlay > know but the kernel don't, we will get the unexpected result. Or maybe > we can add some _require_xxx_feature() helper to enforce user doing test > on the kernel which supports the specified feature? > I think the only sane choice is for this test to relax the expectation of 0 exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid" test cases. Maybe the only fsck run that we are fine with expecting 0 exit code is -n run. As you can see this is common practice for e2fsck: e2fsck -fn "${SCRATCH_DEV}" >> $seqres.full 2>&1 || _fail "fsck should not fail" Thanks, Amir.
On 2018/10/18 12:44, Amir Goldstein Wrote: > On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote: >> >> On 2018/10/16 17:26, Amir Goldstein Wrote: >>> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote: >>>> >>>> Some valid test cases about fsck.overlay may be not valid enough now, >>>> they lose the impure xattr on the parent directory of the simluated >>>> redirect directory, and lose the whiteout which use to cover the origin >>>> lower object. Then fsck.overlay will fix these two inconsistency which >>>> are not those test cases want to cover, thus it will lead to >>>> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by >>>> complement the missing overlay related features. >>>> >>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> >>>> --- >>> >>> Ok. I think it's fine if we merge this fix now, but this way it is going >>> to be quite hard to maintain this test. >>> >>> Imagine every time that you add another feature to fsck.overlay, >>> say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT >>> and break this test. >>> >>> Perhaps it would have been better to construct the test cases by: >>> - mount overlay >>> - create some copied up/ redirected dirs and whiteouts >>> - umount overlay >>> - make minor modifications to upper/lower layer >>> - run fsck >>> >>> Then you wouldn't need to worry about things like impure parent dir >>> and future overlay features. >>> >>> I will leave it to you to decide if you want to fix this now or the >>> next time around... >>> >> >> Indeed, I thought about this choice. If we create overlay on-disk features >> (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes >> non-independent. It will depends on the kernel (overlayfs module) user are >> running the test. Imaging if user want to test the latest fsck.overlay >> on the old kernel which contains a compatible feature xattr fsck.overlay >> know but the kernel don't, we will get the unexpected result. Or maybe >> we can add some _require_xxx_feature() helper to enforce user doing test >> on the kernel which supports the specified feature? >> > > I think the only sane choice is for this test to relax the expectation of 0 > exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid" > test cases. > The meaning of the "valid" test cases is to make sure fsck.overlay will never change the on-disk filesystem if the feature(xattr) we want to test is valid, so the FSCK_OK and FSCK_NONDESTRUCT is totally different. If we relax the expectation of 0(FSCK_OK) exit code, we couldn't distinguish the fsck was changed the file system or not, if so, we also couldn't distinguish it's caused by some bugs of fsck or the base dirs were not valid enough. Then the "valid" test cases cannot catch fsck's fault accurately. So I think make a valid enough overlay image manually now is still the best way. I think maybe after we introduce "feature set" xattr, it will becomes much easier, fsck.overlay will fix things according to feature set, and we create overlay image through mkfs.overlay. So we could disable some irrelevant features to avoid disturbing our tests. Is it fine? Thanks, Yi. > Maybe the only fsck run that we are fine with expecting 0 exit code is > -n run. As you can see this is common practice for e2fsck: > e2fsck -fn "${SCRATCH_DEV}" >> $seqres.full 2>&1 || _fail "fsck should not fail" > > Thanks, > Amir. > > . >
On Fri, Oct 19, 2018 at 3:36 PM zhangyi (F) <yi.zhang@huawei.com> wrote: > > On 2018/10/18 12:44, Amir Goldstein Wrote: > > On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote: > >> > >> On 2018/10/16 17:26, Amir Goldstein Wrote: > >>> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote: > >>>> > >>>> Some valid test cases about fsck.overlay may be not valid enough now, > >>>> they lose the impure xattr on the parent directory of the simluated > >>>> redirect directory, and lose the whiteout which use to cover the origin > >>>> lower object. Then fsck.overlay will fix these two inconsistency which > >>>> are not those test cases want to cover, thus it will lead to > >>>> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by > >>>> complement the missing overlay related features. > >>>> > >>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > >>>> --- > >>> > >>> Ok. I think it's fine if we merge this fix now, but this way it is going > >>> to be quite hard to maintain this test. > >>> > >>> Imagine every time that you add another feature to fsck.overlay, > >>> say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT > >>> and break this test. > >>> > >>> Perhaps it would have been better to construct the test cases by: > >>> - mount overlay > >>> - create some copied up/ redirected dirs and whiteouts > >>> - umount overlay > >>> - make minor modifications to upper/lower layer > >>> - run fsck > >>> > >>> Then you wouldn't need to worry about things like impure parent dir > >>> and future overlay features. > >>> > >>> I will leave it to you to decide if you want to fix this now or the > >>> next time around... > >>> > >> > >> Indeed, I thought about this choice. If we create overlay on-disk features > >> (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes > >> non-independent. It will depends on the kernel (overlayfs module) user are > >> running the test. Imaging if user want to test the latest fsck.overlay > >> on the old kernel which contains a compatible feature xattr fsck.overlay > >> know but the kernel don't, we will get the unexpected result. Or maybe > >> we can add some _require_xxx_feature() helper to enforce user doing test > >> on the kernel which supports the specified feature? > >> > > > > I think the only sane choice is for this test to relax the expectation of 0 > > exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid" > > test cases. > > > The meaning of the "valid" test cases is to make sure fsck.overlay will never > change the on-disk filesystem if the feature(xattr) we want to test is valid, > so the FSCK_OK and FSCK_NONDESTRUCT is totally different. > > If we relax the expectation of 0(FSCK_OK) exit code, we couldn't distinguish > the fsck was changed the file system or not, if so, we also couldn't distinguish > it's caused by some bugs of fsck or the base dirs were not valid enough. Then > the "valid" test cases cannot catch fsck's fault accurately. So I think make > a valid enough overlay image manually now is still the best way. > > I think maybe after we introduce "feature set" xattr, it will becomes much easier, > fsck.overlay will fix things according to feature set, and we create overlay image > through mkfs.overlay. So we could disable some irrelevant features to avoid > disturbing our tests. Is it fine? > Its fine by me to re-open this for discussion that next time fsck.overlay changes and breaks the test. Thanks, Amir.
diff --git a/tests/overlay/045 b/tests/overlay/045 index ed23258..ffca47f 100755 --- a/tests/overlay/045 +++ b/tests/overlay/045 @@ -37,6 +37,7 @@ _require_attrs _require_command "$FSCK_OVERLAY_PROG" fsck.overlay OVL_XATTR_OPAQUE_VAL=y +OVL_XATTR_IMPURE_VAL=y # remove all files from previous tests _scratch_mkfs @@ -69,6 +70,15 @@ make_opaque_dir() $SETFATTR_PROG -n $OVL_XATTR_OPAQUE -v $OVL_XATTR_OPAQUE_VAL $target } +# Create impure directories +make_impure_dir() +{ + for dir in $*; do + mkdir -p $dir + $SETFATTR_PROG -n $OVL_XATTR_IMPURE -v $OVL_XATTR_IMPURE_VAL $dir + done +} + # Create a redirect directory make_redirect_dir() { @@ -155,8 +165,9 @@ echo "+ Valid whiteout(2)" make_test_dirs mkdir $lowerdir/origin touch $lowerdir/origin/foo +make_impure_dir $upperdir make_redirect_dir $upperdir/testdir "origin" -make_whiteout $upperdir/testdir/foo +make_whiteout $upperdir/origin $upperdir/testdir/foo _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p check_whiteout $upperdir/testdir/foo @@ -169,7 +180,8 @@ mkdir -p $lowerdir2/origin/subdir touch $lowerdir2/origin/subdir/foo make_redirect_dir $lowerdir/testdir "origin" mkdir -p $upperdir/testdir/subdir -make_whiteout $upperdir/testdir/subdir/foo +make_whiteout $lowerdir/origin $upperdir/testdir/subdir/foo +make_impure_dir $upperdir/testdir $upperdir _run_check_fsck $FSCK_OK "$lowerdir:$lowerdir2" $upperdir $workdir -p check_whiteout $upperdir/testdir/subdir/foo @@ -195,7 +207,8 @@ mkdir $lowerdir/origin touch $lowerdir/origin/foo make_opaque_dir $upperdir/testdir make_redirect_dir $upperdir/testdir/subdir "/origin" -make_whiteout $upperdir/testdir/subdir/foo +make_whiteout $upperdir/origin $upperdir/testdir/subdir/foo +make_impure_dir $upperdir/testdir _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p check_whiteout $upperdir/testdir/subdir/foo diff --git a/tests/overlay/046 b/tests/overlay/046 index 21645c1..ea4b14f 100755 --- a/tests/overlay/046 +++ b/tests/overlay/046 @@ -40,6 +40,16 @@ _require_command "$FSCK_OVERLAY_PROG" fsck.overlay _scratch_mkfs OVL_XATTR_OPAQUE_VAL=y +OVL_XATTR_IMPURE_VAL=y + +# Create impure directories +make_impure_dir() +{ + for dir in $*; do + mkdir -p $dir + $SETFATTR_PROG -n $OVL_XATTR_IMPURE -v $OVL_XATTR_IMPURE_VAL $dir + done +} # Create a redirect directory make_redirect_dir() @@ -140,6 +150,7 @@ make_test_dirs mkdir $lowerdir/origin make_whiteout $upperdir/origin make_redirect_dir $upperdir/testdir "origin" +make_impure_dir $upperdir _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p check_redirect $upperdir/testdir "origin" @@ -151,6 +162,7 @@ make_test_dirs mkdir $lowerdir/origin make_whiteout $upperdir/origin make_redirect_dir $upperdir/testdir1/testdir2 "/origin" +make_impure_dir $upperdir/testdir1 _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p check_redirect $upperdir/testdir1/testdir2 "/origin" @@ -172,6 +184,7 @@ make_test_dirs mkdir $lowerdir/{testdir1,testdir2} make_redirect_dir $upperdir/testdir1 "testdir2" make_redirect_dir $upperdir/testdir2 "testdir1" +make_impure_dir $upperdir _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p check_redirect $upperdir/testdir1 "testdir2"
Some valid test cases about fsck.overlay may be not valid enough now, they lose the impure xattr on the parent directory of the simluated redirect directory, and lose the whiteout which use to cover the origin lower object. Then fsck.overlay will fix these two inconsistency which are not those test cases want to cover, thus it will lead to fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by complement the missing overlay related features. Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- tests/overlay/045 | 19 ++++++++++++++++--- tests/overlay/046 | 13 +++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-)