ubifs: sync() causes writes even if nothing is changed

ubifs: sync() causes writes even if nothing is changed

[Hans J. Koch]

Running this command:

# while true ; do sync; sleep 1; done

causes two eraseblocks being erased every second, although there
are no writes to the ubifs filesystem. I hacked some printks into
my NAND driver that print page_address and column for each erase.
With that, I get this output every second:

...
[   63.701765] erase p=0x0000ae40 c=0xffffffff
[   63.706534] erase p=0xffffffff c=0xffffffff
[   63.725492] erase p=0x0000ae80 c=0xffffffff
[   63.730260] erase p=0xffffffff c=0xffffffff
...

>From a quick glance at the ubifs code, this might come out of the
garbage collector that is triggered on every sync() and writes
something even if nothing has changed.

Is that really needed?

Thanks,
Hans

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

On Wed, 2010-10-13 at 18:30 +0200, Hans J. Koch wrote:
> Running this command:
> 
> # while true ; do sync; sleep 1; done
> 
> causes two eraseblocks being erased every second, although there
> are no writes to the ubifs filesystem. I hacked some printks into
> my NAND driver that print page_address and column for each erase.
> With that, I get this output every second:
> 
> ...
> [   63.701765] erase p=0x0000ae40 c=0xffffffff
> [   63.706534] erase p=0xffffffff c=0xffffffff
> [   63.725492] erase p=0x0000ae80 c=0xffffffff
> [   63.730260] erase p=0xffffffff c=0xffffffff
> ...
> 
> From a quick glance at the ubifs code, this might come out of the
> garbage collector that is triggered on every sync() and writes
> something even if nothing has changed.
> 
> Is that really needed?

Hi, no, it is not. I'll look at this as soon as I have some time. There
is already Matthieu's issue waiting in my queue, though. Thanks for
reporting.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

On Wed, 2010-10-13 at 18:30 +0200, Hans J. Koch wrote:
> Running this command:
> 
> # while true ; do sync; sleep 1; done
> 
> causes two eraseblocks being erased every second, although there
> are no writes to the ubifs filesystem. I hacked some printks into
> my NAND driver that print page_address and column for each erase.
> With that, I get this output every second:
> 
> ...
> [   63.701765] erase p=0x0000ae40 c=0xffffffff
> [   63.706534] erase p=0xffffffff c=0xffffffff
> [   63.725492] erase p=0x0000ae80 c=0xffffffff
> [   63.730260] erase p=0xffffffff c=0xffffffff
> ...
> 
> From a quick glance at the ubifs code, this might come out of the
> garbage collector that is triggered on every sync() and writes
> something even if nothing has changed.
> 
> Is that really needed?

I briefly looked, and UBIFS is doing a lot of I/O in that case, which is
unnecessary and should not be done, I'll need to investigate this.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: ubifs: sync() causes writes even if nothing is changed

[Sebastian Andrzej Siewior]

Artem Bityutskiy wrote:
>> From a quick glance at the ubifs code, this might come out of the
>> garbage collector that is triggered on every sync() and writes
>> something even if nothing has changed.
>>
>> Is that really needed?
> 
> I briefly looked, and UBIFS is doing a lot of I/O in that case, which is
> unnecessary and should not be done, I'll need to investigate this.

 From what I've seen from the code there are two parts:
- writing "unwritten" data (the write back part I guess). Nothing is done
   here as long as no data is queued
- writing the journal. The first step seems to be to write a node "begin
   write" and the last one is "end write".

Since hjk sees only two erases I would guess that they are initiated by 
the "journal begin" & "end" requests.

Thanks for looking at this.

Sebastian

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

On Thu, 2010-10-21 at 10:23 +0200, Sebastian Andrzej Siewior wrote:
> Artem Bityutskiy wrote:
> >> From a quick glance at the ubifs code, this might come out of the
> >> garbage collector that is triggered on every sync() and writes
> >> something even if nothing has changed.
> >>
> >> Is that really needed?
> > 
> > I briefly looked, and UBIFS is doing a lot of I/O in that case, which is
> > unnecessary and should not be done, I'll need to investigate this.
> 
>  From what I've seen from the code there are two parts:
> - writing "unwritten" data (the write back part I guess). Nothing is done
>    here as long as no data is queued
> - writing the journal. The first step seems to be to write a node "begin
>    write" and the last one is "end write".
> 
> Since hjk sees only two erases I would guess that they are initiated by 
> the "journal begin" & "end" requests.

Yeah, the UBIFS must be writing the commit start and commit end nodes,
and then erasing the previous log LEB (all this is done in log.c). As
soon as I have time I'll look at this, I really do not have time right
now. And there is Matthiew's problem which is actually quite big, but
for some reasons affects only him :-)

May be it is just matter of looking at all journal heads and if they are
empty, do nothing and return from ubifs_commit().

Adrian, would you help with this, may be?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: ubifs: sync() causes writes even if nothing is changed

[Matthieu CASTET]

Hi,

Artem Bityutskiy a écrit :
> On Thu, 2010-10-21 at 10:23 +0200, Sebastian Andrzej Siewior wrote:
>> Artem Bityutskiy wrote:
>>>> From a quick glance at the ubifs code, this might come out of the
>>>> garbage collector that is triggered on every sync() and writes
>>>> something even if nothing has changed.
> 
> Yeah, the UBIFS must be writing the commit start and commit end nodes,
> and then erasing the previous log LEB (all this is done in log.c). As
> soon as I have time I'll look at this, I really do not have time right
> now. And there is Matthiew's problem which is actually quite big, but
> for some reasons affects only him :-)
It could be interesting to know which nand flash are tested.

On our boards ST 256MB chip work fine, but micron flash got unstable 
page problem (sorry I don't have the model number ATM).

And it is getting worse, new SLC flash can now get up to 4 bits error 
per 512.


Matthieu

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

On Thu, 2010-10-21 at 19:04 +0200, Matthieu CASTET wrote:
> Hi,
> 
> Artem Bityutskiy a écrit :
> > On Thu, 2010-10-21 at 10:23 +0200, Sebastian Andrzej Siewior wrote:
> >> Artem Bityutskiy wrote:
> >>>> From a quick glance at the ubifs code, this might come out of the
> >>>> garbage collector that is triggered on every sync() and writes
> >>>> something even if nothing has changed.
> > 
> > Yeah, the UBIFS must be writing the commit start and commit end nodes,
> > and then erasing the previous log LEB (all this is done in log.c). As
> > soon as I have time I'll look at this, I really do not have time right
> > now. And there is Matthiew's problem which is actually quite big, but
> > for some reasons affects only him :-)
> It could be interesting to know which nand flash are tested.
> 
> On our boards ST 256MB chip work fine, but micron flash got unstable 
> page problem (sorry I don't have the model number ATM).
> 
> And it is getting worse, new SLC flash can now get up to 4 bits error 
> per 512.

It would be great if you sent me a board where unstable pages are easy
to reproduce.

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

On Wed, 2010-10-13 at 18:30 +0200, Hans J. Koch wrote:
> Running this command:
> 
> # while true ; do sync; sleep 1; done
> 
> causes two eraseblocks being erased every second, although there
> are no writes to the ubifs filesystem. I hacked some printks into
> my NAND driver that print page_address and column for each erase.
> With that, I get this output every second:
> 
> ...
> [   63.701765] erase p=0x0000ae40 c=0xffffffff
> [   63.706534] erase p=0xffffffff c=0xffffffff
> [   63.725492] erase p=0x0000ae80 c=0xffffffff
> [   63.730260] erase p=0xffffffff c=0xffffffff
> ...
> 
> From a quick glance at the ubifs code, this might come out of the
> garbage collector that is triggered on every sync() and writes
> something even if nothing has changed.

With nandsim I only can see one erase, but this is anyway suboptimal.
The below patch should fix the issue, please, test if you can. I've also
pushed it to ubifs-2.6.git.


>From dca0fe61489805e0eb4ada7c6922856ca91eae52 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Sun, 16 Jan 2011 19:22:02 +0200
Subject: [PATCH] UBIFS: do not start the commit if there is nothing to commit

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 are not dirty znodes (hence, nothing to
commit).

Reported-by: Hans J. Koch <hjk@linutronix.de>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/commit.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/fs/ubifs/commit.c b/fs/ubifs/commit.c
index 02429d8..a963d96 100644
--- a/fs/ubifs/commit.c
+++ b/fs/ubifs/commit.c
@@ -70,6 +70,21 @@ static int do_commit(struct ubifs_info *c)
 		goto out_up;
 	}
 
+	/*
+	 * Every file-system change changes the TNC, and makes the root znode
+	 * dirty. So if the root znode is clean we can just return immediately
+	 * because there must be nothing to commit. Note, se do not have to
+	 * lock @c->tnc_mutex because we have @c->commit_sem in write mode,
+	 * which guarantees that no one else can access TNC functions
+	 * concurrently.
+	 */
+	if (!c->zroot.znode || !test_bit(DIRTY_ZNODE, &c->zroot.znode->flags)) {
+		ubifs_assert(atomic_long_read(&c->dirty_zn_cnt) == 0);
+		err = 0;
+		up_write(&c->commit_sem);
+		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 +177,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:
-- 
1.7.3.4

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: ubifs: sync() causes writes even if nothing is changed

[Adrian Hunter]

On 16/01/11 19:48, ext Artem Bityutskiy wrote:
> On Wed, 2010-10-13 at 18:30 +0200, Hans J. Koch wrote:
>> Running this command:
>>
>> # while true ; do sync; sleep 1; done
>>
>> causes two eraseblocks being erased every second, although there
>> are no writes to the ubifs filesystem. I hacked some printks into
>> my NAND driver that print page_address and column for each erase.
>> With that, I get this output every second:
>>
>> ...
>> [   63.701765] erase p=0x0000ae40 c=0xffffffff
>> [   63.706534] erase p=0xffffffff c=0xffffffff
>> [   63.725492] erase p=0x0000ae80 c=0xffffffff
>> [   63.730260] erase p=0xffffffff c=0xffffffff
>> ...
>>
>>  From a quick glance at the ubifs code, this might come out of the
>> garbage collector that is triggered on every sync() and writes
>> something even if nothing has changed.
>
> With nandsim I only can see one erase, but this is anyway suboptimal.
> The below patch should fix the issue, please, test if you can. I've also
> pushed it to ubifs-2.6.git.
>
>
>> From dca0fe61489805e0eb4ada7c6922856ca91eae52 Mon Sep 17 00:00:00 2001
> From: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
> Date: Sun, 16 Jan 2011 19:22:02 +0200
> Subject: [PATCH] UBIFS: do not start the commit if there is nothing to commit
>
> 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 are not dirty znodes (hence, nothing to
> commit).

Possibly the LPT should be checked also.  Perhaps it can be dirty due
to trivial garbage collection.

Also, have you checked there are no degenerate cases where the commit
is required for some other reason such as consolidating the log or the
recovery  commit?

>
> Reported-by: Hans J. Koch<hjk@linutronix.de>
> Signed-off-by: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
> ---
>   fs/ubifs/commit.c |   17 ++++++++++++++++-
>   1 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/fs/ubifs/commit.c b/fs/ubifs/commit.c
> index 02429d8..a963d96 100644
> --- a/fs/ubifs/commit.c
> +++ b/fs/ubifs/commit.c
> @@ -70,6 +70,21 @@ static int do_commit(struct ubifs_info *c)
>   		goto out_up;
>   	}
>
> +	/*
> +	 * Every file-system change changes the TNC, and makes the root znode
> +	 * dirty. So if the root znode is clean we can just return immediately
> +	 * because there must be nothing to commit. Note, se do not have to
> +	 * lock @c->tnc_mutex because we have @c->commit_sem in write mode,
> +	 * which guarantees that no one else can access TNC functions
> +	 * concurrently.
> +	 */
> +	if (!c->zroot.znode || !test_bit(DIRTY_ZNODE,&c->zroot.znode->flags)) {
> +		ubifs_assert(atomic_long_read(&c->dirty_zn_cnt) == 0);
> +		err = 0;
> +		up_write(&c->commit_sem);
> +		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 +177,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:

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

On Mon, 2011-01-17 at 10:19 +0200, Adrian Hunter wrote:
> Possibly the LPT should be checked also.  Perhaps it can be dirty due
> to trivial garbage collection.

I'll check, thanks.

> Also, have you checked there are no degenerate cases where the commit
> is required for some other reason such as consolidating the log or the
> recovery  commit?

Right, I missed this, will check as well.

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: ubifs: sync() causes writes even if nothing is changed

[Artem Bityutskiy]

[Removed Hans J. Koch from CC as his mailbox is unreachable]

On Mon, 2011-01-17 at 10:19 +0200, Adrian Hunter wrote:
> Possibly the LPT should be checked also.  Perhaps it can be dirty due
> to trivial garbage collection.

Yes, AFAIU we can GC from budgeting, end up with trivial gc which will
make dirt in LTP but not in TNC. So you are right.

But there is another "trival GC" inside lprops subsystem, but I think we
should not worry about it.

> Also, have you checked there are no degenerate cases where the commit
> is required for some other reason such as consolidating the log or the
> recovery  commit?

I do not really see how could recovery be needed if nothing is made
dirty in TNC, but due to complexity of that stuff it is safer to do the
commit if we are mounting or remounting rw, because there may be such
situations.

I'll send new patches shortly, thank you!

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

[PATCH v2 1/3] UBIFS: re-arrange variables in ubifs_info

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

This is a cosmetic patch which re-arranges variables in 'struct ubifs_info'
so that all boolean-like variables which are only changed during mounting or
re-mounting to R/W mode are places together. Then they are turned into
bit-fields, which makes the structure a little bit smaller.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/ubifs.h |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 381d6b2..d1efa37 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1166,22 +1166,22 @@ struct ubifs_debug_info;
  * @rp_uid: reserved pool user ID
  * @rp_gid: reserved pool group ID
  *
- * @empty: if the UBI device is empty
+ * @empty: %1 if the UBI device is empty
+ * @need_recovery: %1 if the file-system needs recovery
+ * @replaying: %1 during journal replay
+ * @remounting_rw: %1 while re-mounting from R/O mode to R/W mode
+ * @always_chk_crc: always check CRCs (while mounting and remounting to R/W
+ *                  mode)
  * @replay_tree: temporary tree used during journal replay
  * @replay_list: temporary list used during journal replay
  * @replay_buds: list of buds to replay
  * @cs_sqnum: sequence number of first node in the log (commit start node)
  * @replay_sqnum: sequence number of node currently being replayed
- * @need_recovery: file-system needs recovery
- * @replaying: set to %1 during journal replay
  * @unclean_leb_list: LEBs to recover when re-mounting R/O mounted FS to R/W
  *                    mode
  * @rcvrd_mst_node: recovered master node to write when re-mounting R/O mounted
  *                  FS to R/W mode
  * @size_tree: inode size information for recovery
- * @remounting_rw: set while re-mounting from R/O mode to R/W mode
- * @always_chk_crc: always check CRCs (while mounting and remounting to R/W
- *                  mode)
  * @mount_opts: UBIFS-specific mount options
  *
  * @dbg: debugging-related information
@@ -1402,19 +1402,19 @@ struct ubifs_info {
 	gid_t rp_gid;
 
 	/* The below fields are used only during mounting and re-mounting */
-	int empty;
+	unsigned int empty:1;
+	unsigned int need_recovery:1;
+	unsigned int replaying:1;
+	unsigned int remounting_rw:1;
+	unsigned int always_chk_crc:1;
 	struct rb_root replay_tree;
 	struct list_head replay_list;
 	struct list_head replay_buds;
 	unsigned long long cs_sqnum;
 	unsigned long long replay_sqnum;
-	int need_recovery;
-	int replaying;
 	struct list_head unclean_leb_list;
 	struct ubifs_mst_node *rcvrd_mst_node;
 	struct rb_root size_tree;
-	int remounting_rw;
-	int always_chk_crc;
 	struct ubifs_mount_opts mount_opts;
 
 #ifdef CONFIG_UBIFS_FS_DEBUG
-- 
1.7.3.4


-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

[PATCH v2 2/3] UBIFS: introduce mounting flag

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

This is a preparational patch which removes the 'c->always_chk_crc' which was
set during mounting and remounting to R/W mode and introduces 'c->mounting'
flag which is set when mounting. Now the 'c->always_chk_crc' flag is the
same as 'c->remounting_rw && c->mounting'.

This patch is a preparation for the next one which will need to know when we
are mounting and remounting to R/W mode, which is exactly what
'c->always_chk_crc' effectively is, but its name does not suite the
next patch. The other possibility would be to just re-name it, but then
we'd end up with less logical flags coverage.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/io.c    |   12 ++++++++----
 fs/ubifs/super.c |   11 ++---------
 fs/ubifs/tnc.c   |   10 +++++++---
 fs/ubifs/ubifs.h |    5 ++---
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index d821731..d1fe562 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -88,8 +88,12 @@ void ubifs_ro_mode(struct ubifs_info *c, int err)
  * This function may skip data nodes CRC checking if @c->no_chk_data_crc is
  * true, which is controlled by corresponding UBIFS mount option. However, if
  * @must_chk_crc is true, then @c->no_chk_data_crc is ignored and CRC is
- * checked. Similarly, if @c->always_chk_crc is true, @c->no_chk_data_crc is
- * ignored and CRC is checked.
+ * checked. Similarly, if @c->mounting or @c->remounting_rw is true (we are
+ * mounting or re-mounting to R/W mode), @c->no_chk_data_crc is ignored and CRC
+ * is checked. This is because during mounting or re-mounting from R/O mode to
+ * R/W mode we may read journal nodes (when replying the journal or doing the
+ * recovery) and the journal nodes may potentially be corrupted, so checking is
+ * required.
  *
  * This function returns zero in case of success and %-EUCLEAN in case of bad
  * CRC or magic.
@@ -131,8 +135,8 @@ int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
 		   node_len > c->ranges[type].max_len)
 		goto out_len;
 
-	if (!must_chk_crc && type == UBIFS_DATA_NODE && !c->always_chk_crc &&
-	     c->no_chk_data_crc)
+	if (!must_chk_crc && type == UBIFS_DATA_NODE && !c->mounting &&
+	    !c->remounting_rw && c->no_chk_data_crc)
 		return 0;
 
 	crc = crc32(UBIFS_CRC32_INIT, buf + 8, node_len - 8);
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 91fac54..703a621 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1194,11 +1194,7 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (c->bulk_read == 1)
 		bu_init(c);
 
-	/*
-	 * We have to check all CRCs, even for data nodes, when we mount the FS
-	 * (specifically, when we are replaying).
-	 */
-	c->always_chk_crc = 1;
+	c->mounting = 1;
 
 	err = ubifs_read_superblock(c);
 	if (err)
@@ -1374,7 +1370,7 @@ static int mount_ubifs(struct ubifs_info *c)
 	if (err)
 		goto out_infos;
 
-	c->always_chk_crc = 0;
+	c->mounting = 0;
 
 	ubifs_msg("mounted UBI device %d, volume %d, name \"%s\"",
 		  c->vi.ubi_num, c->vi.vol_id, c->vi.name);
@@ -1535,7 +1531,6 @@ static int ubifs_remount_rw(struct ubifs_info *c)
 	mutex_lock(&c->umount_mutex);
 	dbg_save_space_info(c);
 	c->remounting_rw = 1;
-	c->always_chk_crc = 1;
 
 	err = check_free_space(c);
 	if (err)
@@ -1642,7 +1637,6 @@ static int ubifs_remount_rw(struct ubifs_info *c)
 	dbg_gen("re-mounted read-write");
 	c->ro_mount = 0;
 	c->remounting_rw = 0;
-	c->always_chk_crc = 0;
 	err = dbg_check_space_info(c);
 	mutex_unlock(&c->umount_mutex);
 	return err;
@@ -1659,7 +1653,6 @@ out:
 	c->ileb_buf = NULL;
 	ubifs_lpt_free(c, 1);
 	c->remounting_rw = 0;
-	c->always_chk_crc = 0;
 	mutex_unlock(&c->umount_mutex);
 	return err;
 }
diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index ad9cf01..de48597 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -447,8 +447,11 @@ static int tnc_read_node_nm(struct ubifs_info *c, struct ubifs_zbranch *zbr,
  *
  * Note, this function does not check CRC of data nodes if @c->no_chk_data_crc
  * is true (it is controlled by corresponding mount option). However, if
- * @c->always_chk_crc is true, @c->no_chk_data_crc is ignored and CRC is always
- * checked.
+ * @c->mounting or @c->remounting_rw is true (we are mounting or re-mounting to
+ * R/W mode), @c->no_chk_data_crc is ignored and CRC is checked. This is
+ * because during mounting or re-mounting from R/O mode to R/W mode we may read
+ * journal nodes (when replying the journal or doing the recovery) and the
+ * journal nodes may potentially be corrupted, so checking is required.
  */
 static int try_read_node(const struct ubifs_info *c, void *buf, int type,
 			 int len, int lnum, int offs)
@@ -476,7 +479,8 @@ static int try_read_node(const struct ubifs_info *c, void *buf, int type,
 	if (node_len != len)
 		return 0;
 
-	if (type == UBIFS_DATA_NODE && !c->always_chk_crc && c->no_chk_data_crc)
+	if (type == UBIFS_DATA_NODE && c->no_chk_data_crc && !c->mounting &&
+	    !c->remounting_rw)
 		return 1;
 
 	crc = crc32(UBIFS_CRC32_INIT, buf + 8, node_len - 8);
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index d1efa37..d182354 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1169,9 +1169,8 @@ struct ubifs_debug_info;
  * @empty: %1 if the UBI device is empty
  * @need_recovery: %1 if the file-system needs recovery
  * @replaying: %1 during journal replay
+ * @mounting: %1 while mounting
  * @remounting_rw: %1 while re-mounting from R/O mode to R/W mode
- * @always_chk_crc: always check CRCs (while mounting and remounting to R/W
- *                  mode)
  * @replay_tree: temporary tree used during journal replay
  * @replay_list: temporary list used during journal replay
  * @replay_buds: list of buds to replay
@@ -1405,8 +1404,8 @@ struct ubifs_info {
 	unsigned int empty:1;
 	unsigned int need_recovery:1;
 	unsigned int replaying:1;
+	unsigned int mounting:1;
 	unsigned int remounting_rw:1;
-	unsigned int always_chk_crc:1;
 	struct rb_root replay_tree;
 	struct list_head replay_list;
 	struct list_head replay_buds;
-- 
1.7.3.4

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

[Artem Bityutskiy]

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(-)

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:
-- 
1.7.3.4

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

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

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

[John Ogness]

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

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

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

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

[John Ogness]

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

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

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)