* ubi: some invalid code in the function scan_peb @ 2023-10-18 5:59 Ryder Wang 2023-10-18 6:31 ` Zhihao Cheng 0 siblings, 1 reply; 6+ messages in thread From: Ryder Wang @ 2023-10-18 5:59 UTC (permalink / raw) To: linux-mtd@lists.infradead.org I just find some dead code in the function scan_peb in drvier/mtd/ubi/attach.c. In the function scan_peb, it calls validate_ec_hdr which calls validate_ec_hdr. If ubi version or erase count is incorrect, it will report -EINVAL and abort the function scan_peb. However, ubi version or erase count will be checked again in scan_peb and such check code will never find the errors because such errors should have been detected by validate_ec_hdr (aborting). Thanks, Ryder ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ubi: some invalid code in the function scan_peb 2023-10-18 5:59 ubi: some invalid code in the function scan_peb Ryder Wang @ 2023-10-18 6:31 ` Zhihao Cheng 2023-10-18 7:28 ` Ryder Wang 0 siblings, 1 reply; 6+ messages in thread From: Zhihao Cheng @ 2023-10-18 6:31 UTC (permalink / raw) To: Ryder Wang, linux-mtd@lists.infradead.org 在 2023/10/18 13:59, Ryder Wang 写道: > I just find some dead code in the function scan_peb in drvier/mtd/ubi/attach.c. > > In the function scan_peb, it calls validate_ec_hdr which calls validate_ec_hdr. If ubi version or erase count is incorrect, it will report -EINVAL and abort the function scan_peb. However, ubi version or erase count will be checked again in scan_peb and such check code will never find the errors because such errors should have been detected by validate_ec_hdr (aborting). > Yes, redundant checking indeed exists in scan_peb(), could you send a patch to clean it up? ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ubi: some invalid code in the function scan_peb 2023-10-18 6:31 ` Zhihao Cheng @ 2023-10-18 7:28 ` Ryder Wang 2023-10-18 7:42 ` Zhihao Cheng [not found] ` <MEYP282MB3164987D45E3CFC61B3E7CECBFD02@MEYP282MB3164.AUSP282.PROD.OUTLOOK.COM> 0 siblings, 2 replies; 6+ messages in thread From: Ryder Wang @ 2023-10-18 7:28 UTC (permalink / raw) To: Zhihao Cheng, linux-mtd@lists.infradead.org The patch below can remove the invalid code. It can pass ubi code build and ubi attach can also be OK from the testing. diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c index 10b2459f8951..8268ca6a91d1 --- a/drivers/mtd/ubi/attach.c +++ b/drivers/mtd/ubi/attach.c @@ -995,28 +995,6 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai, if (!ec_err) { int image_seq; - /* Make sure UBI version is OK */ - if (ech->version != UBI_VERSION) { - ubi_err(ubi, "this UBI version is %d, image version is %d", - UBI_VERSION, (int)ech->version); - return -EINVAL; - } - - ec = be64_to_cpu(ech->ec); - if (ec > UBI_MAX_ERASECOUNTER) { - /* - * Erase counter overflow. The EC headers have 64 bits - * reserved, but we anyway make use of only 31 bit - * values, as this seems to be enough for any existing - * flash. Upgrade UBI and use 64-bit erase counters - * internally. - */ - ubi_err(ubi, "erase counter overflow, max is %d", - UBI_MAX_ERASECOUNTER); - ubi_dump_ec_hdr(ech); - return -EINVAL; - } - /* * Make sure that all PEBs have the same image sequence number. * This allows us to detect situations when users flash UBI From: Zhihao Cheng <chengzhihao1@huawei.com> Sent: Wednesday, October 18, 2023 14:31 To: Ryder Wang <rydercoding@hotmail.com>; linux-mtd@lists.infradead.org <linux-mtd@lists.infradead.org> Subject: Re: ubi: some invalid code in the function scan_peb 在 2023/10/18 13:59, Ryder Wang 写道: > I just find some dead code in the function scan_peb in drvier/mtd/ubi/attach.c. > > In the function scan_peb, it calls validate_ec_hdr which calls validate_ec_hdr. If ubi version or erase count is incorrect, it will report -EINVAL and abort the function scan_peb. However, ubi version or erase count will be checked again in scan_peb and such check code will never find the errors because such errors should have been detected by validate_ec_hdr (aborting). > Yes, redundant checking indeed exists in scan_peb(), could you send a patch to clean it up? ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: ubi: some invalid code in the function scan_peb 2023-10-18 7:28 ` Ryder Wang @ 2023-10-18 7:42 ` Zhihao Cheng [not found] ` <MEYP282MB3164987D45E3CFC61B3E7CECBFD02@MEYP282MB3164.AUSP282.PROD.OUTLOOK.COM> 1 sibling, 0 replies; 6+ messages in thread From: Zhihao Cheng @ 2023-10-18 7:42 UTC (permalink / raw) To: Ryder Wang, linux-mtd@lists.infradead.org 在 2023/10/18 15:28, Ryder Wang 写道: > The patch below can remove the invalid code. It can pass ubi code build and ubi attach can also be OK from the testing. > Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com> > diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c > index 10b2459f8951..8268ca6a91d1 > --- a/drivers/mtd/ubi/attach.c > +++ b/drivers/mtd/ubi/attach.c > @@ -995,28 +995,6 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai, > if (!ec_err) { > int image_seq; > > - /* Make sure UBI version is OK */ > - if (ech->version != UBI_VERSION) { > - ubi_err(ubi, "this UBI version is %d, image version is %d", > - UBI_VERSION, (int)ech->version); > - return -EINVAL; > - } > - > - ec = be64_to_cpu(ech->ec); > - if (ec > UBI_MAX_ERASECOUNTER) { > - /* > - * Erase counter overflow. The EC headers have 64 bits > - * reserved, but we anyway make use of only 31 bit > - * values, as this seems to be enough for any existing > - * flash. Upgrade UBI and use 64-bit erase counters > - * internally. > - */ > - ubi_err(ubi, "erase counter overflow, max is %d", > - UBI_MAX_ERASECOUNTER); > - ubi_dump_ec_hdr(ech); > - return -EINVAL; > - } > - > /* > * Make sure that all PEBs have the same image sequence number. > * This allows us to detect situations when users flash UBI > > > From: Zhihao Cheng <chengzhihao1@huawei.com> > Sent: Wednesday, October 18, 2023 14:31 > To: Ryder Wang <rydercoding@hotmail.com>; linux-mtd@lists.infradead.org <linux-mtd@lists.infradead.org> > Subject: Re: ubi: some invalid code in the function scan_peb > > 在 2023/10/18 13:59, Ryder Wang 写道: >> I just find some dead code in the function scan_peb in drvier/mtd/ubi/attach.c. >> >> In the function scan_peb, it calls validate_ec_hdr which calls validate_ec_hdr. If ubi version or erase count is incorrect, it will report -EINVAL and abort the function scan_peb. However, ubi version or erase count will be checked again in scan_peb and such check code will never find the errors because such errors should have been detected by validate_ec_hdr (aborting). >> > > Yes, redundant checking indeed exists in scan_peb(), could you send a > patch to clean it up? > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <MEYP282MB3164987D45E3CFC61B3E7CECBFD02@MEYP282MB3164.AUSP282.PROD.OUTLOOK.COM>]
* Re: ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability [not found] ` <MEYP282MB3164987D45E3CFC61B3E7CECBFD02@MEYP282MB3164.AUSP282.PROD.OUTLOOK.COM> @ 2024-06-29 11:22 ` Zhihao Cheng 2024-06-29 13:56 ` Richard Weinberger 0 siblings, 1 reply; 6+ messages in thread From: Zhihao Cheng @ 2024-06-29 11:22 UTC (permalink / raw) To: Ryder Wang, linux-mtd@lists.infradead.org, Richard Weinberger 在 2024/6/28 10:28, Ryder Wang 写道: > Hello, > > It looks ubifs has only 1 super block. Super block may be corrupted by power-cut while NAND erasing or writing, ECC error and etc. When super block is corrupted, ubifs mount will always fail. > Have you met the problem caused by corrupted superblock? I only saw once in our product, in which there ware many problem eraseblocks in the flash. > I am asking why ubifs doesn't support 2 copied of super blocks for better fs reliability? As we know, both uibfs master block and ubi volume table have 2 copies, so they are very stronger. Is there any dev plan to support it on super block? I have no ideas about this question either after looking through the history and whitepaper[1]. The superblock is updated in a very low frequency(eg. auto-resize), and it is updated by atomic changing. So I guess that corrupted superblock is hardly generated from the view of UBIFS layer. However, the UBI will do wear-leveing, which could atomically exchange two LEBs(may contain superblock), then the updating frequency of superblock could be increased. The final probability of corrupted superblock is hard to say. So, I'm not sure it's a very valuable thing to maintain two superblocks in UBIFS, afterall, it will change disk structure. [1] http://www.linux-mtd.infradead.org/doc/ubifs_whitepaper.pdf > > Best regards, > Ryder > . > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability 2024-06-29 11:22 ` ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability Zhihao Cheng @ 2024-06-29 13:56 ` Richard Weinberger 0 siblings, 0 replies; 6+ messages in thread From: Richard Weinberger @ 2024-06-29 13:56 UTC (permalink / raw) To: chengzhihao1; +Cc: Ryder Wang, linux-mtd ----- Ursprüngliche Mail ----- > Von: "chengzhihao1" <chengzhihao1@huawei.com> > An: "Ryder Wang" <rydercoding@hotmail.com>, "linux-mtd" <linux-mtd@lists.infradead.org>, "richard" <richard@nod.at> > Gesendet: Samstag, 29. Juni 2024 13:22:34 > Betreff: Re: ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability > 在 2024/6/28 10:28, Ryder Wang 写道: >> Hello, >> >> It looks ubifs has only 1 super block. Super block may be corrupted by power-cut No. The super block must not get corrupted by a sudden loss of power. If you're facing this, something else in your storage stack is broken. >> while NAND erasing or writing, ECC error and etc. When super block is >> corrupted, ubifs mount will always fail. >> > > Have you met the problem caused by corrupted superblock? I only saw once > in our product, in which there ware many problem eraseblocks in the flash. > >> I am asking why ubifs doesn't support 2 copied of super blocks for better fs >> reliability? As we know, both uibfs master block and ubi volume table have 2 >> copies, so they are very stronger. Is there any dev plan to support it on super >> block? > > I have no ideas about this question either after looking through the > history and whitepaper[1]. The superblock is updated in a very low > frequency(eg. auto-resize), and it is updated by atomic changing. So I > guess that corrupted superblock is hardly generated from the view of > UBIFS layer. However, the UBI will do wear-leveing, which could > atomically exchange two LEBs(may contain superblock), then the updating > frequency of superblock could be increased. The final probability of > corrupted superblock is hard to say. So, I'm not sure it's a very > valuable thing to maintain two superblocks in UBIFS, afterall, it will > change disk structure. UBIFS relies on working flash memory, so having multiple copies makes no sense since one of UBIFS' goals is also being space efficient. There is one exception: it has two master LEBs. For historic reasons, it needs to exchange theirs contents automatically, at the time when UBIFS was young, UBI had no atomic-leb-change feature. Thanks, //richard ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-29 13:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-18 5:59 ubi: some invalid code in the function scan_peb Ryder Wang
2023-10-18 6:31 ` Zhihao Cheng
2023-10-18 7:28 ` Ryder Wang
2023-10-18 7:42 ` Zhihao Cheng
[not found] ` <MEYP282MB3164987D45E3CFC61B3E7CECBFD02@MEYP282MB3164.AUSP282.PROD.OUTLOOK.COM>
2024-06-29 11:22 ` ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability Zhihao Cheng
2024-06-29 13:56 ` Richard Weinberger
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.