[RFC PATCH] ubifs do not write master node if recovery needed

[RFC PATCH] ubifs do not write master node if recovery needed

[Sheng Yong]

Hi,
If unclean umount happens, mount ubifs may fail. At the meantime, the
master node on the flash remains dirty. But according to the current
mount procedure, a new master node will be written to the flash if we
try to mount the corrupted fs, no matter whether the recovery succeeds
or fails.

We may hope that the filesystem is not modified if the recovery fails.
However, useless master nodes keep on writing to the flash if we trying
to mount the fs serval times, like the following:

=================================
root@arm:/tmp# ./ubidump /dev/ubi0_0 -l 1 | tail -n 40
	magic          0x6101831
	crc            0x43493d53
	node_type      7 (master node)
	group_type     0 (no node group)
	sqnum          4306
	... ...
look at LEB 1:25088 (101888 bytes left)
scanning padding node at LEB 1:25088
1508 bytes padded at LEB 1:25088, offset now 26624
look at LEB 1:26624 (100352 bytes left)
hit empty space at LEB 1:26624
stop scanning LEB 1 at offset 126976
root@arm:/tmp# mount -t ubifs /dev/ubi0_0 /mnt/
[  291.923753] UBIFS error (pid 1631): ubifs_scan: garbage
[  291.999153] UBIFS error (pid 1631): ubifs_recover_leb: corruption 0
[  292.074298] UBIFS error (pid 1631): ubifs_scanned_corruption: corruption at LEB 5:4096
[  292.169248] UBIFS error (pid 1631): ubifs_scanned_corruption: first 8192 bytes from LEB 5:4096
[  292.170456] UBIFS error (pid 1631): ubifs_recover_leb: LEB 5 scanning failed
mount: mount /dev/ubi0_0 on /mnt failed: Structure needs cleaning
root@arm:/tmp# ./ubidump /dev/ubi0_0 -l 1 | tail -n 40
	magic          0x6101831
	crc            0x4f79c84e
	node_type      7 (master node)
	group_type     0 (no node group)
	sqnum          4307
	... ...
look at LEB 1:27136 (99840 bytes left)
scanning padding node at LEB 1:27136
1508 bytes padded at LEB 1:27136, offset now 28672
look at LEB 1:28672 (98304 bytes left)
hit empty space at LEB 1:28672
stop scanning LEB 1 at offset 126976
root@arm:/tmp# mount -t ubifs /dev/ubi0_0 /mnt/
[  302.376748] UBIFS error (pid 1642): ubifs_scan: garbage
[  302.452169] UBIFS error (pid 1642): ubifs_recover_leb: corruption 0
[  302.527334] UBIFS error (pid 1642): ubifs_scanned_corruption: corruption at LEB 5:4096
[  302.622285] UBIFS error (pid 1642): ubifs_scanned_corruption: first 8192 bytes from LEB 5:4096
[  302.623509] UBIFS error (pid 1642): ubifs_recover_leb: LEB 5 scanning failed
mount: mount /dev/ubi0_0 on /mnt failed: Structure needs cleaning
root@arma15el:/tmp/recovery# ./ubidump /dev/ubi0_0 -l 1 | tail -n 40
	magic          0x6101831
	crc            0x6beb031d
	node_type      7 (master node)
	group_type     0 (no node group)
	sqnum          4308
	... ...
look at LEB 1:29184 (97792 bytes left)
scanning padding node at LEB 1:29184
1508 bytes padded at LEB 1:29184, offset now 30720
look at LEB 1:30720 (96256 bytes left)
hit empty space at LEB 1:30720
stop scanning LEB 1 at offset 126976
=================================

This does not make any sense for both replay and reovery. And the
increasing sqnum makes it difficult to find out the last commit
checkpoint, as a result, we may unable to roll the fs back to last
commit state if we try to recover it.

If the recovery succeed, the master node could get updated when doing
commit next time. For normal mount, the check has no effect.

Sheng Yong (1):
  ubifs: do not write master node if recovery needed

 fs/ubifs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.3.4

[RFC PATCH] ubifs: do not write master node if recovery needed

[Sheng Yong]

If unclean umount happens, ubifs may fail when mounting. Trying to mount
it will write new master nodes on the flash. This is useless but wasting
space and increasing sqnum. So check need_recovery before writing master
node, and don't create new master node if filesystem needs recovery.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 fs/ubifs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index e642067..17e3016 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -1287,7 +1287,7 @@ static int mount_ubifs(struct ubifs_info *c)
 			goto out_lpt;
 	}
 
-	if (!c->ro_mount) {
+	if (!c->ro_mount && !c->need_recovery) {
 		/*
 		 * Set the "dirty" flag so that if we reboot uncleanly we
 		 * will notice this immediately on the next mount.
-- 
1.8.3.4

Re: [RFC PATCH] ubifs: do not write master node if recovery needed

[Artem Bityutskiy]

On Thu, 2015-02-05 at 09:54 +0000, Sheng Yong wrote:
> If unclean umount happens, ubifs may fail when mounting. Trying to mount
> it will write new master nodes on the flash. This is useless but wasting
> space and increasing sqnum. So check need_recovery before writing master
> node, and don't create new master node if filesystem needs recovery.

Looks like you caught a bug, thanks, but I need to check the code a bit
more carefully. So when will the master not be updated then, if you do
not mark it dirty now?

Artem.

Re: [RFC PATCH] ubifs: do not write master node if recovery needed

[shengyong]

在 2015/2/7 1:29, Artem Bityutskiy 写道:
> On Thu, 2015-02-05 at 09:54 +0000, Sheng Yong wrote:
>> If unclean umount happens, ubifs may fail when mounting. Trying to mount
>> it will write new master nodes on the flash. This is useless but wasting
>> space and increasing sqnum. So check need_recovery before writing master
>> node, and don't create new master node if filesystem needs recovery.
> 
> Looks like you caught a bug, thanks, but I need to check the code a bit
> more carefully. So when will the master not be updated then, if you do
> not mark it dirty now?
AFAIK, if the need_recovery is set after reading master and before writing
master, the master is always marked dirty by last mount (if unclean umount
happend), and each committing will update master later, with dirty flag set.
This is same as normal mount procedure. So I don't think we need to mark it
dirty here, and only if the recovery fails, the master will not be updated.

thanks,
Sheng
> 
> Artem.
> 
> 
>