Re: [EXT] [PATCH v2 3/3] mtd: rawnand: micron: Address the shallow erase issue

From: Boris Brezillon <boris.brezillon@collabora.com>
To: "Bean Huo (beanhuo)" <beanhuo@micron.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	Steve deRosier <derosier@gmail.com>,
	"Zoltan Szubbocsev \(zszubbocsev\)" <zszubbocsev@micron.com>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	Piotr Wojtaszczyk <WojtaszczykP@cumminsallison.com>
Subject: Re: [EXT] [PATCH v2 3/3] mtd: rawnand: micron: Address the shallow erase issue
Date: Wed, 6 May 2020 18:04:20 +0200	[thread overview]
Message-ID: <20200506180420.13996633@collabora.com> (raw)
In-Reply-To: <BN7PR08MB5684D8DFC50CB93B53705619DBA40@BN7PR08MB5684.namprd08.prod.outlook.com>

On Wed, 6 May 2020 15:50:32 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris
> 
> > 
> > On Wed, 6 May 2020 08:28:43 +0000
> > "Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:
> >   
> > > Hi, Miquel
> > > I have two questions about your patch, please help me.
> > >  
> > > > +	 */
> > > > +	for (eb = first_eb; eb < first_eb + nb_eb; eb++) {
> > > > +		/* Il all the first pages are not written yet, do it */
> > > > +		if (micron->writtenp[eb] != MICRON_PAGE_MASK_TRIGGER)
> > > > +			micron_nand_avoid_shallow_erase(chip, eb);
> > > > +
> > > > +		micron->writtenp[eb] = 0;
> > > > +	}  
> > >
> > >
> > > Here, if the power loss happens before erasing this block, for the
> > > next time boot up, What will happen from FS layer in case FS detect this filled  
> > data?
> > 
> > Most likely ECC errors will be returned, but that doesn't matter since this block
> > was about to be erased. You have pretty much the same problem for partially
> > erase blocks already, and that should be handled by the wear-leveling/FS, if not,
> > that would be bug (note that it's properly handled by UBI, which just considers
> > the block as invalid and schedules an erase).
> >   
> 
> Concerning this, I still have question, for the UBIFS,  If I am correct, there are
> EC and VID header both being damaged, then UBIFS will re-erase it. I don't know if
> UBIFS can handle there is dirty/filling data in the some pages  and EC/VID valid.
> Maybe Richard has fixed it.

If the block is being erased that means there's another one mapped to
the same LEB, or the block is simply not needed anymore. In both cases,
this old block shouldn't be referenced. Again, if that happens, it's a
bug.

> 
> > >
> > > This micron->written is stored in the system memory, once power cut,
> > > for the next time Boot up, will it be reinstated or it will be 0x00?  
> > 
> > Yep, and that shouldn't be a problem, it just means we might have unneeded
> > page writes if the pages were already written, but, other than the perf penalty it
> > incurs, it should work fine.
> > 
> > We can optimize that a bit by adding a ->post_read_page() hook so we can flag
> > already read pages as written/erased and avoid those unneeded writes in some
> > situations.  
> 
> That means we assume this block being read before erasing.

Not necessarily. If pages have been read because the MTD user wanted to
access then data (e.g. UBI reads the EC/VID header, UBIFS read its
metadata, ...) it's a good opportunity for us to flag pages as
'written/erased'. But we should not generate extra IOs just for that.

You seem to assume that page reads are almost free compared to erase
operations, and that's true if you only consider the time it takes to
load a page in the NAND cache, but doing IOs is much more expensive
that you think on a shitload of platforms. If we do as you suggest, we
might have 2 rounds of IOs, one to read the pages, and one to write
them if they've not been written already.

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