On Thu, Aug 29, 2024 at 2:51 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Thu, 29 Aug 2024 at 12:29, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > But maybe we can ignore crash safety of metacopy on ubifs, because > > 1. the ubifs users may not be using this feature > > 2. ubifs may be nice and takes care of ordering O_TMPFILE > > metadata updates before exposing the link > > > > Then we can do the following: > > IF (metacopy_enabled) > > fsync only in ovl_copy_up_file() > > ELSE > > fsync only in ovl_copy_up_metadata() > > > > Let me know what you think. > > Sounds like a good compromise. > Fei, Could you please test the attached patch and confirm that your use case does not depend on metacopy enabled? In any case, I am holding on to your patch in case someone reports a performance regression with this unconditional fsync approach. Thanks, Amir.
From 6ddc5bbdd2010d90d7eca0abb340eaeeafafc38a Mon Sep 17 00:00:00 2001 From: Amir Goldstein <amir73il@xxxxxxxxx> Date: Thu, 29 Aug 2024 17:51:08 +0200 Subject: [PATCH] ovl: fsync after metadata copy-up For upper filesystems which do not use strict ordering of persisting metadata changes (e.g. ubifs), 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. Permission lost of the new upper parent directory was observed during power-cut stress test. Fix by moving the fsync call after metadata copy to make sure that the metadata copied up directory and files persists to disk before renaming from tmp to final destination. With metacopy enabled, this change will hurt performance of workloads such as chown -R, so we keep the legacy behavior of fsync on data copyup and never fsync on copyup of metadata of files and directories. Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxj-pOvmw1-uXR3qVdqtLjSkwcR9nVKcNU_vC10Zyf2miQ@xxxxxxxxxxxxxx/ Reported-by: Fei Lv <feilv@xxxxxxxxxxxx> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> --- fs/overlayfs/copy_up.c | 43 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index a5ef2005a2cc..e9348ac1548a 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -243,8 +243,24 @@ static int ovl_verify_area(loff_t pos, loff_t pos2, loff_t len, loff_t totlen) return 0; } +static int ovl_sync_file(struct path *path) +{ + struct file *new_file; + int err; + + new_file = ovl_path_open(path, O_RDONLY); + 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) + struct file *new_file, loff_t len, + bool datasync) { struct path datapath; struct file *old_file; @@ -342,7 +358,8 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, len -= bytes; } - if (!error && ovl_should_sync(ofs)) + /* call fsync once, either now or later along with metadata */ + if (!error && ovl_should_sync(ofs) && datasync) error = vfs_fsync(new_file, 0); out_fput: fput(old_file); @@ -574,6 +591,7 @@ struct ovl_copy_up_ctx { bool indexed; bool metacopy; bool metacopy_digest; + bool metadata_fsync; }; static int ovl_link_up(struct ovl_copy_up_ctx *c) @@ -634,7 +652,8 @@ static int ovl_copy_up_data(struct ovl_copy_up_ctx *c, const struct path *temp) if (IS_ERR(new_file)) return PTR_ERR(new_file); - err = ovl_copy_up_file(ofs, c->dentry, new_file, c->stat.size); + err = ovl_copy_up_file(ofs, c->dentry, new_file, c->stat.size, + !c->metadata_fsync); fput(new_file); return err; @@ -701,6 +720,10 @@ 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); + /* fsync metadata before move into upper dir */ + if (!err && ovl_should_sync(ofs) && c->metadata_fsync) + err = ovl_sync_file(&upperpath); + return err; } @@ -860,7 +883,8 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c) temp = tmpfile->f_path.dentry; if (!c->metacopy && c->stat.size) { - err = ovl_copy_up_file(ofs, c->dentry, tmpfile, c->stat.size); + err = ovl_copy_up_file(ofs, c->dentry, tmpfile, c->stat.size, + !c->metadata_fsync); if (err) goto out_fput; } @@ -1135,6 +1159,17 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry, !kgid_has_mapping(current_user_ns(), ctx.stat.gid)) return -EOVERFLOW; + /* + * With metacopy disabled, we fsync after final metadata copyup, for + * both regular and non-regular files to get atomic copyup semantics + * on filesystems that do not use strict metadata ordering (e.g. ubifs). + * + * With metacopy enabled we want to avoid fsync on all meta copyup + * that will hurt performance of workloads such as chown -R, so we + * only fsync on data copyup and never fsync on copyup of non-regular + * files and directories. + */ + ctx.metadata_fsync = !OVL_FS(dentry->d_sb)->config.metacopy; ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags); if (parent) { -- 2.34.1
