Re: [RFC PATCH 2/2] mtd: nand: use nand_check_erased_ecc_chunk in default ECC read functions

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Adding Artem and Richard in the loop.

On Mon, 3 Aug 2015 13:16:02 +0200
Andrea Scian <rnd4@dave-tech.it> wrote:

> 
> Dear Boris,
> 
> Il 31/07/2015 18:27, Boris Brezillon ha scritto:
> > On Fri, 31 Jul 2015 18:19:30 +0200
> > Andrea Scian <rnd4@dave-tech.it> wrote:
> >
> >> Il 31/07/2015 16:10, Boris Brezillon ha scritto:
> >>> On Fri, 31 Jul 2015 15:40:13 +0200
> >>> Andrea Scian <rnd4@dave-tech.it> wrote:
> >>>
> >>>> Boris,
> >>>>
> >>>> Il 31/07/2015 12:32, Boris Brezillon ha scritto:
> >>>>> Hi Andrea,
> >>>>>
> >>>>> Adding Han in Cc.
> >>>>>
> >>>>> On Fri, 31 Jul 2015 12:07:21 +0200
> >>>>> Andrea Scian <rnd4@dave-tech.it> wrote:
> >>>>>
> >>>>>> Dear Boris,
> >>>>>>
> >>>>>>
> >>>>>> Il 30/07/2015 19:34, Boris Brezillon ha scritto:
> >>>>>>> The default NAND read functions are relying on an underlying controller
> >>>>>>> to correct bitflips, but some of those controller cannot properly fix
> >>>>>>> bitflips in erased pages.
> >>>>>>> In case of ECC failures, check if the page of subpage is empty before
> >>>>>>> reporting an ECC failure.
> >>>>>> I'm still wondering if chip->ecc.strength is the right threshold.
> >>>>>>
> >>>>>> Did you see my comments here [1]? WDYT?
> >>>>> Yes I've read it, and decided to go for ecc->strength as a first
> >>>>> step (I'm more interested in discussing the approach than the threshold
> >>>>> value right now ;-)).
> >>>> I perfectly understand, that's the reason why I ask if you want to move
> >>>> to another thread ;-)
> >>>>
> >>>>> Anyway, as you pointed out in the thread, writing data on an erased
> >>>>> page already containing some bitflips might generate even more
> >>>>> bitflips, so using a different threshold for the erased page check
> >>>>> makes sense. This threshold should definitely be correlated to the ECC
> >>>>> strength, but how, that's the question.
> >>>>>
> >>>>> How about taking a rather conservative value like 10% of the specified
> >>>>> ECC strength, and see how it goes.
> >>>> Yes, I think that there's no real way to get the right value, other than
> >>>> feedbacks from on-field testing with various devices.
> >>>>
> >>>> I'm also thinking about changing how a NAND page is written on the
> >>>> device, now that we know that even erased page may have (too many!)
> >>>> bitflips if they has not been so-freshly erased.
> >>>>
> >>>> Read on NAND device is lot's faster that write, so maybe we can:
> >>>>
> >>>> a) read the page before write it, check for bitflips on erased area and
> >>>> write it only if it fit our threshold
> >>>>
> >>>> b) read the page after write it and check if the bitflips are lower that
> >>>> a give value
> >>>>
> >>>> In this way:
> >>>> - we can use ecc_strength as read threshold, because it fits all the
> >>>> other NAND read
> >>>>
> >>>> - we can use "something a bit lower than" mtd->bitflip_threshold on
> >>>> read-before-write or read-after-write. If we don't do so the block will
> >>>> be scrubbed next time we read it again (if we are lucky.. if we are
> >>>> unlucky the block will have bitflip > ecc_strength!): IOW we did a write
> >>>> that will trigger another erase/write cycle.
> >>>>
> >>>> Am I misunderstanding something?
> >>> Nope, but this implies doing an extra read after each write :-/
> >>>
> >> Let's wait what the others says about this, but I would like to put some
> >> numbers in it.
> > Sure.
> >
> >> My micron MLC device says
> >> - read page max 75 uS
> >> - write page typ 1300uS, max 2600uS
> >>
> >> If we implement read-before-write (which is, IMO, the best approach), in
> >> the worst overhead we have is 1375uS vs 1300uS, which is ~6%.
> >> Please note that, if you read a page that "is not suitable" for write,
> >> you avoid the write time, schedule it for scrubbing, and use another
> >> free page.
> > Indeed, that's not such a big overhead.
> >
> >> Probably I'm a bit optimistic because we also need to take in account
> >> other latencies (DMA setup, ECC engine, buffer copies and so on) but
> >> it's a starting point ;-)
> > Yep, if you test it, could you provide some speed test results (with
> > and without this solution).
> 
> I think I can find some time to do some performance tests on real hardware.
> Can you please help me in finding:
> - which benchmark to use (currently I'm using bonnie++ on UBIFS, maybe I 
> can you just mtd_speedtest)
> - where to implement those read

I think the test should be done at the UBI layer if we want to check
the real impact of the additional read sequence, but given the answer I
gave to your other question I'm not sure this is relevant anymore ;-).

> 
> For the second point I think we can implement it a UBI or MTD level.
> I think the former will allow us to easily schedule scrubbing and choose 
> another block to issue the write to. However I don't really know how to 
> implement it (I don't really know so much about the UBI code).
> 
> The latter, at least for me, is easier to implement: I think I can find 
> the place to add the page read on my own, anyway any clue is welcome ;-)
> But I think it will be harder (or impossible) to choose where to issue 
> the write, unless UBI already do so when it saw an MTD write failure.
> 
> > And I wonder if we shouldn't do it the other way around, write then
> > read-back and check the content.
> > Of course this implies doing the extra write even when the erased page
> > contains too many bitflips, but at least your sure that the data you
> > put in the page were correct at that time.
> >
> 
> You're right, I think this is something that once we can find inside the 
> MTD code (something like "check NAND written page" kconfig option) but I 
> cannot find this option anymore on latest kernel.
> 
> You're approach will also have another advantage, currently nearly all 
> platform will use software implementation for ECC check on erased 
> blocks, while nearly all of them has hardware ECC once programmed. Using 
> hardware ECC will remove CPU load and, maybe, be faster than call 
> nand_check_erased_ecc_chunk()
> I also think that the situation of having failure on write is very 
> unlikely, unless you have a very "used" NAND device or you're not using 
> Richard's bitrot check. So we'll have a performance impact (issuing 
> another write) only when it's really needed.

I didn't check before suggesting that, but it seems that the UBI layer
is already doing this check for you [1], so if you're using UBI/UBIFS
you shouldn't worry about bitflips in erased pages: if there is any,
and their presence impact the write result, they should be detected.
AFAICT, the only thing that is not checked is whether the number of
bitflips after a write exceed the bitflips threshold or not, and I
guess this can be added.

Best Regards,

Boris

[1]http://lxr.free-electrons.com/source/drivers/mtd/ubi/io.c#L294

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com