| Message ID | 20180303104554.5958-1-richard@nod.at |
|---|---|
| State | Accepted |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | ubi: Reject MLC NAND | expand |
On Sat, 3 Mar 2018 11:45:54 +0100 Richard Weinberger <richard@nod.at> wrote: > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > most likely lose all your data upon a power-cut or due to read/write > disturb. > In order to protect users from bad surprises, refuse to attach to MLC > NAND. > > Cc: stable@vger.kernel.org > Signed-off-by: Richard Weinberger <richard@nod.at> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/mtd/ubi/build.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index e941395de3ae..753494e042d5 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -854,6 +854,17 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, > return -EINVAL; > } > > + /* > + * Both UBI and UBIFS have been designed for SLC NAND and NOR flashes. > + * MLC NAND is different and needs special care, otherwise UBI or UBIFS > + * will die soon and you will lose all your data. > + */ > + if (mtd->type == MTD_MLCNANDFLASH) { > + pr_err("ubi: refuse attaching mtd%d - MLC NAND is not supported\n", > + mtd->index); > + return -EINVAL; > + } > + > if (ubi_num == UBI_DEV_NUM_AUTO) { > /* Search for an empty slot in the @ubi_devices array */ > for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++)
On Sat 2018-03-03 11:45:54, Richard Weinberger wrote: > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > most likely lose all your data upon a power-cut or due to read/write > disturb. > In order to protect users from bad surprises, refuse to attach to MLC > NAND. > > Cc: stable@vger.kernel.org That sounds like _really_ bad idea for stable. All it does is it removes support for hardware that somehow works. Now... Yes, handling flash is hard, MLC NAND is worst of the bunch. But... Read disturb is not unique to MLC, right? Happens to all the flashes, its just that MLC has tighter margins for error. Power-cut. UBIFS is just not power-cut safe, right? My notes say that power-cut during erase could result in flash that contains all 1s, but will start showing errors very quickly. Again, not MLC specific. Can be solved with battery... (And yes, there are some problems specific to MLC, where parts of page need to be written in specific order. Not sure how it affects ubifs. But it was not listed as a reason for the patch.) > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/ubi/build.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index e941395de3ae..753494e042d5 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -854,6 +854,17 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, > return -EINVAL; > } > > + /* > + * Both UBI and UBIFS have been designed for SLC NAND and NOR flashes. > + * MLC NAND is different and needs special care, otherwise UBI or UBIFS > + * will die soon and you will lose all your data. > + */ > + if (mtd->type == MTD_MLCNANDFLASH) { > + pr_err("ubi: refuse attaching mtd%d - MLC NAND is not supported\n", > + mtd->index); > + return -EINVAL; > + } > + > if (ubi_num == UBI_DEV_NUM_AUTO) { > /* Search for an empty slot in the @ubi_devices array */ > for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++)
On 03/07/2018 12:18 AM, Pavel Machek wrote: > Now... Yes, handling flash is hard, MLC NAND is worst of the > bunch. But... > MLC is not just more flimsy than SLC NAND. The real problem is that on MLC NAND, pages come in pairs. Multiple voltage levels inside a single, physical memory cell are used to encode more than one bit. Instead of just having pages that are twice as big, the flash exposes them as *two different pages*. Those pages are usually not ordered sequentially either, but according to a vendor/device specific pairing scheme. > Read disturb is not unique to MLC, right? Happens to all the flashes, > its just that MLC has tighter margins for error. > You don't just have more read disturb on a single page. Reading one page will also affect the paired page as well.[0] In addition to that, you also have program disturb. Even *writing successfully* to the upper page of a pair also causes bit flips on the lower one, affecting completely different data.[0] This could be addressed using the experimental ubihealthd that crawls the flash from time to time (at UBI level), making UBI notice and fix bit flips.[0] > Power-cut. UBIFS is just not power-cut safe, right? My notes say that > power-cut during erase could result in flash that contains all 1s, but > will start showing errors very quickly. Again, not MLC specific. Can > be solved with battery... > > (And yes, there are some problems specific to MLC, where parts of page > need to be written in specific order. Not sure how it affects > ubifs. But it was not listed as a reason for the patch.) > Both UBI and UBIFS were designed to be power cut tolerant.[1] On MLC however, if you interrupt a write access to the upper page of a pair, you will also corrupt the lower one, i.e. *data that was already written*, which is a serious issue.[0] Richard and Boris did spend a lot of time working on a solution for that at UBI level which would currently still require a lot of testing and fine tuning, but sadly we ran out of budget. The proposed patch tries to prevent further bad surprises for people who try to use UBI and UBIFS on top of MLC by making sure that UBI _refuses_ to run on MLC (at least for now). Regards, David [0] http://linux-mtd.infradead.org/doc/ubifs.html#L_ubifs_mlc [1] http://linux-mtd.infradead.org/doc/ubifs.html#L_powercut
Pavel, Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek: > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote: > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > > most likely lose all your data upon a power-cut or due to read/write > > disturb. > > In order to protect users from bad surprises, refuse to attach to MLC > > NAND. > > > > Cc: stable@vger.kernel.org > > That sounds like _really_ bad idea for stable. All it does is it > removes support for hardware that somehow works. MLC is not supported and does not work. Full stop. If someone manages to get it somehow work, either with hardware or software hacks they are on their own. Having it in stable is the only chance we have to get it into vendor kernels. All this patch does is representing the current state of UBI/UBIFS. In the last months I got a way too much boards with MLC NAND on my desk where UBIFS broke. Customers wished they had known before... Thanks, //richard
On Wed, 2018-03-07 at 09:01 +0100, Richard Weinberger wrote: > Pavel, > > Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek: > > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote: > > > While UBI and UBIFS seem to work at first sight with MLC NAND, > > > you will > > > most likely lose all your data upon a power-cut or due to > > > read/write > > > disturb. > > > In order to protect users from bad surprises, refuse to attach to > > > MLC > > > NAND. > > > > > > Cc: stable@vger.kernel.org > > > > That sounds like _really_ bad idea for stable. All it does is it > > removes support for hardware that somehow works. > > MLC is not supported and does not work. Full stop. Ack.
On Wed 2018-03-07 09:01:16, Richard Weinberger wrote: > Pavel, > > Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek: > > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote: > > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > > > most likely lose all your data upon a power-cut or due to read/write > > > disturb. > > > In order to protect users from bad surprises, refuse to attach to MLC > > > NAND. > > > > > > Cc: stable@vger.kernel.org > > > > That sounds like _really_ bad idea for stable. All it does is it > > removes support for hardware that somehow works. > > MLC is not supported and does not work. Full stop. > If someone manages to get it somehow work, either with hardware or software > hacks they are on their own. > Having it in stable is the only chance we have to get it into vendor > kernels. Can you show how it meets the stable kernel criteria? They are documented in tree. This should not be in stable. And I'd like to see changelog improved. Real reason MLC is not supported is upper/lower page parts on MLC. And real fix to work with bigger pages in UBI. Pavel
Hi Pavel, On Wed, Mar 7, 2018 at 1:43 PM, Pavel Machek <pavel@ucw.cz> wrote: > On Wed 2018-03-07 09:01:16, Richard Weinberger wrote: >> Pavel, >> >> Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek: >> > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote: >> > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will >> > > most likely lose all your data upon a power-cut or due to read/write >> > > disturb. >> > > In order to protect users from bad surprises, refuse to attach to MLC >> > > NAND. >> > > >> > > Cc: stable@vger.kernel.org >> > >> > That sounds like _really_ bad idea for stable. All it does is it >> > removes support for hardware that somehow works. >> >> MLC is not supported and does not work. Full stop. >> If someone manages to get it somehow work, either with hardware or software >> hacks they are on their own. >> Having it in stable is the only chance we have to get it into vendor >> kernels. > > Can you show how it meets the stable kernel criteria? They are > documented in tree. This should not be in stable. > > And I'd like to see changelog improved. Real reason MLC is not > supported is upper/lower page parts on MLC. And real fix to work with > bigger pages in UBI. > To clarify one thing: the reason for this is MLC has actually never been supported, nor worked properly. The fact that it kinda worked was incidental and the cause of major problems for people due to that not being clear. This patch only makes it explicit and avoids people mistakenly trying to use UBIFS on MLC flash and risking their data and products. To me, that's what's important. This is an important patch, even if all it does is keep people from loosing data. It also changes the conversation from "I have a corrupted UBIFS device, BTW it's on MLC..." to "What can we do to get UBIFS to work on MLC". I don't know what the stable criteria is with re: to this patch. But what I do know is if it doesn't go back into the various stables, there will be manufacturers who will continue to try to use UBIFS on MLC in ignorance for the next several years until the current stable kernels EOL, despite there being a known patch that would make it immediately obvious they shouldn't. Thanks, - Steve
On Wed, 2018-03-07 at 14:08 -0800, Steve deRosier wrote: > > To clarify one thing: the reason for this is MLC has actually never > been supported, nor worked properly. The fact that it kinda worked was > incidental and the cause of major problems for people due to that not > being clear. This patch only makes it explicit and avoids people > mistakenly trying to use UBIFS on MLC flash and risking their data and > products. To me, that's what's important. > > This is an important patch, even if all it does is keep people from > loosing data. It also changes the conversation from "I have a > corrupted UBIFS device, BTW it's on MLC..." to "What can we do to get > UBIFS to work on MLC". This is a bug fix. UBI on MLC never worked. It was a bug that we ever permitted it. This is now fixed. Actually making UBI on MLC *work* would be a feature, and wouldn't be backported to stable. But this one is *absolutely* appropriate for a stable backport.
On Wed 2018-03-07 22:11:13, David Woodhouse wrote: > > > On Wed, 2018-03-07 at 14:08 -0800, Steve deRosier wrote: > > > > To clarify one thing: the reason for this is MLC has actually never > > been supported, nor worked properly. The fact that it kinda worked was > > incidental and the cause of major problems for people due to that not > > being clear. This patch only makes it explicit and avoids people > > mistakenly trying to use UBIFS on MLC flash and risking their data and > > products. To me, that's what's important. > > > > This is an important patch, even if all it does is keep people from > > loosing data. It also changes the conversation from "I have a > > corrupted UBIFS device, BTW it's on MLC..." to "What can we do to get > > UBIFS to work on MLC". Well, for -stable I'd suggest printk(KERN_ALERT ...) but keep the system running. > This is a bug fix. > > UBI on MLC never worked. It was a bug that we ever permitted it. This > is now fixed. Yeah, well, so lets say I have a working hardware (maybe using read-only UBI on MLC), update to next stable kernel, and now kernel refuses to see the partition. I'll certainly not consider this patch a bug fix. Removing support for hardware that "only works by mistake" may be good idea, but maybe it is slightly too surprising for a -stable. Pavel
Hi Pavel, On Wed, 7 Mar 2018 23:17:33 +0100 Pavel Machek <pavel@ucw.cz> wrote: > On Wed 2018-03-07 22:11:13, David Woodhouse wrote: > > > > > > On Wed, 2018-03-07 at 14:08 -0800, Steve deRosier wrote: > > > > > > To clarify one thing: the reason for this is MLC has actually never > > > been supported, nor worked properly. The fact that it kinda worked was > > > incidental and the cause of major problems for people due to that not > > > being clear. This patch only makes it explicit and avoids people > > > mistakenly trying to use UBIFS on MLC flash and risking their data and > > > products. To me, that's what's important. > > > > > > This is an important patch, even if all it does is keep people from > > > loosing data. It also changes the conversation from "I have a > > > corrupted UBIFS device, BTW it's on MLC..." to "What can we do to get > > > UBIFS to work on MLC". > > Well, for -stable I'd suggest printk(KERN_ALERT ...) but keep the > system running. > > > This is a bug fix. > > > > UBI on MLC never worked. It was a bug that we ever permitted it. This > > is now fixed. > > Yeah, well, so lets say I have a working hardware (maybe using > read-only UBI on MLC), update to next stable kernel, and now kernel > refuses to see the partition. Read-only does not save you from the read-disturb issue, and you even have to take care of programming the full erase-block on some MLC NANDs, which AFAIR is not done when updating a static volume. I have one simple question: did you ever play with MLC NANDs or are you just trolling? If you had, like Richard and I did when working on MLC support, I'm pretty sure you wouldn't play this "don't backport to stable" card. Now, if you volunteer to add reliable MLC support, I can send you a few boards to play with. I even have a "working but not so tested PoC" here [1] if you want to finish the job, but please don't do the mistake of thinking the fix is that simple. > > I'll certainly not consider this patch a bug fix. And apparently a lot of people disagree with you on this point, and I guess all of them had problems with MLC NANDs. > > Removing support for hardware that "only works by mistake" may be good > idea, but maybe it is slightly too surprising for a -stable. I wouldn't say "work by mistake" but "seems to work at first but in the end breaks", so definitely a candidate for -stable IMO. Regards, Boris [1]https://github.com/bbrezillon/linux/tree/nand/mlc
On Wed, 7 Mar 2018 22:43:42 +0100 Pavel Machek <pavel@ucw.cz> wrote: > On Wed 2018-03-07 09:01:16, Richard Weinberger wrote: > > Pavel, > > > > Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek: > > > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote: > > > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > > > > most likely lose all your data upon a power-cut or due to read/write > > > > disturb. > > > > In order to protect users from bad surprises, refuse to attach to MLC > > > > NAND. > > > > > > > > Cc: stable@vger.kernel.org > > > > > > That sounds like _really_ bad idea for stable. All it does is it > > > removes support for hardware that somehow works. > > > > MLC is not supported and does not work. Full stop. > > If someone manages to get it somehow work, either with hardware or software > > hacks they are on their own. > > Having it in stable is the only chance we have to get it into vendor > > kernels. > > Can you show how it meets the stable kernel criteria? They are > documented in tree. This should not be in stable. > > And I'd like to see changelog improved. Real reason MLC is not > supported is upper/lower page parts on MLC. And real fix to work with > bigger pages in UBI. Come on! Don't you think this would have been fixed already if it was that easy?! Have you looked at an MLC datasheet to see how paired pages are combined? If you had you would now that paired pages are almost all the time not contiguous, thus preventing the trick you're suggesting here. Please document yourself before doing such presumptuous statements (you can have a look at these slides if you want some details about why this is not so simple [1]). I'm definitely not saying supporting MLC NANDs in Linux is impossible, and if you're interested in working on this topic I'd be happy to help. But please don't block this patch without understanding what supporting MLC NANDs implies. [1]https://events.static.linuxfound.org/sites/events/files/slides/ubi-mlc.pdf
Hi Boris, On Wed, Mar 07, 2018 at 11:30:57PM +0100, Boris Brezillon wrote: > I have one simple question: did you ever play with MLC NANDs or are you > just trolling? If you had, like Richard and I did when working on MLC > support, I'm pretty sure you wouldn't play this "don't backport to > stable" card. Just adding my two cents here, I've always had stability issues with ubifs on NAND and I never knew what type of NAND I've had in these devices (eg: Iomega Iconnect with 256 MB NAND IIRC), so maybe this has never been relevant, maybe it has been, I don't know. However, as a stable maintainer I also know that we want to encourage people to always keep their kernels up to date with latest fixes, and that if there is the slightest risk that a loss of functionality makes people revert and stick to an older, working version, keeping their bugs forever, it's twice as worse, because : - they still run on the bug you wanted to fix - they are now exposed to all bugs fixed later. And we all do this as users, thinking "I'll bisect and report tomorrow" and priorities change, let's be honnest. Thus I think that if you are absolutely certain that it's impossible that people are accidently using this combination, it's probably fine, but if people are using it, you're just displacing the burden on the stable team who will have to explain to each and every user complaining "my system stopped booting after an upgrade to 4.x.y". A big red alert polluting the logs and console every minute, and a pause at boot saying "your NAND, your data and all your kids photos will die soon if you don't switch to another FS" is more productive as users will be less tempted to blindly revert and will at least ask what the problem is and what their solutions are. There's nothing more frustrating than a regression in a stable branch, even for something that was not supposed to work but did by accident. > I wouldn't say "work by mistake" but "seems to work at first but in the > end breaks", so definitely a candidate for -stable IMO. Well, removing support definitely makes the end closer and possibly even prevents the user from recovering their data. I know that data loss is terrible, but data confiscation is similar from the user's point of view. Users don't know the technical details so they will do all things we often consider stupid or impossible, but when warned they know that the risk is on their side and they cannot put the fault at anybody anymore. It tends to work better. Just my two cents, Willy
On Sat, Mar 3, 2018 at 11:45 AM, Richard Weinberger <richard@nod.at> wrote: > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > most likely lose all your data upon a power-cut or due to read/write > disturb. > > In order to protect users from bad surprises, refuse to attach to MLC > NAND. > > Cc: stable@vger.kernel.org > Signed-off-by: Richard Weinberger <richard@nod.at> I'm sorry to disturb in this interesting discussion about what "stable" really means as in "stable kernel". Stable for who and in what sense, that seems to be the question. But my main problem here is to understand who the consumers of the MLC NAND devices really are. I hear some talk here about lab boards. But where is this really deployed, large-scale? And who are the people that will have their devices potentially not booting after this patch? I am pretty sure these people are board support or customization consultants with work being done for some certain products, and not hobbyists and even less end consumers, right? What kind of devices are MLC NANDs being deployed in? Certainly not laptops, tablets and phones, they all use eMMC and even start to venture into UFS (unified flash storage). What is using these flashes? Routers and switches? NAS boxes? Industrial control? Automotive? Or are (God forbid, but would not surprise me) talking about a Linux instance running inside of eMMCs or UFS devices? Yours, Linus Walleij
Linus, Am Donnerstag, 8. März 2018, 14:42:16 CET schrieb Linus Walleij: > On Sat, Mar 3, 2018 at 11:45 AM, Richard Weinberger <richard@nod.at> wrote: > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will > > most likely lose all your data upon a power-cut or due to read/write > > disturb. > > > > In order to protect users from bad surprises, refuse to attach to MLC > > NAND. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Richard Weinberger <richard@nod.at> > > I'm sorry to disturb in this interesting discussion about what > "stable" really means as in "stable kernel". Stable for who and > in what sense, that seems to be the question. > > But my main problem here is to understand who the consumers > of the MLC NAND devices really are. > > I hear some talk here about lab boards. But where is this > really deployed, large-scale? And who are the people that > will have their devices potentially not booting after this patch? > > I am pretty sure these people are board support or > customization consultants with work being done for some > certain products, and not hobbyists and even less end > consumers, right? Correct. I saw vendor specific kernels that have hardware and software hacks to make UBI on MLC NAND somehow work. Somehow in terms of, the filesystem will die just a little later. But these people do _not_ run a vanilla (stable) kernel. We support mainline, nothing more, nothing less. > What kind of devices are MLC NANDs being deployed in? > Certainly not laptops, tablets and phones, they all use eMMC > and even start to venture into UFS (unified flash storage). > > What is using these flashes? Routers and switches? NAS boxes? > Industrial control? Automotive? > > Or are (God forbid, but would not surprise me) talking about a > Linux instance running inside of eMMCs or UFS devices? We need to clarify that even more. Upstream UBI and UBIFS cannot work with MLC NAND correctly. Any such product would die within days... I have many MLC boards on my desk, by running a mainline kernel on them, I can kill every single one within a few minutes, either by plugging the power or triggering read-disturb. As stated by David Woodhouse, it was a huge mistake by UBI to not reject MLC NAND from the very beginning. The sole purpose of this patch is fixing that mistake and make it very clear. Thanks, //richard
On Thu, 2018-03-08 at 15:43 +0100, Richard Weinberger wrote: > As stated by David Woodhouse, it was a huge mistake by UBI to not > reject MLC > NAND from the very beginning. Correction: when we were developing UBI/UBIFS and upstreamed them, MLC was widely used yet we did not really know about it. So there was nothing to reject yet. The mistake is that we did not add the reject timely. When people started reporting MLC issues we were answering that UBI/UBIFS stack needs more work to make MLC safe to use, and we hoped someone would do the work. Artem.
Artem, Am Donnerstag, 8. März 2018, 16:01:15 CET schrieb Artem Bityutskiy: > On Thu, 2018-03-08 at 15:43 +0100, Richard Weinberger wrote: > > As stated by David Woodhouse, it was a huge mistake by UBI to not > > reject MLC > > NAND from the very beginning. > > Correction: when we were developing UBI/UBIFS and upstreamed them, MLC > was widely used yet we did not really know about it. So there was > nothing to reject yet. You mean *not* widely used? > The mistake is that we did not add the reject timely. When people > started reporting MLC issues we were answering that UBI/UBIFS stack > needs more work to make MLC safe to use, and we hoped someone would do > the work. True. TBH Boris and I also thought that adding MLC support is not a that big deal... Thanks, //richard
On Thu, 2018-03-08 at 16:28 +0100, Richard Weinberger wrote:
> You mean *not* widely used?
Yes, indeed, thanks for correction. MLC just wasn't there yet when
UBI/UBIFS were upstreamed.
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index e941395de3ae..753494e042d5 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -854,6 +854,17 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, return -EINVAL; } + /* + * Both UBI and UBIFS have been designed for SLC NAND and NOR flashes. + * MLC NAND is different and needs special care, otherwise UBI or UBIFS + * will die soon and you will lose all your data. + */ + if (mtd->type == MTD_MLCNANDFLASH) { + pr_err("ubi: refuse attaching mtd%d - MLC NAND is not supported\n", + mtd->index); + return -EINVAL; + } + if (ubi_num == UBI_DEV_NUM_AUTO) { /* Search for an empty slot in the @ubi_devices array */ for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++)
While UBI and UBIFS seem to work at first sight with MLC NAND, you will most likely lose all your data upon a power-cut or due to read/write disturb. In order to protect users from bad surprises, refuse to attach to MLC NAND. Cc: stable@vger.kernel.org Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/build.c | 11 +++++++++++ 1 file changed, 11 insertions(+)