> > -----邮件原件----- > 发件人: Amir Goldstein [mailto:amir73il@xxxxxxxxx] > 发送时间: 2024年7月19日 15:24 > 收件人: Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> > 抄送: miklos@xxxxxxxxxx; linux-unionfs@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Xu Lianghu（徐良虎） > <lianghuxu@xxxxxxxxxxxx> > 主题: Re: [PATCH] ovl: fsync after metadata copy-up via mount option "upsync=strict" > > On Thu, Jul 18, 2024 at 6:43 AM Fei Lv <feilv@xxxxxxxxxxxx> wrote: > > > > If a directory only exist in low layer, create a new file in it > > trigger directory copy-up. Permission lost of the new directory in > > upper layer was observed during power-cut stress test. > > You should specify that this outcome happens on very specific upper fs (i.e. ubifs) which does not enforce ordering on storing of metadata changes. OK. > > > > > Fix by adding new mount opion "upsync=strict", make sure data/metadata > > of copied up directory written to disk before renaming from tmp to > > final destination. > > > > Signed-off-by: Fei Lv <feilv@xxxxxxxxxxxx> > > --- > > OPT_sync changed to OPT_upsync since mount option "sync" already used. > > I see. I don't like the name "upsync" so much, it has other meanings how about using the option "fsync"? OK. > > Here is a suggested documentation (which should be accompanied to any patch) OK. > > diff --git a/Documentation/filesystems/overlayfs.rst > b/Documentation/filesystems/overlayfs.rst > index 165514401441..f8183ddf8c4d 100644 > --- a/Documentation/filesystems/overlayfs.rst > +++ b/Documentation/filesystems/overlayfs.rst > @@ -742,6 +742,42 @@ controlled by the "uuid" mount option, which supports these values: > mounted with "uuid=on". > > > +Durability and copy up > +---------------------- > + > +The fsync(2) and fdatasync(2) system calls ensure that the metadata and > +data of a file, respectively, are safely written to the backing > +storage, which is expected to guarantee the existence of the information post system crash. > + > +Without the fdatasync(2) call, there is no guarantee that the observed > +data after a system crash will be either the old or the new data, but > +in practice, the observed data after crash is often the old or new data or a mix of both. > + > +When overlayfs file is modified for the first time, copy up will create > +a copy of the lower file and its parent directories in the upper layer. > +In case of a system crash, if fdatasync(2) was not called after the > +modification, the upper file could end up with no data at all (i.e. > +zeros), which would be an unusual outcome. To avoid this experience, > +overlayfs calls fsync(2) on the upper file before completing the copy up with rename(2) to make the copy > up "atomic". > + > +Depending on the backing filesystem (e.g. ubifs), fsync(2) before > +rename(2) may not be enough to provide the "atomic" copy up behavior > +and fsync(2) on the copied up parent directories is required as well. > + > +Overlayfs can be tuned to prefer performance or durability when storing > +to the underlying upper layer. This is controlled by the "fsync" mount > +option, which supports these values: > + > +- "ordered": (default) > + Call fsync(2) on upper file before completion of copy up. > +- "strict": > + Call fsync(2) on upper file and directories before completion of copy up. > +- "volatile": [*] > + Prefer performance over durability (see `Volatile mount`_) > + > +[*] The mount option "volatile" is an alias to "fsync=volatile". > + > + > Volatile mount > -------------- > > > > > fs/overlayfs/copy_up.c | 21 +++++++++++++++++++++ > > fs/overlayfs/ovl_entry.h | 20 ++++++++++++++++++-- > > fs/overlayfs/params.c | 33 +++++++++++++++++++++++++++++---- > > fs/overlayfs/super.c | 2 +- > > 4 files changed, 69 insertions(+), 7 deletions(-) > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index > > a5ef2005a2cc..b6f021ad7a43 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -243,6 +243,21 @@ static int ovl_verify_area(loff_t pos, loff_t pos2, loff_t len, loff_t totlen) > > return 0; > > } > > > > +static int ovl_copy_up_sync(struct path *path) { > > + struct file *new_file; > > + int err; > > + > > + new_file = ovl_path_open(path, O_LARGEFILE | O_WRONLY); > > I don't think any of those O_ flags are needed for fsync. > Can a directory be opened O_WRONLY??? This function may be called with file or directory, shall I need to use different flags? Such as below: static int ovl_copy_up_sync(struct path *path, bool is_dir) { struct file *new_file; int flags; int err; flags = is_dir ? (O_RDONLY | O_DIRECTORY) : (O_LARGEFILE | O_WRONLY); new_file = ovl_path_open(path, flags); if (IS_ERR(new_file)) return PTR_ERR(new_file); err = vfs_fsync(new_file, 0); fput(new_file); return err; } > > > + if (IS_ERR(new_file)) > > + return PTR_ERR(new_file); > > + > > + err = vfs_fsync(new_file, 0); > > + fput(new_file); > > + > > + return err; > > +} > > + > > static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, > > struct file *new_file, loff_t len) { @@ > > -701,6 +716,9 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp) > > err = ovl_set_attr(ofs, temp, &c->stat); > > inode_unlock(temp->d_inode); > > > > + if (!err && ovl_should_sync_strict(ofs)) > > + err = ovl_copy_up_sync(&upperpath); > > + > > return err; > > } > > > > @@ -1104,6 +1122,9 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c) > > ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry)); > > ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry)); > > ovl_set_upperdata(d_inode(c->dentry)); > > + > > + if (!err && ovl_should_sync_strict(ofs)) > > + err = ovl_copy_up_sync(&upperpath); > > fsync was probably already called in ovl_copy_up_file() making this call redundant and fsync of the removal > of metacopy xattr does not add any safety. My original idea was that ovl_should_sync and ovl_should_sync_strict should not be enabled at the same time. The reasons are as follows: If bothe ovl_should_sync and ovl_should_sync_strict return ture for "fsync=strict", and power cut between ovl_copy_up_file and ovl_copy_up_metadata for file copy-up, seems this file may also lost permission? > > > out_free: > > kfree(capability); > > out: > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index > > cb449ab310a7..4592e6f7dcf7 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_SYNC_DATA, > > + OVL_SYNC_STRICT, > > + OVL_SYNC_OFF, > > +}; > > + > > struct ovl_config { > > char *upperdir; > > char *workdir; > > @@ -18,7 +24,7 @@ struct ovl_config { > > int xino; > > bool metacopy; > > bool userxattr; > > - bool ovl_volatile; > > + int sync_mode; > > }; > > > > struct ovl_sb { > > @@ -120,7 +126,17 @@ static inline struct ovl_fs *OVL_FS(struct > > super_block *sb) > > > > static inline bool ovl_should_sync(struct ovl_fs *ofs) { > > - return !ofs->config.ovl_volatile; > > + return ofs->config.sync_mode == OVL_SYNC_DATA; > > return ofs->config.sync_mode != OVL_SYNC_OFF; or > return ofs->config.sync_mode != OVL_FSYNC_VOLATILE; There are risks if ovl_should_sync and ovl_should_sync_strict enabled at the same time. The reasons are above. > > > +} > > + > > +static inline bool ovl_should_sync_strict(struct ovl_fs *ofs) { > > + return ofs->config.sync_mode == OVL_SYNC_STRICT; } > > + > > +static inline bool ovl_is_volatile(struct ovl_config *config) { > > + return config->sync_mode == OVL_SYNC_OFF; > > } > > > > static inline unsigned int ovl_numlower(struct ovl_entry *oe) diff > > --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c index > > 4860fcc4611b..5d5538dd3de7 100644 > > --- a/fs/overlayfs/params.c > > +++ b/fs/overlayfs/params.c > > @@ -58,6 +58,7 @@ enum ovl_opt { > > Opt_xino, > > Opt_metacopy, > > Opt_verity, > > + Opt_upsync, > > Opt_volatile, > > }; > > > > @@ -139,6 +140,23 @@ static int ovl_verity_mode_def(void) > > return OVL_VERITY_OFF; > > } > > > > +static const struct constant_table ovl_parameter_upsync[] = { > > + { "data", OVL_SYNC_DATA }, > > + { "strict", OVL_SYNC_STRICT }, > > + { "off", OVL_SYNC_OFF }, > > + {} > > +}; > > + > > +static const char *ovl_upsync_mode(struct ovl_config *config) { > > + return ovl_parameter_upsync[config->sync_mode].name; > > +} > > + > > +static int ovl_upsync_mode_def(void) > > +{ > > + return OVL_SYNC_DATA; > > +} > > + > > const struct fs_parameter_spec ovl_parameter_spec[] = { > > fsparam_string_empty("lowerdir", Opt_lowerdir), > > fsparam_string("lowerdir+", Opt_lowerdir_add), > > @@ -154,6 +172,7 @@ const struct fs_parameter_spec ovl_parameter_spec[] = { > > fsparam_enum("xino", Opt_xino, ovl_parameter_xino), > > fsparam_enum("metacopy", Opt_metacopy, ovl_parameter_bool), > > fsparam_enum("verity", Opt_verity, ovl_parameter_verity), > > + fsparam_enum("upsync", Opt_upsync, ovl_parameter_upsync), > > fsparam_flag("volatile", Opt_volatile), > > {} > > }; > > @@ -617,8 +636,11 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param) > > case Opt_verity: > > config->verity_mode = result.uint_32; > > break; > > + case Opt_upsync: > > + config->sync_mode = result.uint_32; > > + break; > > case Opt_volatile: > > - config->ovl_volatile = true; > > + config->sync_mode = OVL_SYNC_OFF; > > break; > > case Opt_userxattr: > > config->userxattr = true; @@ -802,9 +824,9 @@ int > > ovl_fs_params_verify(const struct ovl_fs_context *ctx, > > config->index = false; > > } > > > > - if (!config->upperdir && config->ovl_volatile) { > > + if (!config->upperdir && ovl_is_volatile(config)) { > > pr_info("option \"volatile\" is meaningless in a > > non-upper mount, ignoring it.\n"); > > This message would be confusing if mount option is "syncup=off" > but if the option is "fsync=volatile" I think the message can stay as it is. > > Thanks, > Amir. Yes. That sounds good! We thought this place was a little weird, too... > > > - config->ovl_volatile = false; > > + config->sync_mode = ovl_upsync_mode_def(); > > } > > > > if (!config->upperdir && config->uuid == OVL_UUID_ON) { @@ > > -997,8 +1019,11 @@ int ovl_show_options(struct seq_file *m, struct dentry *dentry) > > if (ofs->config.metacopy != ovl_metacopy_def) > > seq_printf(m, ",metacopy=%s", > > ofs->config.metacopy ? "on" : "off"); > > - if (ofs->config.ovl_volatile) > > + if (ovl_is_volatile(&ofs->config)) > > seq_puts(m, ",volatile"); > > + else if (ofs->config.sync_mode != ovl_upsync_mode_def()) > > + seq_printf(m, ",upsync=%s", > > + ovl_upsync_mode(&ofs->config)); > > if (ofs->config.userxattr) > > seq_puts(m, ",userxattr"); > > if (ofs->config.verity_mode != ovl_verity_mode_def()) diff > > --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index > > 06a231970cb5..824cbcf40523 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -750,7 +750,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs, > > * For volatile mount, create a incompat/volatile/dirty file to keep > > * track of it. > > */ > > - if (ofs->config.ovl_volatile) { > > + if (ovl_is_volatile(&ofs->config)) { > > err = ovl_create_volatile_dirty(ofs); > > if (err < 0) { > > pr_err("Failed to create volatile/dirty > > file.\n"); > > > > base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd > > -- > > 2.45.2 > > Thanks, Fei
