| Message ID | 1367092363-5249-1-git-send-email-pekon@ti.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pekon, thanks for the patch. Generally, I am fine to change the reading/scanning path, but carefully. On Sun, 2013-04-28 at 01:22 +0530, Gupta, Pekon wrote: > From: "Gupta, Pekon" <pekon@ti.com> > > During mounting of UBI volume, all PEB headers are scanned and checked. > This scanning of PEB header is done > - To re-creating PEB to LEB map table. > - To filter out bad PEB or alien (non-UBI) PEB. > - recover corrupt PEB effected by sudden power-failure. > During this scanning both erase_header and volume_id_header are scanned. > This volume recovery time is critical for some safety use-cases where system > should recover as soon as possible after the fault. > > This patch tries to optimize erase-header checks done during scan by: > - re-ordering data checks based on below analysis. > - removing ubi_check_pattern(ec_hdr, 0xFF, UBI_EC_HDR_SIZE) > (checking of 0xFF in page-data) > - REASON1: even if first few bytes 'sizeof(ec_hdr)' of page-data are > 0xFF, still it does not guarantee that page is erased | blank. > - REASON2: As per analysis below, pages with invalid magic-number need > to be erased in most of the conditions. Thus explicit checking > of 0xFF in page-data can be avoided. > > MTD device driver can return following return-code for read access (mtd_read) > -------------------------------------------------------------------------- > RETURN_VALUE REASON NEXT STEP > -------------------------------------------------------------------------- > 0 no errors or bit-flips detected parse data > > EUCLEAN correctable bit-flips detected parse data & scrub PEB > > EBADMSG uncorrectable bit-flip detected parse data & scrub PEB > > <others> device error or incomplete data reject data > -------------------------------------------------------------------------- > > Parsing the read_data can result in following combinations: > -------------------------------------------------------------------------- > MAGIC > ECC HDR_CRC NUMBER CONCLUSION NEXT STEP > -------------------------------------------------------------------------- > OK valid valid* valid UBI erase_header parse header > EUCLEAN valid valid* valid UBI erase_header parse header > EBADMSG valid valid* valid UBI erase_header parse header > > OK invalid valid corrupted UBI erase_header depends on vid_hdr > EUCLEAN invalid valid corrupted UBI erase_header depends on vid_hdr > EBADMSG invalid -- undeterministic_data** schedule for erase > > OK invalid invalid undeterministic_data** schedule for erase > EUCLEAN invalid invalid undeterministic_data** schedule for erase > EBADMSG invalid invalid undeterministic_data** schedule for erase > -------------------------------------------------------------------------- > > where > 'valid*': As hdr_crc covers magic-number field so matching of hdr_crc > implicitely indicates matching of magic=number. > > underministic_data**: page-data can be any of the following > (a) programmed page (non-UBI) > (b) programmed page (all 0xFF > (c) erased page without bit-flips > (d) erased page with bit-flips > (d) valid UBI erase_header with un-recoverable bit-flips corrupting > erase-header content. > > Signed-off-by: Gupta, Pekon <pekon@ti.com> So the subject says this is an optimization, but I do not really see what did you optimize? Speed? If yes, some numbers? Or this is about improving robustness? May be some clear explanations about that? Or this is just simplifying the code and making it more clear? I am fine with all of that, just want to clearly see this explained in the commit message. Also, the patch is a bit too big. Can you try to figure out smaller steps and split it on smaller pieces? There are also minor cosmetic things, but I do not want to touch them now. Note: I am processing the mtd mailing list from oldest e-mails to newest, with few exceptions. But when I answer an e-mail, and the person re-sends patches, I usually jump to the new version right away. So if you resend, I should look at them with a lot smaller delay :-)
Hi Artem, > > Hi Pekon, > > thanks for the patch. Generally, I am fine to change the > reading/scanning path, but carefully. > [Pekon]: Yes I understand, that this patch should not break something already working. Though I have tested this with various scenarios mention in previous mail. But still I was waiting a 'Ack' from Brain. http://lists.infradead.org/pipermail/linux-mtd/2013-May/046738.html But if others can test it also, It would be good. > > > > Signed-off-by: Gupta, Pekon <pekon@ti.com> > > So the subject says this is an optimization, but I do not really see > what did you optimize? Speed? If yes, some numbers? > > Or this is about improving robustness? May be some clear explanations > about that? > > Or this is just simplifying the code and making it more clear? > > I am fine with all of that, just want to clearly see this explained in > the commit message. > [Pekon]: Its more of simplifying code, I'll re-word the commit. It should also improve some speed, in some scenarios, as I have removed all 0xFF header check. I'll try to get numbers for that. > Also, the patch is a bit too big. Can you try to figure out smaller > steps and split it on smaller pieces? > [Pekon]: Yes, I tried this, but majority chunk of patch is change in 'ubi_io_read_ec_hdr()', which was completely re-written, in order to simplify code. I'll try to split the patch, but then it might not be logical split, and there would be dependency between patches. > There are also minor cosmetic things, but I do not want to touch them > now. > [Pekon]: I'll re-base them on 3.10-rcx. So its easy for you to check. But right now I'm caught in other clean-up work, so re-submission would be bit delayed.. > Note: I am processing the mtd mailing list from oldest e-mails to > newest, with few exceptions. But when I answer an e-mail, and the > person > re-sends patches, I usually jump to the new version right away. So if > you resend, I should look at them with a lot smaller delay :-) > [Pekon]: No issues for me.. I don't see these patches as emergency, but good to have from a long term prospective. And I'm more than happy to receive feedbacks. with regards, pekon
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c index c071d41..f6a8d9e 100644 --- a/drivers/mtd/ubi/attach.c +++ b/drivers/mtd/ubi/attach.c @@ -848,6 +848,16 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai, return add_to_list(ai, pnum, UBI_UNKNOWN, UBI_UNKNOWN, UBI_UNKNOWN, 1, &ai->erase); case UBI_IO_BAD_HDR_EBADMSG: + /* un=recoverable erase-header + * unknown erase-count: can reset to mean erase-count after erase + * unknown image_seq: cannot determine if PEB was programmed + * unknown version: cannot determine UBI protocol to use + * So no point in checking vid_hdr, schedule this PEB for erase. + */ + ai->empty_peb_count += 1; + return add_to_list(ai, pnum, UBI_UNKNOWN, UBI_UNKNOWN, + UBI_UNKNOWN, 1, &ai->erase); + case UBI_IO_BAD_HDR: /* * We have to also look at the VID header, possibly it is not diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index bf79def..0b9b0af 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -537,7 +537,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum) err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0); if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR || - err1 == UBI_IO_FF) + err1 == UBI_IO_FF || err1 == UBI_IO_FF_BITFLIPS) /* * Both VID and EC headers are corrupted, so we can * safely erase this PEB and not afraid that it will be @@ -741,93 +741,71 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum, struct ubi_ec_hdr *ec_hdr, int verbose) { int err, read_err; - uint32_t crc, magic, hdr_crc; + uint32_t crc, hdr_crc; dbg_io("read EC header from PEB %d", pnum); ubi_assert(pnum >= 0 && pnum < ubi->peb_count); read_err = ubi_io_read(ubi, ec_hdr, pnum, 0, UBI_EC_HDR_SIZE); - if (read_err) { - if (read_err != UBI_IO_BITFLIPS && !mtd_is_eccerr(read_err)) - return read_err; - - /* - * We read all the data, but either a correctable bit-flip - * occurred, or MTD reported a data integrity error - * (uncorrectable ECC error in case of NAND). The former is - * harmless, the later may mean that the read data is - * corrupted. But we have a CRC check-sum and we will detect - * this. If the EC header is still OK, we just report this as - * there was a bit-flip, to force scrubbing. - */ - } - - magic = be32_to_cpu(ec_hdr->magic); - if (magic != UBI_EC_HDR_MAGIC) { - if (mtd_is_eccerr(read_err)) - return UBI_IO_BAD_HDR_EBADMSG; - - /* - * The magic field is wrong. Let's check if we have read all - * 0xFF. If yes, this physical eraseblock is assumed to be - * empty. - */ - if (ubi_check_pattern(ec_hdr, 0xFF, UBI_EC_HDR_SIZE)) { - /* The physical eraseblock is supposedly empty */ - if (verbose) - ubi_warn("no EC header found at PEB %d, only 0xFF bytes", - pnum); - dbg_bld("no EC header found at PEB %d, only 0xFF bytes", - pnum); - if (!read_err) - return UBI_IO_FF; - else - return UBI_IO_FF_BITFLIPS; - } - - /* - * This is not a valid erase counter header, and these are not - * 0xFF bytes. Report that the header is corrupted. - */ - if (verbose) { - ubi_warn("bad magic number at PEB %d: %08x instead of %08x", - pnum, magic, UBI_EC_HDR_MAGIC); - ubi_dump_ec_hdr(ec_hdr); - } - dbg_bld("bad magic number at PEB %d: %08x instead of %08x", - pnum, magic, UBI_EC_HDR_MAGIC); - return UBI_IO_BAD_HDR; - } - - crc = crc32(UBI_CRC32_INIT, ec_hdr, UBI_EC_HDR_SIZE_CRC); - hdr_crc = be32_to_cpu(ec_hdr->hdr_crc); - - if (hdr_crc != crc) { - if (verbose) { - ubi_warn("bad EC header CRC at PEB %d, calculated %#08x, read %#08x", - pnum, crc, hdr_crc); - ubi_dump_ec_hdr(ec_hdr); + switch (read_err) { + case -EBADMSG: + /* un-correctable bit-flips detected + * Case-1: uncorrectable bit-flip within erase-header bytes + * hdr_crc with fail. erase-header underministic. + * Case-2: uncorrectable bit-flip outside erase-header bytes + * hdr_crc will match. erase-header can be parsed. + * Case-3: 'unstable bit-flip issue' + * power-failure during programming of erase-header. + * So even if hdr_crc matches, information may get corrupt + * later during future reads, like when scubbing this PEB, + * thereby causing leakage of information into next PEB */ + crc = crc32(UBI_CRC32_INIT, ec_hdr, UBI_EC_HDR_SIZE_CRC); + hdr_crc = be32_to_cpu(ec_hdr->hdr_crc); + if (hdr_crc == crc) + /* implicitely checks magic-number also */ + goto good_peb; + else + /* undeterministic data, magic-number can be corrupt */ + return UBI_IO_BAD_HDR_EBADMSG; + break; + case 0: + case UBI_IO_BITFLIPS: + crc = crc32(UBI_CRC32_INIT, ec_hdr, UBI_EC_HDR_SIZE_CRC); + hdr_crc = be32_to_cpu(ec_hdr->hdr_crc); + /* In a stable UBI system, most of PEB will match hdr_crc */ + if (likely(hdr_crc == crc)) { + /* magic-numnber check implicetely covered in CRC */ + goto good_peb; + } else if (UBI_EC_HDR_MAGIC == be32_to_cpu(ec_hdr->magic)) { + /* seems vid-hdr */ + return UBI_IO_BAD_HDR; + } else { + /* Not a vid-hdr. + * - programmed page with non-UBI data + * - erased blank page + */ + if (verbose) { + ubi_warn("bad magic number in ec-hdr of PEB %d" + , pnum); + ubi_dump_ec_hdr(ec_hdr); + } + dbg_bld("bad magic number in ec-hdr of PEB %d", pnum); + return UBI_IO_BAD_HDR_EBADMSG; } - dbg_bld("bad EC header CRC at PEB %d, calculated %#08x, read %#08x", - pnum, crc, hdr_crc); + break; - if (!read_err) - return UBI_IO_BAD_HDR; - else - return UBI_IO_BAD_HDR_EBADMSG; + default: + /* -EIO (may be incomplete page reads) */ + return read_err; } - /* And of course validate what has just been read from the media */ +good_peb: err = validate_ec_hdr(ubi, ec_hdr); if (err) { ubi_err("validation failed for PEB %d", pnum); return -EINVAL; } - - /* - * If there was %-EBADMSG, but the header CRC is still OK, report about - * a bit-flip to force scrubbing on this PEB. - */ + /* UBI_IO_BITFLIPS it will force scrubbing this PEB*/ return read_err ? UBI_IO_BITFLIPS : 0; }