Re: [PATCH v1 11/11] mtd: new support oops logger based on pstore/blk

liaoweixiong
2020-02-06T13:10:57Z

hi Miquel Raynal,

On 2020/1/23 AM 1:41, Miquel Raynal wrote:
> Hello,
>
>
>>>>>> +/*
>>>>>> + * All zones will be read as pstore/blk will read zone one by one when do
>>>>>> + * recover.
>>>>>> + */
>>>>>> +static ssize_t mtdpstore_read(char *buf, size_t size, loff_t off)
>>>>>> +{
>>>>>> + struct mtdpstore_context *cxt = &oops_cxt;
>>>>>> + size_t retlen;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (mtdpstore_block_isbad(cxt, off))
>>>>>> + return -ENEXT;
>>>>>> +
>>>>>> + pr_debug("try to read off 0x%llx size %zu\n", off, size);
>>>>>> + ret = mtd_read(cxt->mtd, off, size, &retlen, (u_char *)buf);
>>>>>> + if ((ret < 0 && !mtd_is_bitflip(ret)) || size != retlen) {
>>>>>
>>>>> IIRC size != retlen does not mean it failed, but that you should
>>>>> continue reading after retlen bytes, no?
>>>>> >>
>>>> Yes, you are right. I will fix it. Thanks.
>>>>
>>>>> Also, mtd_is_bitflip() does not mean that you are reading a false
>>>>> buffer, but that the data has been corrected as it contained bitflips.
>>>>> mtd_is_eccerr() however, would be meaningful.
>>>>> >>
>>>> Sure I know mtd_is_bitflip() does not mean failure, but I do not think
>>>> mtd_is_eccerr() should be here since the codes are ret < 0 and NOT
>>>> mtd_is_bitflip().
>>>
>>> Yes, just drop this check, only keep ret < 0.
>>>
>>
>> If I don't get it wrong, it should not be dropped here. Like your words,
>> "mtd_is_bitflip() does not mean that you are reading a false buffer,
>> but that the data has been corrected as it contained bitflips.", the
>> data I get are valid even if mtd_is_bitflip() return true. It's correct
>> data and it's no need to go to handle error. To me, the codes
>> should be:
>> if (ret < 0 && !mit_is_bitflip())
>> [error handling]
>
> Please check the implementation of mtd_is_bitflip(). You'll probably
> figure out what I am saying.
>
> https://elixir.bootlin.com/linux/latest/source/include/linux/mtd/mtd.h#L585
>

How about the codes as follows:

for (done = 0, retlen = 0; done < size; done += retlen) {
ret = mtd_read(..., &retlen, ...);
if (!ret)
continue;
/*
* do nothing if bitflip and ecc error occurs because whether
* it's bitflip or ECC error, just a small number of bits flip
* and the impact on log data is so small. The mtdpstore just
* hands over what it gets and user can judge whether the data
* is valid or not.
*/
if (mtd_is_bitflip(ret)) {
dev_warn("bitflip at....");
continue;
} else if (mtd_is_eccerr(ret)) {
dev_warn("eccerr at....");
retlen = retlen == 0 ? size : retlen;
continue;
} else {
dev_err("read failure at...");
/* this zone is broken, try next one */
return -ENEXT;
}
}

>
> |...]
>
>>>>>> + return;
>>>>>> + }
>>>>>> + if (unlikely(info->dmesg_size % mtd->writesize)) {
>>>>>> + pr_err("record size %lu KB must align to write size %d KB\n",
>>>>>> + info->dmesg_size / 1024,
>>>>>> + mtd->writesize / 1024);
>>>>>
>>>>> This condition is weird, why would you check this?
>>>>> >>
>>>> pstore/blk will write 'record_size' dmesg log at one time.
>>>> Since each write data must be aligned to 'writesize' for flash, I am not
>>>> sure
>>>> all flash drivers are compatible with misaligned data, that's why i
>>>> check this.
>>>
>>> I think you should enforce this alignment instead of checking it.
>>>
>>
>> Do you mean that mtdpstore should enforce this alignment while running?
>> The way I can think of is to handle a buffer aligned to writesize and
>> write to flash with this aligned buffer.
>>
>> That causes some error. The MTD device will be divided into mutil
>> chunks accroding to dmesg_size. Each chunk stores a individual
>> OOPS/Panic log. If dmesg_size is misaligned to writesize, the last
>> write results in next write failure because the page of flash can only
>> be programed once before next erase and the page shared by two chunks
>> has been used by the last write. Besides, we can not read to buffer,
>> ersae and write back as there is no read/erase for panic case.
>
> I mean: what is the usual size of dmesg? I don't get why you need it to

The usual size of dmesg is 64K, usually be equal to log_buf size.

> be ie. a multiple of 2k. It probably is actually, I don't know if there
> is a standard. But if dmesg_size is eg 3k, just skip the end of the
> partially written page and start writing at the next page?
>

1. upper layer do not support to skip partially written page
The upper layer pstore/blk will not skip the end of the partially
written page since it is not only used for MTD device, but also
block device, which has no page limited. A common practice at the
upper layer is to check the size and limit size to be aligned. We
make dmesg_size to be a multiple of 4K for greater compatibility.

2. chunks management and size per write
The mtdpstore tells pstore/blk how large the device is. Then
pstore/blk will divide it into several chunks according to
dmesg_size. The pstore/blk will write dmesg_size data at a time.

In a word, the amount of data written each time can not lead to page
slicing, so, dmesg_size must be aligned to writesize.

>>
>>>>
>>>>>> + return;
>>>>>> + }
>>>>>> + if (unlikely(mtd->size > MTDPSTORE_MAX_MTD_SIZE)) {
>>>>>> + pr_err("mtd%d is too large (limit is %d MiB)\n",
>>>>>> + mtd->index,
>>>>>> + MTDPSTORE_MAX_MTD_SIZE / 1024 / 1024);
>>>>>
>>>>> Same question? I could understand that it is easier to manage blocks
>>>>> knowing their maximum number though.
>>>>> >>
>>>> It refers to mtdoops.
>>>
>>> What do you mean?
>>>
>>
>> To me, it's unnecessary to check at all, however it is really there
>> on codes of mtdoops. I refer to module mtdoops when I design mtdpstore.
>> It may be helpfull for some cases out of my think, that's why I keep it.
>
> Why not.
>

OK, I will drop it.

> [...]
>
>>>>
>>>> In case of repeated erase when users remove several log files, mtdpstore
>>>> do remove jobs when exit.
>>>>
>>>> Besides, mtdpstore do not check the return code to ensure write back valid
>>>> log as much as possible.
>>>
>>> You are not in a critical path, I don't understand why you don't check
>>> it? If it returns an error, it means the data is not written. IMHO it
>>> is best to alert the user than to silently fail.
>>>
>>
>> This function will be called only when mtd device is removing. It's
>> useless to alert the user but try to flush the other valid data to
>
> It is useful to alert the user! It means something went wrong while
> everything seems fine.
>
>> flash as mush as possible by which reduce losses. If it's just
>> because of busy, what happens next time?
>
> Just because of busy? I don't get it.

I want to express that if the write fails due to busy, the next one
may succeed.

>
> I'm okay with the idea of trying to write the other chunks though:
>
> while (remaining_chunk) {
> ret = mtd_write()
> if (ret) {
> alert-user;
> continue;
> }
> }
>

OK, I will fix it.

>>
>>>>
>>>>>> +. >>>> + off += zonesize;
>>>>>> + size -= min_t(unsigned int, zonesize, size);
>>>>>> + }
>>>>>> +
>>>>>> +free:
>>>>>> + kfree(buf);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>
>>>
>>> [...]
>>>
>>>>>
>>>>> Thanks,
>>>>> Miquèl
>>>>> >>
>>>> I will collect more suggestions and submit the new version at one time.
>>>>
>>>
>>> Sure, no hurry.
>>>
>>
>> I am on holiday, please forgive me for my slow response.
>
> Take your time, as I said, no hurry.
>
>>
>>>
>>> Thanks,
>>> Miquèl
>>>
>
>
>
>
> Thanks,
> Miquèl
>