Patchwork [v2,3/3] UBIFS: do not start the commit if there is nothing to commit

login
register
mail settings
Submitter Artem Bityutskiy
Date Jan. 18, 2011, 7:30 a.m.
Message ID <1295335817.2470.31.camel@koala>
Download mbox | patch
Permalink /patch/79265/
State New
Headers show

Comments

Artem Bityutskiy - Jan. 18, 2011, 7:30 a.m.
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

This patch fixes suboptimal UBIFS 'sync_fs()' implementation which causes
flash I/O even if the file-system is synchronized. E.g., a 'printk()'
in the MTD erasure function (e.g., 'nand_erase_nand()') can show that
for every 'sync' shell command UBIFS erases at least one eraseblock.

So '$ while true; do sync; done' will cause huge amount of flash I/O.

The reason for this is that UBIFS commits in 'sync_fs()', and starts the
commit even if there is nothing to commit, e.g., it anyway changes the
log. This patch adds a check in the 'do_commit()' UBIFS functions which
prevents the commit if there is nothing to commit.

Reported-by: Hans J. Koch <hjk@linutronix.de>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/commit.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 53 insertions(+), 1 deletions(-)
Artem Bityutskiy - Jan. 18, 2011, 7:36 a.m.
On Tue, 2011-01-18 at 09:30 +0200, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> This patch fixes suboptimal UBIFS 'sync_fs()' implementation which causes
> flash I/O even if the file-system is synchronized. E.g., a 'printk()'
> in the MTD erasure function (e.g., 'nand_erase_nand()') can show that
> for every 'sync' shell command UBIFS erases at least one eraseblock.
> 
> So '$ while true; do sync; done' will cause huge amount of flash I/O.
> 
> The reason for this is that UBIFS commits in 'sync_fs()', and starts the
> commit even if there is nothing to commit, e.g., it anyway changes the
> log. This patch adds a check in the 'do_commit()' UBIFS functions which
> prevents the commit if there is nothing to commit.
> 
> Reported-by: Hans J. Koch <hjk@linutronix.de>
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Adrian, I've also pushed these 3 patches to ubifs-2.6.git / master.
Could you please validate them?
John Ogness - Jan. 18, 2011, 12:29 p.m.
Hi,

With these patches, there is no longer an erase for _every_
sync. However, there is still one erase that occurs when ubifs is
mounted. The following script will cause an erase with each sync.

while true; do
    mount -t ubifs ubi0_2 /tmp/mnt
    sync
    umount /tmp/mnt
done

John Ogness
Artem Bityutskiy - Jan. 21, 2011, 11:13 a.m.
On Tue, 2011-01-18 at 13:29 +0100, John Ogness wrote:
> Hi,
> 
> With these patches, there is no longer an erase for _every_
> sync. However, there is still one erase that occurs when ubifs is
> mounted. The following script will cause an erase with each sync.
> 
> while true; do
>     mount -t ubifs ubi0_2 /tmp/mnt
>     sync
>     umount /tmp/mnt
> done

Hi, thanks, I need to look why there are still erases after mount.
Probably this is actually part of mounting, not part of the first sync.
But otherwise it sounds like you are satisfied, right? But I also have
to do power cut tests emulation before really pushing this forward.
John Ogness - Jan. 21, 2011, 11:28 a.m.
On 2011-01-21, Artem Bityutskiy wrote:
> I need to look why there are still erases after mount.  Probably
> this is actually part of mounting, not part of the first sync.  But
> otherwise it sounds like you are satisfied, right?

Yes, it is definately a big improvement. Thanks!

> But I also have to do power cut tests emulation before really
> pushing this forward.

OK.

BTW: The return value description for the function nothing_to_commit()
is backwards. It says that it returns 1 if there is something to
commit, but actually it returns 1 if there is nothing to commit.

John Ogness
Artem Bityutskiy - Jan. 25, 2011, 8:20 a.m.
On Fri, 2011-01-21 at 12:28 +0100, John Ogness wrote:
> On 2011-01-21, Artem Bityutskiy wrote:
> > I need to look why there are still erases after mount.  Probably
> > this is actually part of mounting, not part of the first sync.  But
> > otherwise it sounds like you are satisfied, right?
> 
> Yes, it is definately a big improvement. Thanks!
> 
> > But I also have to do power cut tests emulation before really
> > pushing this forward.

The power cut simulation test is running.

> BTW: The return value description for the function nothing_to_commit()
> is backwards. It says that it returns 1 if there is something to
> commit, but actually it returns 1 if there is nothing to commit.

Amended, thanks. Also added:

Tested-by: John Ogness <john.ogness@linutronix.de>

Thanks.

Patch

diff --git a/fs/ubifs/commit.c b/fs/ubifs/commit.c
index 02429d8..fdd5112 100644
--- a/fs/ubifs/commit.c
+++ b/fs/ubifs/commit.c
@@ -48,6 +48,52 @@ 
 #include <linux/slab.h>
 #include "ubifs.h"
 
+/*
+ * nothing_to_commit - check if there is nothing to commit.
+ * @c: UBIFS file-system description object
+ *
+ * This is a helper function which checks if there is anything to commit. It is
+ * used as an optimization to avoid starting the commit if it is not really
+ * necessary. Indeed, the commit operation always assumes flash I/O (e.g.,
+ * writing the commit start node to the log), and it is better to avoid doing
+ * this unnecessarily. E.g., 'ubifs_sync_fs()' runs the commit, but if there is
+ * nothing to commit, it is more optimal to avoid any flash I/O.
+ *
+ * This function returns %1 if there is something to commit and %0 otherwise.
+ */
+static int nothing_to_commit(struct ubifs_info *c)
+{
+	/*
+	 * During mounting or remounting from R/O mode to R/W mode we may
+	 * commit for various recovery-related reasons.
+	 */
+	if (c->mounting || c->remounting_rw)
+		return 0;
+
+	/*
+	 * If the root TNC node is dirty, we definitely have something to
+	 * commit.
+	 */
+	if (c->zroot.znode && test_bit(DIRTY_ZNODE, &c->zroot.znode->flags))
+		return 0;
+
+	/*
+	 * Even though the TNC is clean, the LPT tree may have dirty nodes. For
+	 * example, this may happen if the budgeting subsystem invoked GC to
+	 * make some free space, and the GC found an LEB with only dirty and
+	 * free space. In this case GC would just change the lprops of this
+	 * LEB (by turning all space into free space) and unmap it.
+	 */
+	if (c->nroot && test_bit(DIRTY_CNODE, &c->nroot->flags))
+		return 0;
+
+	ubifs_assert(atomic_long_read(&c->dirty_zn_cnt) == 0);
+	ubifs_assert(c->dirty_pn_cnt == 0);
+	ubifs_assert(c->dirty_nn_cnt == 0);
+
+	return 1;
+}
+
 /**
  * do_commit - commit the journal.
  * @c: UBIFS file-system description object
@@ -70,6 +116,12 @@  static int do_commit(struct ubifs_info *c)
 		goto out_up;
 	}
 
+	if (nothing_to_commit(c)) {
+		up_write(&c->commit_sem);
+		err = 0;
+		goto out_cancel;
+	}
+
 	/* Sync all write buffers (necessary for recovery) */
 	for (i = 0; i < c->jhead_cnt; i++) {
 		err = ubifs_wbuf_sync(&c->jheads[i].wbuf);
@@ -162,12 +214,12 @@  static int do_commit(struct ubifs_info *c)
 	if (err)
 		goto out;
 
+out_cancel:
 	spin_lock(&c->cs_lock);
 	c->cmt_state = COMMIT_RESTING;
 	wake_up(&c->cmt_wq);
 	dbg_cmt("commit end");
 	spin_unlock(&c->cs_lock);
-
 	return 0;
 
 out_up: