> -----邮件原件----- > 发件人: 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=/overl > > 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. 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; 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); + else + vfs_fsync(new_file, 0); + + fput(new_file); + } + } 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; }; 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, 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"}, {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; + default: pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p); return -EINVAL; Thanks, Fei
