On 2018/3/12 17:19, Amir Goldstein Wrote: > 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()) > You are right, I get it. >> >>> 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 > Yes, you are right, I see this check in e2fsck/unix.c. >>> - -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. > Right. > 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. > Yes, we can do the same thing like "mv" does when overlayfs doesn't support rename syscall for dir(redirect_dir=off). >>> - -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. > Yes, I get your point, It's a good point to avoid debate, and seems no apparent defects. I'd like to add this file if no different opinions. Thanks, Yi. -- 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
