* [PATCH 1/2] UBIFS: fix free log space calculation
@ 2014-07-16 12:42 Artem Bityutskiy
2014-07-16 12:42 ` [PATCH 2/2] UBIFS: add a log overlap assertion Artem Bityutskiy
2014-08-11 3:21 ` [PATCH 1/2] UBIFS: fix free log space calculation hujianyang
0 siblings, 2 replies; 7+ messages in thread
From: Artem Bityutskiy @ 2014-07-16 12:42 UTC (permalink / raw)
To: hujianyang; +Cc: MTD Maling List
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Hu (hujianyang <hujianyang@huawei.com>) discovered an issue in the
'empty_log_bytes()' function, which calculates how many bytes are left in the
log:
"
If 'c->lhead_lnum + 1 == c->ltail_lnum' and 'c->lhead_offs == c->leb_size', 'h'
would equalent to 't' and 'empty_log_bytes()' would return 'c->log_bytes'
instead of 0.
"
At this point it is not clear what would be the consequences of this, and
whether this may lead to any problems, but this patch addresses the issue just
in case.
Reported-by: hujianyang <hujianyang@huawei.com>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
fs/ubifs/log.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/ubifs/log.c b/fs/ubifs/log.c
index ed24422..7e818ec 100644
--- a/fs/ubifs/log.c
+++ b/fs/ubifs/log.c
@@ -106,10 +106,14 @@ static inline long long empty_log_bytes(const struct ubifs_info *c)
h = (long long)c->lhead_lnum * c->leb_size + c->lhead_offs;
t = (long long)c->ltail_lnum * c->leb_size;
- if (h >= t)
+ if (h > t)
return c->log_bytes - h + t;
- else
+ else if (h != t)
return t - h;
+ else if (c->lhead_lnum != c->ltail_lnum)
+ return 0;
+ else
+ return c->log_bytes;
}
/**
--
1.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] UBIFS: add a log overlap assertion
2014-07-16 12:42 [PATCH 1/2] UBIFS: fix free log space calculation Artem Bityutskiy
@ 2014-07-16 12:42 ` Artem Bityutskiy
2014-07-22 1:35 ` hujianyang
2014-08-11 3:21 ` [PATCH 1/2] UBIFS: fix free log space calculation hujianyang
1 sibling, 1 reply; 7+ messages in thread
From: Artem Bityutskiy @ 2014-07-16 12:42 UTC (permalink / raw)
To: hujianyang; +Cc: MTD Maling List
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Add an assertion which checkes that the head of the log never overlaps with the
tail of the log.
Suggested-by: hujianyang <hujianyang@huawei.com>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
fs/ubifs/misc.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/ubifs/misc.h b/fs/ubifs/misc.h
index ee7cb5e..81bbf79 100644
--- a/fs/ubifs/misc.h
+++ b/fs/ubifs/misc.h
@@ -297,6 +297,7 @@ static inline int ubifs_next_log_lnum(const struct ubifs_info *c, int lnum)
if (lnum > c->log_last)
lnum = UBIFS_LOG_LNUM;
+ ubifs_assert(lnum != c->ltail_lnum);
return lnum;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UBIFS: add a log overlap assertion
2014-07-16 12:42 ` [PATCH 2/2] UBIFS: add a log overlap assertion Artem Bityutskiy
@ 2014-07-22 1:35 ` hujianyang
2014-07-28 16:18 ` Artem Bityutskiy
0 siblings, 1 reply; 7+ messages in thread
From: hujianyang @ 2014-07-22 1:35 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: MTD Maling List
On 2014/7/16 20:42, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> Add an assertion which checkes that the head of the log never overlaps with the
> tail of the log.
>
> Suggested-by: hujianyang <hujianyang@huawei.com>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
> fs/ubifs/misc.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/ubifs/misc.h b/fs/ubifs/misc.h
> index ee7cb5e..81bbf79 100644
> --- a/fs/ubifs/misc.h
> +++ b/fs/ubifs/misc.h
> @@ -297,6 +297,7 @@ static inline int ubifs_next_log_lnum(const struct ubifs_info *c, int lnum)
> if (lnum > c->log_last)
> lnum = UBIFS_LOG_LNUM;
>
> + ubifs_assert(lnum != c->ltail_lnum);
> return lnum;
> }
>
>
Hi Artem,
I just come back to work today.
I have to tell you this patch seems wrong. I've tested this patch
and saw lots of 'assert_failed()'. We use ubifs_next_log_lnum()
as an iterator to scan the hole log area and the return value of
this function may equal to c->ltail_lnum.
see:
fixup_free_space() in fs/ubifs/sb.c line 712
ubifs_log_post_commit() in fs/ubifs/log.c line 520
and so on.
How about my former patch or a new patch like this?
diff --git a/fs/ubifs/log.c b/fs/ubifs/log.c
index 7e818ec..c14628f 100644
--- a/fs/ubifs/log.c
+++ b/fs/ubifs/log.c
@@ -244,6 +244,7 @@ int ubifs_add_bud_to_log(struct ubifs_info *c, int jhead, int lnum, int offs)
if (c->lhead_offs > c->leb_size - c->ref_node_alsz) {
c->lhead_lnum = ubifs_next_log_lnum(c, c->lhead_lnum);
+ ubifs_assert(c->lhead_lnum != c->ltail_lnum);
c->lhead_offs = 0;
}
@@ -408,6 +409,7 @@ int ubifs_log_start_commit(struct ubifs_info *c, int *ltail_lnum)
/* Switch to the next log LEB */
if (c->lhead_offs) {
c->lhead_lnum = ubifs_next_log_lnum(c, c->lhead_lnum);
+ ubifs_assert(c->lhead_lnum != c->ltail_lnum);
c->lhead_offs = 0;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UBIFS: add a log overlap assertion
2014-07-22 1:35 ` hujianyang
@ 2014-07-28 16:18 ` Artem Bityutskiy
2014-07-29 10:27 ` hujianyang
0 siblings, 1 reply; 7+ messages in thread
From: Artem Bityutskiy @ 2014-07-28 16:18 UTC (permalink / raw)
To: hujianyang; +Cc: MTD Maling List
On Tue, 2014-07-22 at 09:35 +0800, hujianyang wrote:
> I have to tell you this patch seems wrong. I've tested this patch
> and saw lots of 'assert_failed()'. We use ubifs_next_log_lnum()
> as an iterator to scan the hole log area and the return value of
> this function may equal to c->ltail_lnum.
OK, thanks. I've reverted my bogus patch.
Would you please re-send your patch with proper commit message, etc, so
that I could 'git am' it. This time I'd like to apply a patch which has
been tested. Thanks!
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UBIFS: add a log overlap assertion
2014-07-28 16:18 ` Artem Bityutskiy
@ 2014-07-29 10:27 ` hujianyang
0 siblings, 0 replies; 7+ messages in thread
From: hujianyang @ 2014-07-29 10:27 UTC (permalink / raw)
To: dedekind1; +Cc: MTD Maling List
>
> Would you please re-send your patch with proper commit message, etc, so
> that I could 'git am' it. This time I'd like to apply a patch which has
> been tested. Thanks!
>
I've tested this patch and resend it.
I don't think you can just am it because you know, I'm not very
good at English. So I think you should change some comments as
before. Sorry for that, and thanks for your help these days.
I'd like to improve my English but it needs some time. -.-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] UBIFS: fix free log space calculation
2014-07-16 12:42 [PATCH 1/2] UBIFS: fix free log space calculation Artem Bityutskiy
2014-07-16 12:42 ` [PATCH 2/2] UBIFS: add a log overlap assertion Artem Bityutskiy
@ 2014-08-11 3:21 ` hujianyang
2014-09-08 9:43 ` Artem Bityutskiy
1 sibling, 1 reply; 7+ messages in thread
From: hujianyang @ 2014-08-11 3:21 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: MTD Maling List
> ---
> fs/ubifs/log.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ubifs/log.c b/fs/ubifs/log.c
> index ed24422..7e818ec 100644
> --- a/fs/ubifs/log.c
> +++ b/fs/ubifs/log.c
> @@ -106,10 +106,14 @@ static inline long long empty_log_bytes(const struct ubifs_info *c)
> h = (long long)c->lhead_lnum * c->leb_size + c->lhead_offs;
> t = (long long)c->ltail_lnum * c->leb_size;
>
> - if (h >= t)
> + if (h > t)
> return c->log_bytes - h + t;
> - else
> + else if (h != t)
> return t - h;
> + else if (c->lhead_lnum != c->ltail_lnum)
> + return 0;
> + else
> + return c->log_bytes;
> }
>
> /**
>
Hi Artem,
After almost one month test, the error I've reported
UBIFS: background thread "ubifs_bgt0_0" started, PID 2222
UBIFS: recovery needed
UBIFS error (pid 2220): replay_log_leb: first log node at LEB 4:0 is not CS node
UBIFS error (pid 2220): replay_log_leb: log error detected while replaying the log at LEB 4:0
magic 0x6101831
crc 0x81e22cd5
node_type 8 (reference node)
group_type 0 (no node group)
sqnum 2375337
len 64
lnum 5273
offs 122880
jhead 2
UBIFS: background thread "ubifs_bgt0_0" stops
never came out again.
I think this patch really solve that problem. Now I want to show my
analysis and wish you add "Cc stable" for this patch.
With the help of ubidump tool, I got the MST NODEs and LOG NODEs of
the image which is broken.
master node:
look at LEB 1:20480 (106496 bytes left)
scanning master node at LEB 1:20480
magic 0x6101831
crc 0x38ff5c67
node_type 7 (master node)
group_type 0 (no node group)
sqnum 15799802
len 512
highest_inum 161
commit number 7954
flags 0x3
log_lnum 4
root_lnum 2258
root_offs 7048
root_len 148
gc_lnum 2324
ihead_lnum 2258
ihead_offs 8192
index_size 2511408
lpt_lnum 15
lpt_offs 113571
nhead_lnum 15
nhead_offs 114688
ltab_lnum 15
ltab_offs 110592
lsave_lnum 0
lsave_offs 0
lscan_lnum 2930
leb_cnt 6844
empty_lebs 3362
idx_lebs 23
total_free 430188544
total_dirty 56069504
total_used 377968720
total_dead 7032
total_dark 31196320
look at LEB 1:20992 (105984 bytes left)
scanning padding node at LEB 1:20992
1508 bytes padded at LEB 1:20992, offset now 22528
look at LEB 1:22528 (104448 bytes left)
hit empty space at LEB 1:22528
stop scanning LEB 1 at offset 126976
log nodes:
leb number first sqnum first node type full/not full
3 15801163 ref node full
4 15801288 ref node full
5 15801433 ref node full
6 15801598 ref node full
7 15801966 ref node not full
8 15800414 ref node full
9 15800538 ref node full
10 15800664 ref node full
11 15800789 ref node full
12 15800915 ref node full
13 15801039 ref node full
The min_io_size of this device is 2048 and the leb_size of it is
126976. Each of leb is full except 7 means no frequently power
cutting happen because log leb will change into next one if the
volume is remount.
We know cs node must in leb 4 as @log_lnum is 4 in mst node. But
it is a ref node as we see it in leb 4. What happened?
Leb 7 has the highest sqnum and it is not full, that means it is
the last log leb before an unclean power off. So we know leb 8 is
the oldest log leb exist. But it has a higher sqnum than mst node,
15799802, that means mst node is written before the oldest node
in current log area.
Each time we write a cs node, we write a new mst node. The sqnum
of this mst node should higher than cs node. So the cs node in
leb 4 is overlapped.
We keep an empty leb in log area by @min_log_bytes to start-up a
new commit. When commit start, we may use this leb and set
@min_log_bytes to zero.
Suppose old ltail_lnum is 5 and lhead_lnum is 3, leb 3 is full and
a new commit start. We write a new cs node at the beginning of leb
4 and set @min_log_bytes to zero. Now, as empty_log_bytes() is wrong,
ubifs_add_bud_to_log() may wrap leb 5, leb 6... At last, overlap
the leb 4 which has the newest cs node and mst_node would point to,
why not? we don't have any assertion or any protection to void this
except the check of empty_log_bytes() with an error shows above.
Now an unfortunately power cut happens, this volume is broken.
I think this patch fixed this problem by correctly protect the log
area. So maybe you can add "Cc stable" before you send it to mainline.
By the way, commit 8989cd69b9d2 "UBIFS: fix a race condition" fix
a real potential race and useful for avoiding errors in log area.
Could you please add "Cc stable" for it too?
Thanks,
Hu
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] UBIFS: fix free log space calculation
2014-08-11 3:21 ` [PATCH 1/2] UBIFS: fix free log space calculation hujianyang
@ 2014-09-08 9:43 ` Artem Bityutskiy
0 siblings, 0 replies; 7+ messages in thread
From: Artem Bityutskiy @ 2014-09-08 9:43 UTC (permalink / raw)
To: hujianyang; +Cc: MTD Maling List
On Mon, 2014-08-11 at 11:21 +0800, hujianyang wrote:
> > ---
> > fs/ubifs/log.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ubifs/log.c b/fs/ubifs/log.c
> > index ed24422..7e818ec 100644
> > --- a/fs/ubifs/log.c
> > +++ b/fs/ubifs/log.c
> > @@ -106,10 +106,14 @@ static inline long long empty_log_bytes(const struct ubifs_info *c)
> > h = (long long)c->lhead_lnum * c->leb_size + c->lhead_offs;
> > t = (long long)c->ltail_lnum * c->leb_size;
> >
> > - if (h >= t)
> > + if (h > t)
> > return c->log_bytes - h + t;
> > - else
> > + else if (h != t)
> > return t - h;
> > + else if (c->lhead_lnum != c->ltail_lnum)
> > + return 0;
> > + else
> > + return c->log_bytes;
> > }
> >
> > /**
> >
>
> Hi Artem,
>
> After almost one month test, the error I've reported
>
> UBIFS: background thread "ubifs_bgt0_0" started, PID 2222
> UBIFS: recovery needed
> UBIFS error (pid 2220): replay_log_leb: first log node at LEB 4:0 is not CS node
> UBIFS error (pid 2220): replay_log_leb: log error detected while replaying the log at LEB 4:0
> magic 0x6101831
> crc 0x81e22cd5
> node_type 8 (reference node)
> group_type 0 (no node group)
> sqnum 2375337
> len 64
> lnum 5273
> offs 122880
> jhead 2
> UBIFS: background thread "ubifs_bgt0_0" stops
>
> never came out again.
>
> I think this patch really solve that problem. Now I want to show my
> analysis and wish you add "Cc stable" for this patch.
>
> With the help of ubidump tool, I got the MST NODEs and LOG NODEs of
> the image which is broken.
>
> master node:
>
> look at LEB 1:20480 (106496 bytes left)
> scanning master node at LEB 1:20480
> magic 0x6101831
> crc 0x38ff5c67
> node_type 7 (master node)
> group_type 0 (no node group)
> sqnum 15799802
> len 512
> highest_inum 161
> commit number 7954
> flags 0x3
> log_lnum 4
> root_lnum 2258
> root_offs 7048
> root_len 148
> gc_lnum 2324
> ihead_lnum 2258
> ihead_offs 8192
> index_size 2511408
> lpt_lnum 15
> lpt_offs 113571
> nhead_lnum 15
> nhead_offs 114688
> ltab_lnum 15
> ltab_offs 110592
> lsave_lnum 0
> lsave_offs 0
> lscan_lnum 2930
> leb_cnt 6844
> empty_lebs 3362
> idx_lebs 23
> total_free 430188544
> total_dirty 56069504
> total_used 377968720
> total_dead 7032
> total_dark 31196320
> look at LEB 1:20992 (105984 bytes left)
> scanning padding node at LEB 1:20992
> 1508 bytes padded at LEB 1:20992, offset now 22528
> look at LEB 1:22528 (104448 bytes left)
> hit empty space at LEB 1:22528
> stop scanning LEB 1 at offset 126976
>
> log nodes:
>
> leb number first sqnum first node type full/not full
> 3 15801163 ref node full
> 4 15801288 ref node full
> 5 15801433 ref node full
> 6 15801598 ref node full
> 7 15801966 ref node not full
> 8 15800414 ref node full
> 9 15800538 ref node full
> 10 15800664 ref node full
> 11 15800789 ref node full
> 12 15800915 ref node full
> 13 15801039 ref node full
>
> The min_io_size of this device is 2048 and the leb_size of it is
> 126976. Each of leb is full except 7 means no frequently power
> cutting happen because log leb will change into next one if the
> volume is remount.
>
> We know cs node must in leb 4 as @log_lnum is 4 in mst node. But
> it is a ref node as we see it in leb 4. What happened?
>
> Leb 7 has the highest sqnum and it is not full, that means it is
> the last log leb before an unclean power off. So we know leb 8 is
> the oldest log leb exist. But it has a higher sqnum than mst node,
> 15799802, that means mst node is written before the oldest node
> in current log area.
>
> Each time we write a cs node, we write a new mst node. The sqnum
> of this mst node should higher than cs node. So the cs node in
> leb 4 is overlapped.
>
> We keep an empty leb in log area by @min_log_bytes to start-up a
> new commit. When commit start, we may use this leb and set
> @min_log_bytes to zero.
>
> Suppose old ltail_lnum is 5 and lhead_lnum is 3, leb 3 is full and
> a new commit start. We write a new cs node at the beginning of leb
> 4 and set @min_log_bytes to zero. Now, as empty_log_bytes() is wrong,
> ubifs_add_bud_to_log() may wrap leb 5, leb 6... At last, overlap
> the leb 4 which has the newest cs node and mst_node would point to,
> why not? we don't have any assertion or any protection to void this
> except the check of empty_log_bytes() with an error shows above.
>
> Now an unfortunately power cut happens, this volume is broken.
>
> I think this patch fixed this problem by correctly protect the log
> area. So maybe you can add "Cc stable" before you send it to mainline.
>
> By the way, commit 8989cd69b9d2 "UBIFS: fix a race condition" fix
> a real potential race and useful for avoiding errors in log area.
> Could you please add "Cc stable" for it too?
OK, I am adding these tags:
Cc: stable@vger.kernel.org
Tested-by: hujianyang <hujianyang@huawei.com>
And putting the patches to the linux-next queue.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-08 9:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-16 12:42 [PATCH 1/2] UBIFS: fix free log space calculation Artem Bityutskiy
2014-07-16 12:42 ` [PATCH 2/2] UBIFS: add a log overlap assertion Artem Bityutskiy
2014-07-22 1:35 ` hujianyang
2014-07-28 16:18 ` Artem Bityutskiy
2014-07-29 10:27 ` hujianyang
2014-08-11 3:21 ` [PATCH 1/2] UBIFS: fix free log space calculation hujianyang
2014-09-08 9:43 ` Artem Bityutskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.