| Message ID | 54CC4F77.1000801@huawei.com |
|---|---|
| State | Deferred |
| Headers | show |
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.
On 2015/1/31 17:51, Artem Bityutskiy wrote: > 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. > OK. >> + 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. > That's true~! >> + /* 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. > Thanks~! > 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. > Artem, Thanks for your reviewing. Could I add a Reviewed-by after resending a v2 patch? Sorry to say I'm too tired today and still have other stuff needs to be done before my leave. I'd like to recheck your comments and resend this patch next Monday. Thank you very much~! Hu
On Sat, 2015-01-31 at 18:34 +0800, hujianyang wrote: > Artem, Thanks for your reviewing. Could I add a Reviewed-by after resending > a v2 patch? Well, I do want to help you to have things done quicker by letting you adding the Reviewed-by tag already in v2. But the protocol is the protocol, the Reviewed-by tag is usually something the reviewer replies with when he sees the final version of the patch. Besides, I am still the one who merges UBIFS stuff. I am letting Richard to be the UBI master so far, since this is the area he demonstrated the excellence at, not UBIFS. UBIFS and UBI are very different, just like MTD and UBI. Knowing one of them well does not make you automatically fluent in the other. So the layout I proposed Richard was: 1. Richard owns UBI, I most probably will slowly shade away from that area. 2. Richard owns mtd-utils 3. I still own UBIFS, but will be ready to hand it over to the next brave one. <sentiment> UBI is my baby, it is my first serious piece of work I've done. Working with smart people from Linutronix, for IBM was fantastic! But UBI is "grown up" and it is time for me to move forward. I am very happy to have Richard to take it over, thanks Richard. Maintain it smartly, make sure your users are happy. I will be glad to help with architectural/design things. Whenever there is a debate, or a need in an educated opinion, I'll be happy to give my input. </sentiment>
On 2015/1/31 20:16, Artem Bityutskiy wrote: > On Sat, 2015-01-31 at 18:34 +0800, hujianyang wrote: >> Artem, Thanks for your reviewing. Could I add a Reviewed-by after resending >> a v2 patch? > > Well, I do want to help you to have things done quicker by letting you > adding the Reviewed-by tag already in v2. But the protocol is the > protocol, the Reviewed-by tag is usually something the reviewer replies > with when he sees the final version of the patch. > Sure. I just thought it's a simple problem and I could fix it as you suggested in v2. Since I'm not a careful man, I wish you could keep on commenting my v2 patch. Thanks for your help in this patch again. > Besides, I am still the one who merges UBIFS stuff. I am letting Richard > to be the UBI master so far, since this is the area he demonstrated the > excellence at, not UBIFS. > > UBIFS and UBI are very different, just like MTD and UBI. Knowing one of > them well does not make you automatically fluent in the other. > > So the layout I proposed Richard was: > > 1. Richard owns UBI, I most probably will slowly shade away from that > area. > 2. Richard owns mtd-utils > 3. I still own UBIFS, but will be ready to hand it over to the next > brave one. > I think you are busy with other stuff these days. I'm sorry to disturb you a lot. > <sentiment> > UBI is my baby, it is my first serious piece of work I've done. Working > with smart people from Linutronix, for IBM was fantastic! > > But UBI is "grown up" and it is time for me to move forward. I am very > happy to have Richard to take it over, thanks Richard. Maintain it > smartly, make sure your users are happy. > > I will be glad to help with architectural/design things. Whenever there > is a debate, or a need in an educated opinion, I'll be happy to give my > input. > </sentiment> > > > Thanks~! Another problem. Someone <markus.heininger@online.de> asked: "UBIFS: Is it possible to get the compressed size of a file?" I think it's an interesting problem. Actually we used to separate an UBI device into several volumes for space management reason. So could we realize the compressed size getting mechanism and import *quota* to UBIFS itself. BR, Hu
Am 02.02.2015 um 04:10 schrieb hujianyang: > Another problem. Someone <markus.heininger@online.de> asked: > > "UBIFS: Is it possible to get the compressed size of a file?" > > I think it's an interesting problem. Actually we used to separate an UBI > device into several volumes for space management reason. So could we > realize the compressed size getting mechanism and import *quota* to UBIFS > itself. You can get the actual size by accumulating the UBIFS data node sizes. But this means that you have to load the actual data node which is rather expensive. I had a short chat with Artem on this, we agreed that a nice solution would be to store the actual size in an inode. But that is a non-trivial change to UBIFS. Thanks, //richard
Hi Richard, On 2015/2/2 16:20, Richard Weinberger wrote: > Am 02.02.2015 um 04:10 schrieb hujianyang: >> Another problem. Someone <markus.heininger@online.de> asked: >> >> "UBIFS: Is it possible to get the compressed size of a file?" >> >> I think it's an interesting problem. Actually we used to separate an UBI >> device into several volumes for space management reason. So could we >> realize the compressed size getting mechanism and import *quota* to UBIFS >> itself. > > You can get the actual size by accumulating the UBIFS data node sizes. and xattr node sizes too? may store in another variable. > But this means that you have to load the actual data node which is rather > expensive. I had a short chat with Artem on this, we agreed that a nice solution > would be to store the actual size in an inode. But that is a non-trivial change > to UBIFS. > Yes, old images may not store the actual size and it's hard to get compressed size from this kind of images by a new driver. Users may get actual size from an ioctl call and maybe we could return an error if this feature is not supported. How about increasing file-system driver version and applying actual size support in a higher version? or mark this feature as a flag in super_node? Another 'expensive' implement is whole scan the partition to get the actual size of a required file in the old images. What's your plan after that chat? BR, Hu
diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c index 3187925..f13f4b2 100644 --- a/fs/ubifs/replay.c +++ b/fs/ubifs/replay.c @@ -846,7 +846,14 @@ static int replay_log_leb(struct ubifs_info *c, int lnum, int offs, void *sbuf) } if (sleb->nodes_cnt == 0) { - err = 1; + if (unlikely(c->cs_sqnum == 0)) { + /* This is the first log LEB, should not be empty */ + ubifs_err("first log leb LEB %d:%d is empty, no CS node exist", + lnum, offs); + err = -EINVAL; + } else { + err = 1; + } goto out; }
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. 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. We reproduced this bug by fault injection: turn first log leb into all 0xFF. UBIFS driver mount the polluted partition normally. But errors occur while running fs_stress on this mount: [89068.055183] UBI error: ubi_io_read: error -74 (ECC error) while reading 59 bytes from PEB 711:33088, read 59 bytes [89068.179877] UBIFS error (pid 10517): ubifs_check_node: bad magic 0x101031, expected 0x6101831 [89068.179882] UBIFS error (pid 10517): ubifs_check_node: bad node at LEB 591:28992 [89068.179891] Not a node, first 24 bytes: [89068.179892] 00000000: 31 10 10 00 37 84 64 04 10 04 00 00 00 00 00 00 20 00 00 00 02 01 00 00 1...7.d......... ....... [89068.180282] UBIFS error (pid 10517): ubifs_read_node: expected node type 2 This patch fix the problem by checking c->cs_sqnum to guarantee the empty leb is not first log leb and return an error if the first log leb is incorrectly empty. After this, we could catch *first log empty* error in place. Signed-off-by: hujianyang <hujianyang@huawei.com> --- fs/ubifs/replay.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)