| Message ID | 20220301191329.22329-1-zajec5@gmail.com |
|---|---|
| State | Under Review |
| Delegated to: | Rafał Miłecki |
| Headers | show |
| Series | kernel: force atomic renames in ubifs | expand |
Rafał, ----- Ursprüngliche Mail ----- > Von: "Rafał Miłecki" <zajec5@gmail.com> > An: "OpenWrt Development List" <openwrt-devel@lists.openwrt.org> > CC: "Koen Vandeputte" <koen.vandeputte@ncentric.com>, "richard" <richard@nod.at>, "Rafał Miłecki" <rafal@milecki.pl> > Gesendet: Dienstag, 1. März 2022 20:13:29 > Betreff: [PATCH] kernel: force atomic renames in ubifs > From: Rafał Miłecki <rafal@milecki.pl> > > This deals with user-spaces apps that don't handle all syncing > correctly. It prevents user ending up with an empty file. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../510-ubifs-force-atomic-renames.patch | 46 +++++++++++++++++++ > .../510-ubifs-force-atomic-renames.patch | 46 +++++++++++++++++++ > 2 files changed, 92 insertions(+) > create mode 100644 > target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch > create mode 100644 > target/linux/generic/pending-5.4/510-ubifs-force-atomic-renames.patch > > diff --git > a/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch > b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch > new file mode 100644 > index 0000000000..80f5f1b910 > --- /dev/null > +++ b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch > @@ -0,0 +1,46 @@ > +From: Richard Weinberger <richard@nod.at> > +Date: Mon, 28 Feb 2022 16:07:41 +0100 > +Subject: [PATCH] ubifs: force atomic renames > +MIME-Version: 1.0 > +Content-Type: text/plain; charset=UTF-8 > +Content-Transfer-Encoding: 8bit > + > +Before actual rename make sure that the old inode data has been flash > +written. This is similar to what other filesystems do and workarounds > +bugs in some user-space apps. > + > +With this change updating file using tmpfile & rename() will never > +result in losing all content e.g. on power cut. Please slow down a bit. :-) The commit message is not written by me and also not entirely correct. The patch is not about making rename atomic. rename is already atomic on UBIFS. It is about syncing in flight pages when a file is overwritten by rename. While I plan to upstream this change it still needs more testing. I'm also not sure about the overhead it causes. Flushing the write buffers can cause more garbage collection and may trigger write amplification. Thanks, //richard
Hi Richard, On 1.03.2022 20:37, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- >> Von: "Rafał Miłecki" <zajec5@gmail.com> >> An: "OpenWrt Development List" <openwrt-devel@lists.openwrt.org> >> CC: "Koen Vandeputte" <koen.vandeputte@ncentric.com>, "richard" <richard@nod.at>, "Rafał Miłecki" <rafal@milecki.pl> >> Gesendet: Dienstag, 1. März 2022 20:13:29 >> Betreff: [PATCH] kernel: force atomic renames in ubifs > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This deals with user-spaces apps that don't handle all syncing >> correctly. It prevents user ending up with an empty file. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> .../510-ubifs-force-atomic-renames.patch | 46 +++++++++++++++++++ >> .../510-ubifs-force-atomic-renames.patch | 46 +++++++++++++++++++ >> 2 files changed, 92 insertions(+) >> create mode 100644 >> target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch >> create mode 100644 >> target/linux/generic/pending-5.4/510-ubifs-force-atomic-renames.patch >> >> diff --git >> a/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch >> b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch >> new file mode 100644 >> index 0000000000..80f5f1b910 >> --- /dev/null >> +++ b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch >> @@ -0,0 +1,46 @@ >> +From: Richard Weinberger <richard@nod.at> >> +Date: Mon, 28 Feb 2022 16:07:41 +0100 >> +Subject: [PATCH] ubifs: force atomic renames >> +MIME-Version: 1.0 >> +Content-Type: text/plain; charset=UTF-8 >> +Content-Transfer-Encoding: 8bit >> + >> +Before actual rename make sure that the old inode data has been flash >> +written. This is similar to what other filesystems do and workarounds >> +bugs in some user-space apps. >> + >> +With this change updating file using tmpfile & rename() will never >> +result in losing all content e.g. on power cut. > > Please slow down a bit. :-) > The commit message is not written by me and also not entirely correct. > > The patch is not about making rename atomic. rename is already atomic > on UBIFS. > It is about syncing in flight pages when a file is overwritten by rename. > > While I plan to upstream this change it still needs more testing. > I'm also not sure about the overhead it causes. Flushing the write buffers > can cause more garbage collection and may trigger write amplification. I didn't end up pushing this change to OpenWrt since your e-mail. I use it however in my project in production in thousands of devices. I'd very much like to see a proper change upstream. Could you take a look at it, please?
diff --git a/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch new file mode 100644 index 0000000000..80f5f1b910 --- /dev/null +++ b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch @@ -0,0 +1,46 @@ +From: Richard Weinberger <richard@nod.at> +Date: Mon, 28 Feb 2022 16:07:41 +0100 +Subject: [PATCH] ubifs: force atomic renames +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Before actual rename make sure that the old inode data has been flash +written. This is similar to what other filesystems do and workarounds +bugs in some user-space apps. + +With this change updating file using tmpfile & rename() will never +result in losing all content e.g. on power cut. + +Signed-off-by: Rafał Miłecki <rafal@milecki.pl> +--- + fs/ubifs/dir.c | 18 ++++++++++++++---- + 1 file changed, 14 insertions(+), 4 deletions(-) + +--- a/fs/ubifs/dir.c ++++ b/fs/ubifs/dir.c +@@ -1287,10 +1287,20 @@ static int do_rename(struct inode *old_d + return err; + } + +- if (unlink && is_dir) { +- err = ubifs_check_dir_empty(new_inode); +- if (err) +- return err; ++ if (unlink) { ++ if (is_dir) { ++ err = ubifs_check_dir_empty(new_inode); ++ if (err) ++ return err; ++ } else { ++ err = filemap_fdatawrite(old_inode->i_mapping); ++ if (err) ++ return err; ++ ++ err = ubifs_sync_wbufs_by_inode(c, old_inode); ++ if (err) ++ return err; ++ } + } + + err = fscrypt_setup_filename(old_dir, &old_dentry->d_name, 0, &old_nm); diff --git a/target/linux/generic/pending-5.4/510-ubifs-force-atomic-renames.patch b/target/linux/generic/pending-5.4/510-ubifs-force-atomic-renames.patch new file mode 100644 index 0000000000..30bc686854 --- /dev/null +++ b/target/linux/generic/pending-5.4/510-ubifs-force-atomic-renames.patch @@ -0,0 +1,46 @@ +From: Richard Weinberger <richard@nod.at> +Date: Mon, 28 Feb 2022 16:07:41 +0100 +Subject: [PATCH] ubifs: force atomic renames +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Before actual rename make sure that the old inode data has been flash +written. This is similar to what other filesystems do and workarounds +bugs in some user-space apps. + +With this change updating file using tmpfile & rename() will never +result in losing all content e.g. on power cut. + +Signed-off-by: Rafał Miłecki <rafal@milecki.pl> +--- + fs/ubifs/dir.c | 18 ++++++++++++++---- + 1 file changed, 14 insertions(+), 4 deletions(-) + +--- a/fs/ubifs/dir.c ++++ b/fs/ubifs/dir.c +@@ -1295,10 +1295,20 @@ static int do_rename(struct inode *old_d + return err; + } + +- if (unlink && is_dir) { +- err = ubifs_check_dir_empty(new_inode); +- if (err) +- return err; ++ if (unlink) { ++ if (is_dir) { ++ err = ubifs_check_dir_empty(new_inode); ++ if (err) ++ return err; ++ } else { ++ err = filemap_fdatawrite(old_inode->i_mapping); ++ if (err) ++ return err; ++ ++ err = ubifs_sync_wbufs_by_inode(c, old_inode); ++ if (err) ++ return err; ++ } + } + + err = fscrypt_setup_filename(old_dir, &old_dentry->d_name, 0, &old_nm);