Re: [PATCH resend] UBIFS: return -EINVAL if first log leb is empty

[Artem Bityutskiy <dedekind1@gmail.com>]

Hi Hujianyang,

On Sat, 2015-01-31 at 11:43 +0800, hujianyang wrote:
> CS node is recognized as a sign in UBIFS log replay mechanism.
> Log relaying during mount should find the CS node in first log
> leb at beginning and then replay the following uncommitted
> buds.

Lets use the "log head" term instead of "the first log LEB", just to be
consistent.

> Here is a bug in log replay path: If first log leb, which is
> indicated by @log_lnum in mst_node, is empty, current UBIFS
> replay nothing and directly mount the partition without any
> warning. This action will put filesystem in an abnormal state,
> e.g. space management in LPT area is incorrect to the real
> space usage in main area.

Looks like a good catch, thank you! I have few requests, though.

> 
>  	if (sleb->nodes_cnt == 0) {
> -		err = 1;
> +		if (unlikely(c->cs_sqnum == 0)) {

Please, do not use unlikely(), it may only be used on hot-paths, which
the replay code is certainly not.

IIUC, this if translates to "is this the head of the log?". If I am
right, then the better place for doing this check is the caller function
- 'ubifs_replay_journal()'. In that function you just do

if (lnum == c->lhead_lnum)

which is way easier to understand than "if (c->cs_sqnum == 0)", I think.

> +			/* This is the first log LEB, should not be empty */

I'd expand this comment a bit to:

The head of the log must always start with the "commit start" node on a
properly formatted UBIFS. But we found no nodes at all, which means that
something went wrong and we cannot proceed mounting the file-system.

I think the next developer who looks at the will appreciate these kinds
of commentaries.

> +			ubifs_err("first log leb LEB %d:%d is empty, no CS node exist",
> +				  lnum, offs);

Lets use consistent terminology. We call it the "log head".

I'd phrase it like this:

no UBIFS nodes found at the log head LEB %d:%d, possibly the FS is
corrupted.