On Mon, Mar 12, 2018 at 10:44 AM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote: > On 2018/3/11 16:12, Amir Goldstein Wrote: >> On Thu, Mar 1, 2018 at 3:11 PM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote: >>> On 2018/3/1 21:03, Amir Goldstein Wrote: >>>> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote: >>>>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs >>>>> for checking consistency of underlying dirs of overlay filesystem. >>>>> These helpers works only if fsck.overlay exists. >>>>> >>>>> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like >>>>> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in >>>>> common/rc to detect a dir is a mount point or not. >>>>> >>>>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ] >>>>> >>>>> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx> >>>>> --- >>>>> README.overlay | 10 ++++-- >>>>> common/config | 4 +++ >>>>> common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> common/rc | 18 +++++++++-- >>>>> 4 files changed, 127 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/README.overlay b/README.overlay >>>>> index dfb8234..3cbefab 100644 >>>>> --- a/README.overlay >>>>> +++ b/README.overlay >>>>> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run. >>>>> An easy way to accomplish this is by running './check <some test>' once, >>>>> before running './check -overlay'. >>>>> >>>>> +'./check -overlay' support check overlay test and scratch dirs, >>>>> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck >>>>> +options need to given directly. >>>>> + >>>>> Because of the lack of mkfs support, multi-section config files are only >>>>> partly supported with './check -overlay'. Only multi-section files that >>>>> do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay. >>>>> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options: >>>>> MOUNT_OPTIONS="-o pquota" >>>>> TEST_FS_MOUNT_OPTS="-o noatime" >>>>> OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off" >>>>> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off" >>>> >>>> typo for the phony options. I believe it is going to be: "-n -o >>>> redirect_dir=off" >>> >>> Oh, sorry for the mistake, it is "-n -o redirect_dir=off" >>> >> >> Zhangyi, >> >> One thing that has occurred to me now, which should be fixed before >> first release >> is that fsck.overlay should return an error for unknown/unsupported options >> (e.g. fsck.overlay -o blah doesn't complain). >> If this doesn't happen for first release then in the future we will >> not be able to >> write test scripts to check if installed fsck.overlay version supports >> feature X. >> > Yes, It's time to impliment the feature/option set and do the corresponding > work for the already known features. I am writing the kernel side feature set > recent days, I will add these feature into fsck.overlay after kernel's work done. > > BTW, the feature set in kernel do the following, some suggestions? > 1) Add feature set into upper root xattr "trusted.overlay.feature_[compat|imcompat|rocompat]" > if user give mount options "xxx=on" or some feature xattr/dir detected. > 2) If unknown imcompat feature in xattr detected, fall back to try to ro-mount. fail mount for unknown incompat > 3) If unknown ro-compat featrure in xattr detected, mount fail. fallback to ro-mount (see ext4_feature_set_ok()) > >> About the possible meaning of <feature>=on/off and the meaning of <defaults> >> in the context of fsck.overlay, I think it is worth a separate >> discussion, but the >> way I see it: >> - When fsck.overlay *supports* a <feature> it should be able to detect if that >> <feature> was ever enabled on overlay (e.g. known xattr exists or >> index dir exists). > > Agree, and we can also fix feature set when detect feature. > >> - When fsck.overlay detects an unknown <feature> it should abort > > Agree. A little more detail: if user specify upper and work dir, check imcompat > unknown feature; if not, check ro-imcompat unknown feature. If detected, fsck > about. > No exactly. The difference between ext4 kernel driver and e2fsck - ext4 *allows* to mount with unknown compat features and *allows* to mount ro with unknonw ro-compat features. e2fsck *aborts* for any unknown compat/rocompat/incomapt feature Meaning that you MAY be able to mount new ext4 with old kernel, but you CANNOT fsck new ext4 with old e2fsck >> - -o <feature>=off means wipe all traces of <feature> from overlay and fix if >> necessary (e.g. make a copy of redirect dir before removing >> redirect_dir xattr) > > Agree. But not sure it is easy to implement of making a copy of redirect dir > when redirect_dir=off. Because it modify the directory hierarchy, it may affect > the origin/nlink xattr in files/dirs in index dir when index=on or nfs_export=on, > maybe also affect some other feature future. > This is why fsck MUST refuse to run if it detects an unknown feature, even if that feature is "compatible" for rw mounting, fixing may break it. And about disabling redirect_dir, yes, it means that all renamed directories are replaced with pure opaque (not indexed) directories and all the content from merged dir needs to be copied up recursively to the new directory. This is not trivial and may be time consuming, but with underlying fs clone support it is also not that bad. In fact, if you try to 'mv dir newdir' on overlay without redirect_dir, the 'mv' tool already does that fallback to create new dir and recursive rename. >> - -o <feature>=on means bring overlay to a "feature consistent state", >> as if feature >> was enabled from day 1 (e.g. index all copies up lower hardlinks) > > Agree. > >> - If <feature>=[on/off] is not specified, then if <feature> is not >> detected, it should >> not be explicitly enabled and if feature is detected it should be >> made consistent > > Agree. > >> That last rule is certainly debatable, so maybe best if we leave that >> decision to >> admin via default policy configuration file. >> > > Something like /etc/mke2fs.conf but not for mkfs? Do you mean if feature > detected conflict with this config(eg: we detect redirect dir but "redirect_dir=off" > was setted in ths config file), fsck should check and fix fs according to this > config file? > > I think it may confusion and conflict with mount options(eg: redirect_dir=on) > previous mount or next mount. And IIUC, there is no such feature config file for > ext4/xfs, fsck.ext4 will detect feature from super block and fix fs according to > this detected result. So I think it's better to detect feature feature by fsck > itself, keep features as it was. > I agree it is best to not ask user anything because user probably doesn't know the answer anyway. If you follow the discussion that has started on adding a config file for mkfs.xfs, you'll see it is mainly intended to be used by distributions. Imagine a disro that set the kernel config OVERLAY_FS_INDEX with the intention that all overlays will be indexed. This distro may want to make the default behavior of fsck.overlay to construct an index by default. For you, the role of a config file is to reduce "bike shedding" - if there are conflicting options about what should be done in case feature X is detected or not detected and option X=on/off is not specified, instead of debating what the right thing to do by default, you delegate the decision to "config file" and set the hard coded default to what *you* think is the best behavior. So my advise to you is to add a config file if and only if (or when) it becomes beneficial to you for reconciling different opinions. Cheers, Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
