UBI: recover_peb and power cut safety

[Artem Bityutskiy]

On Thu, 2016-06-16 at 12:03 +0200, Richard Weinberger wrote:
> Forgot to CC Artem.
> 
> On Thu, Jun 16, 2016 at 11:46 AM, Richard Weinberger
> <richard.weinberger at gmail.com> wrote:
> > 
> > Jörg,
> > 
> > On Thu, Jun 16, 2016 at 10:37 AM, Jörg Pfähler <pfaehler at isse.de>
> > wrote:
> > > 
> > > Hi,
> > > 
> > > I would greatly appreciate some clarification with respect to
> > > power cut safety
> > > during writing of an erase block in UBI, specifically power cut
> > > safety of
> > > recover_peb.
> > > 
> > > During a normal write operation (ubi_eba_write_leb in
> > > mtd/ubi/eba.c) UBI tries
> > > to move the contents of the block (and the new contents) to a new
> > > location via
> > > recover_peb, if the write fails. However, recover_peb does not
> > > seem to use the
> > > capability to exchange the (logical) block atomically (as
> > > ubi_eba_atomic_leb_change in mtd/ubi/eba.c does). Specifically,
> > > it does not
> > > seem to write the amount of data and its checksum to the VID
> > > header. Thus, if
> > > the system crashes in the middle of recover_peb before the
> > > old/broken block
> > > could be erased, we are left with a newer version of the block
> > > (the sequence
> > > number in the header is increased by recover_peb), but without
> > > having moved
> > > all the contents of the old block. This would obviously lead to
> > > data loss.
> > > Thus, It seems to me that recover_peb (and therefore
> > > ubi_eba_write_leb) is not
> > > power cut safe or is there some other mechanism distinct from the
> > > one used by
> > > ubi_eba_atomic_leb_change to achieve this? If not I would suggest
> > > using
> > > ubi_eba_atomic_leb_change in ubi_eba_write_leb instead of
> > > recover_peb.
> > Hmm, you are right, if ubi_eba_write() is facing -EIO from the MTD
> > driver we can
> > lose the whole erase block upon power cut.
> > So you found a bug. :-)
> > 
> > Artem, can you tell more on this?
> > I'd guess that recover_peb() is older than
> > ubi_eba_atomic_leb_change() and
> > therefore it was not used.
> > And nobody noticed so far since the condition is hard to hit.
> > 
> > That said, switching to ubi_eba_atomic_leb_change() seems like a
> > good
> > plan to me.
> > Jörg, please send a patch and explain how you tested it.

Yes indeed, very bad bug, good catch.