Re: [PATCH] ubifs: wbuf->offs must be aligned to max_write_size

[ぉゅぅ <xoxyuxu@gmail.com>]

Thanks for your reply!

I am sorry for the patch that is hard to understand.
This is my first patch post. I will do my best to convey my intentions.

2019年5月6日(月) 7:08 Richard Weinberger <richard.weinberger@gmail.com>:
>
> On Tue, Apr 30, 2019 at 4:29 PM Yuichi Nakai <xoxyuxu@gmail.com> wrote:
> >
> > UBIFS has a journal recovery function.
> > It is useful for devices that experience a power failure.
> >
> > And as per comment of ubifs_wbuf_sync_nolock(), wbuf is optimized for
> > performance by writing aligned max_write_size.
> >
> > In following environment, checking offset is not aligned to max_write_buffer.
> >
> > - Using a SPI-NOR device with a write buffer size over 256 bytes
> >   For example: Micron MT28EW01GABA, its write buffer is 512 words
> > - LEB hedaer size is 64 bytes
> > - UBI header size is 64 bytes
>
> There are no such things are LEB or UBI header.
> Do you mean UBI EC und UBI VID headers?

I missed writing a correct word..
Yes, UBI EC header and UBI VID header are both 64bytes in our environment.

> What is c->leb_start in your case?

It is 128 Bytes where 64+64.
There is no padding / alignment  because NOR-Flash. (min. IO size is 1.)

Following ASCII art is an erase block, tick(+) is writer buffer boundary.
|------------+------------+------------+------------+------------|
|<->|     ; EC header
|   |<->| ; VID header
|<----->| ; c->leb_start : LEB start offset from an erase block
        |<---+------------+------------+------------+----------->| ;
LEB(c->leb_size)

wbuf takes into account max write size bounds, so references leb_start.
This implementation can be found as follows:
----
FILE: fs/ubifs/io.c
int ubifs_wbuf_init(struct ubifs_info *c, struct ubifs_wbuf *wbuf)
/*
 * If the LEB starts at the max. write size aligned address, then
 * write-buffer size has to be set to @c->max_write_size. Otherwise,
 * set it to something smaller so that it ends at the closest max.
 * write size boundary.
 */
size = c->max_write_size - (c->leb_start % c->max_write_size);
----

Currently, is_last_write() rounds up 'offset' to c->max_write_size.
But the offset is start from c->leb_start.
It is NOT aligned write buffer in our case (shown as above AA).

I think that the implementation of round-up is for write buffer boundary

My understanding is...
The reason the implementation rounds up to a write buffer boundary is that
the effects of NOR Flash writes are considered to fall on a buffer boundary.


> > So if write buffer command make a crrupt data in a block,
> > is_last_write() and no_more_nodes() can not check correctly.
> >
> > This patch adjusts wbuf writes to max_write_size, taking into account
> > leb_start. The recovery process also checks the data at the corrected
> > alignment position.
>
> I have a hard time understanding your patch.
> Are you facing a failure? If so, please share it.

Yes.
But the data can not be sent because it was found in our company.
As I work for traditional Japanese company, the data can not send.
In our case, a journal LEB has a broken data in last empty area.
It should treat as padding data, I think.
And subsequent nodes were not broken.
So we want to recover subsequent nodes, too.
Although it is not the end of the journal data, I do not know why the
data is broken.

The padding data which is 0xFFs in the padding area is checked in
is_last_write().
The current implementation does not consider leb_start, so it starts checking
from an offset different from the true max write buffer boundary.

So the last broken data in a write buffer boundary is not skipped, so
ubifs_recover_leb() detects as a crrupted.


Reproduce scenario:
1. system is booting up
2. UBI attached mtdx
3. UBI can get some volume
4. UBIFS tries to mount the ubi partition
5. Its flag indicate 'need recovery'. So UBIFS executes recovery
sequence by calling
    ubifs_replay_journal() from mount_ubifs() which is called from
mounting with UBIFS.

call trace is as follows:
mount_ubifs()
-> ubifs_replay_journal()
 -> replay_buds()
  -> replay_bud()
   -> ubifs_recover_leb()
    -> ubifs_scan_a_node()
    -> is_last_write()

The above description is for is_last_write(). Others are also same.

For NAND, max_write_size and size of headers are
c->min_io_size(perhaps a sector size).
So this modification is not effected.

> --
> Thanks,
> //richard

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