Re: [PATCH] kernel: force atomic renames in ubifs
Hi Richard, On 1.03.2022 20:37, Richard Weinberger wrote: - Ursprüngliche Mail - Von: "Rafał Miłecki" An: "OpenWrt Development List" CC: "Koen Vandeputte" , "richard" , "Rafał Miłecki" Gesendet: Dienstag, 1. März 2022 20:13:29 Betreff: [PATCH] kernel: force atomic renames in ubifs From: Rafał Miłecki 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 --- .../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 00..80f5f1b910 --- /dev/null +++ b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch @@ -0,0 +1,46 @@ +From: Richard Weinberger +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? ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] kernel: force atomic renames in ubifs
Rafał, - Ursprüngliche Mail - > Von: "Rafał Miłecki" > An: "OpenWrt Development List" > CC: "Koen Vandeputte" , "richard" > , "Rafał Miłecki" > Gesendet: Dienstag, 1. März 2022 20:13:29 > Betreff: [PATCH] kernel: force atomic renames in ubifs > From: Rafał Miłecki > > 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 > --- > .../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 00..80f5f1b910 > --- /dev/null > +++ b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch > @@ -0,0 +1,46 @@ > +From: Richard Weinberger > +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 ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH] kernel: force atomic renames in ubifs
From: Rafał Miłecki
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
---
.../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 00..80f5f1b910
--- /dev/null
+++ b/target/linux/generic/pending-5.10/510-ubifs-force-atomic-renames.patch
@@ -0,0 +1,46 @@
+From: Richard Weinberger
+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
+---
+ 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 00..30bc686854
--- /dev/null
+++ b/target/linux/generic/pending-5.4/510-ubifs-force-atomic-renames.patch
@@ -0,0 +1,46 @@
+From: Richard Weinberger
+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
+---
+ 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);
--
2.34.1
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
