> -----邮件原件----- > 发件人: Amir Goldstein [mailto:amir73il@xxxxxxxxx] > 发送时间: 2024年7月15日 18:08 > 收件人: Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> > 抄送: miklos@xxxxxxxxxx; overlayfs <linux-unionfs@xxxxxxxxxxxxxxx> > 主题: Re: overlayfs issue: dir permission lost during overlayfs copy-up > > On Mon, Jul 15, 2024 at 9:07 AM Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> wrote: > > > > > > > > > -----邮件原件----- > > > 发件人: Amir Goldstein [mailto:amir73il@xxxxxxxxx] > > > 发送时间: 2024年7月12日 17:41 > > > 收件人: Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> > > > 抄送: miklos@xxxxxxxxxx; overlayfs <linux-unionfs@xxxxxxxxxxxxxxx> > > > 主题: Re: overlayfs issue: dir permission lost during overlayfs > > > copy-up > > > > > > On Fri, Jul 12, 2024 at 7:18 AM Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > Dear Amir, > > > > > > > > > > > > > > > > Seems issue disappeared with below changes, can you help review below patch? > > > > > > > > > > > > > > > > Thank you! > > > > > > > > > > > > > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > > > > > > > index 48bca5817f..e543b5563d 100644 > > > > > > > > --- a/fs/overlayfs/copy_up.c > > > > > > > > +++ b/fs/overlayfs/copy_up.c > > > > > > > > @@ -851,9 +851,11 @@ static int ovl_copy_up_one(struct dentry > > > > *parent, struct dentry *dentry, > > > > > > > > > > > > > > > > int ovl_copy_up_flags(struct dentry *dentry, int flags) > > > > > > > > { > > > > > > > > + struct super_block *sb = dentry->d_sb; > > > > > > > > int err = 0; > > > > > > > > const struct cred *old_cred; > > > > > > > > bool disconnected = (dentry->d_flags & > > > > DCACHE_DISCONNECTED); > > > > > > > > + unsigned int copies = 0; > > > > > > > > > > > > > > > > /* > > > > > > > > * With NFS export, copy up can get called for a disconnected non-dir. > > > > > > > > @@ -887,9 +889,14 @@ int ovl_copy_up_flags(struct dentry *dentry, > > > > int > > > > flags) > > > > > > > > > > > > > > > > dput(parent); > > > > > > > > dput(next); > > > > > > > > + > > > > > > > > + copies++; > > > > > > > > } > > > > > > > > ovl_revert_creds(dentry->d_sb, old_cred); > > > > > > > > > > > > > > > > + if (copies && d_is_dir(dentry) && sb->s_op->sync_fs) > > > > > > > > + sb->s_op->sync_fs(sb, 1); > > > > > > > > + > > > > > > > > > > I am not sure if it is acceptable to add sync to parent dir copy up although this should be > relatively rare so maybe its fine?? > > > but if you do add sync you should be using fsync on the copied up parent directory - not ->sync_fs. > > > > > > Anyway, this check is wrong. > > > You should not be checking for d_is_dir(dentry), you should be > > > checking if any *parents* were copied > up, > > > > > > See more about this below... > > > > > > > > > > > > > > > > > > > 发件人: Lv Fei（吕飞） > > > > 发送时间: 2024年7月12日 11:35 > > > > 收件人: 'amir73il@xxxxxxxxx' <amir73il@xxxxxxxxx> > > > > 主题: overlayfs issue: dir permission lost during overlayfs copy-up > > > > > > > > > > > > > > > > > > > > > > > > Dear Amir, > > > > > > > > > > > > > > > > Sorry to bother you. > > > > > > > > > > > > > > > > Recently, we had a problem with overlayfs dir copy-up flow. > > > > > > > > > > > > > > > > Description: > > > > > > > > If a dir eyelyn/ exist in low layer, not exist in upper layer, after creating a new file(e.g. > eyelyn/ eyelyn.log) in this dir from overlayfs, permission of eyelyn/ may be abnormal after > power-cut. > > > > > > > > If add a sync after creating a new file, permission of eyelyn/ is always correct. > > > > > > > > > > > > > > > > Kernel Version: > > > > > > > > Linux OpenWrt 5.4.276+ #25 PREEMPT Fri Jul 12 02:21:17 UTC 2024 > > > > armv7l GNU/Linux > > > > > > > > > > > > > > > > Test Step: > > > > > > > > 1. mount –t squashfs /dev/mtdblock19 /system/etc > > > > > > > > root@OpenWrt:/system/etc# ls -l > > > > > > > > drwxr-xr-x 2 root root 3 Jul 11 2024 eyelyn/ > > > > > > > > > > > > > > > > 2. mount –t ubifs ubi0:etc /overlay/etc > > > > > > > > root@OpenWrt:/overlay/etc# ls -l > > > > > > > > drwxr-xr-x 8 root root 1360 Jan 1 08:01 root/ > > > > > > > > drwxr-xr-x 3 root root 224 Jan 1 08:00 work/ > > > > > > > > root@OpenWrt:/overlay/etc# ls -al root/ > > > > > > > > drwxr-xr-x 8 root root 1360 Jan 1 08:01 ./ > > > > > > > > drwxr-xr-x 4 root root 288 Jan 1 08:00 ../ > > > > > > > > > > > > > > > > 3. mount –t overlay /system/etc -o > > > > noatime,lowerdir=/system/etc,upperdir=/overlay/etc/root,workdir=/o > > > > verl > > > > ay/etc/work > > > > > > > > > > > > > > > > 4. echo system > /system/etc /eyelyn/eyelyn.log > > > > > > > > > > > > > > > > 5. power cut > > > > > > > > > > > > > > > > 6. after next power on, sometimes dir eyelyn/ has wrong permission > > > > (d---------) > > > > > > > > > > > > > > > > mount –t ubifs ubi0:etc /overlay/etc > > > > > > > > root@OpenWrt:/overlay/etc# ls -l root/ > > > > > > > > d--------- 1 root root 232 Jan 1 08:00 eyelyn > > > > > > > > root@OpenWrt:/overlay/etc# ls –l system/etc/eyelyn/eyelyn.log > > > > > > > > -rw-r--r-- 1 root root 0 Jan 1 08:00 /system/etc/eyelyn/eyelyn.log > > > > > > > > > > > > > > > > if we add sync to step 4, that is “echo system > /system/etc /eyelyn/eyelyn.log && sync”, then > everything is right. > > > > > > > > > > > > > > > > Do you have any suggestions? > > > > > > > > > > > > > > > > > Overlayfs creates the upper dir in work directory, sets its metadata and only then moves it into > place, so the above is an "issue" with ubifs. > > > > > > The thing about this "issue" is that the behavior that after move the old permissions cannot be > observed is not defined by POSIX, but it is the facto the behavior of most of the modern filesystems > (xfs, ext4 and most probably btrfs). > > > > > > If you want to add a feature that adds fsync to copied up parent directories for filesystems like > ubifs that are not "strictly ordered metadata" then I think this needs to be an opt-in feature. > > > > > > I must admit that this requirement from the upper fs is not documented and cannot be automatically > tested by overlayfs (fs do not advertise "strictly ordered metadata" property). It just happens to > be true for most of the common fs used as upper fs. > > > > > > I wish we had called the mount option "volatile" "sync=none" and then we could have added > "sync=strict" for this and "sync=data" as the default. > > > We can still do that and have "volatile" be an alias for "sync=none". > > > > > > Thanks, > > > Amir. > > > > Very glad to receive your reply, Thank you for explanation. > > As you suggested, I try to add mount option "sync=strict", change to use fsync for parent dir. Please help have a look. > > > > Ok, but if you want to submit this change please follow https://www.kernel.org/doc/html/latest/process/submitting-patches.html Thanks, I will learn the submit flow and have a try. > See comments below > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index > > 48bca5817f..4258b8da8d 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -851,6 +851,7 @@ static int ovl_copy_up_one(struct dentry *parent, > > struct dentry *dentry, > > > > int ovl_copy_up_flags(struct dentry *dentry, int flags) { > > + struct ovl_fs *ofs = dentry->d_sb->s_fs_info; > > ofs = OVL_FS(dentry->d_sb); > > > int err = 0; > > const struct cred *old_cred; > > bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED); > > @@ -884,6 +885,24 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags) > > } > > > > err = ovl_copy_up_one(parent, next, flags); > > + if (ofs->config.volatile_sync && d_is_dir(next)) { > > + struct path upperpath; > > + struct file *new_file; > > + > > + ovl_path_upper(next, &upperpath); > > + > > + new_file = ovl_path_open(&upperpath, > > + O_LARGEFILE | O_WRONLY); > > + if (!IS_ERR(new_file)) { > > + if (ofs->config.volatile_sync == > > + OVL_VOLATILE_SYNC_DATA) > > + vfs_fsync(new_file, 1); > > Not needed already done in ovl_copy_up_file() > > > + else > > + vfs_fsync(new_file, 0); > > + > > + fput(new_file); > > + } > > + } > > Not the right place for fsync. > This should be at the end of ovl_copy_up_metadata() similar to fsync at the end of ovl_copy_up_file(). > The check for sync mode should be abstracted by the helper like ovl_should_sync(), maybe ovl_should_sync_strict() > > > > > dput(parent); > > dput(next); > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index > > 2daba08f78..873d997fb9 100644 > > --- a/fs/overlayfs/ovl_entry.h > > +++ b/fs/overlayfs/ovl_entry.h > > @@ -5,6 +5,12 @@ > > * Copyright (C) 2016 Red Hat, Inc. > > */ > > > > +enum { > > + OVL_VOLATILE_SYNC_NONE, > > + OVL_VOLATILE_SYNC_DATA, > > + OVL_VOLATILE_SYNC_STRICT, > > +}; > > + > > struct ovl_config { > > char *lowerdir; > > char *upperdir; > > @@ -18,6 +24,7 @@ struct ovl_config { > > int xino; > > bool metacopy; > > bool override_creds; > > + int volatile_sync; > > This word volatile_ is unneeded and wrong. "volatile" means "no sync" > Please *replace the config ovl_volatile with sync_mode, don't keep both and grep for all access to ovl_volatile to replace them with adjusted sync_mode code. > > > }; > > > > struct ovl_sb { > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index > > 093af1dcbd..68dee1850b 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -416,6 +416,9 @@ enum { > > OPT_METACOPY_OFF, > > OPT_OVERRIDE_CREDS_ON, > > OPT_OVERRIDE_CREDS_OFF, > > + OPT_VOLATILE_SYNC_NONE, > > + OPT_VOLATILE_SYNC_DATA, > > + OPT_VOLATILE_SYNC_STRICT, > > This word _VOLATILE_ is unneeded and wrong. "volatile" means "no sync" > Which version are you basing your patch on? > You should be developing on top of current upstream kernel, where this parsing code is at params.c. > > You should follow the example of ovl_parameter_verity() which accepts enum values {off, on, require} which is quite the same as what you want for sync mode. Sorry, kernel version I am working on is 5.4.276. Seems there is much difference. > > > OPT_ERR, > > }; > > > > @@ -436,6 +439,9 @@ static const match_table_t ovl_tokens = { > > {OPT_METACOPY_OFF, "metacopy=off"}, > > {OPT_OVERRIDE_CREDS_ON, "override_creds=on"}, > > {OPT_OVERRIDE_CREDS_OFF, "override_creds=off"}, > > + {OPT_VOLATILE_SYNC_NONE, "sync=none"}, > > + {OPT_VOLATILE_SYNC_DATA, "sync=data"}, > > + {OPT_VOLATILE_SYNC_STRICT, "sync=strict"}, > > Note that you need to add the new sync option AND keep the legacy "volatile" mount option. > > > {OPT_ERR, NULL} > > }; > > > > @@ -495,6 +501,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > > if (!config->redirect_mode) > > return -ENOMEM; > > config->override_creds = ovl_override_creds_def; > > + config->volatile_sync = OVL_VOLATILE_SYNC_ DATA; > > > > while ((p = ovl_next_opt(&opt)) != NULL) { > > int token; > > @@ -583,6 +590,18 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > > config->override_creds = false; > > break; > > > > + case OPT_VOLATILE_SYNC_NONE: > > + config->volatile_sync = OVL_VOLATILE_SYNC_NONE; > > + break; > > + > > + case OPT_VOLATILE_SYNC_DATA: > > + config->volatile_sync = OVL_VOLATILE_SYNC_DATA; > > + break; > > + > > + case OPT_VOLATILE_SYNC_STRICT: > > + config->volatile_sync = OVL_VOLATILE_SYNC_STRICT; > > + break; > > + > > > And effectively the two mount options to do the same, i.e.: > > case Opt_sync: > config->sync_mode = result.uint_32; > break; > case Opt_volatile: > config->sync_mode = OVL_SYNC_OFF; > break; > > > default: > > pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p); > > return -EINVAL; > > > > Also need to display the mode in ovl_show_options(). > > Thanks, > Amir, After reading latest code, found the mount option "volatile", now I understand what you mean by "sync=none"/"sync=data"/"sync=strict", "sync=data" is default mount option. what I need to do is extending the meaning of config->ovl_volatile(used to control data sync), using config->sync_mode instead. And for version 5.4.276, I need to add fsync at the end of ovl_copy_up_inode (correspond to latest function ovl_copy_up_metadata), right? Thank you for your patience! Fei
