ubi: some invalid code in the function scan_peb

ubi: some invalid code in the function scan_peb

[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).

Thanks,
Ryder
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: ubi: some invalid code in the function scan_peb

[Zhihao Cheng]

在 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/

Re: ubi: some invalid code in the function scan_peb

[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.

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/

Re: ubi: some invalid code in the function scan_peb

[Zhihao Cheng]

在 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/

Re: ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability

[Zhihao Cheng]

在 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/

Re: ubifs: why ubifs doesn't support 2 copied of super blocks for better fs reliability

[Richard Weinberger]

----- 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/