Re: [PATCH v2 01/11] pstore/blk: new support logger for block devices

[WeiXiong Liao]

hi Kees Cook,

On 2020/3/21 äå2:20, Kees Cook wrote:
> On Fri, Mar 20, 2020 at 09:50:36AM +0800, WeiXiong Liao wrote:
>> On 2020/3/19 AM 1:23, Kees Cook wrote:
>>> On Thu, Feb 27, 2020 at 04:21:51PM +0800, liaoweixiong wrote:
>>>> On 2020/2/26 AM 8:52, Kees Cook wrote:
>>>>> On Fri, Feb 07, 2020 at 08:25:45PM +0800, WeiXiong Liao wrote:
>>>>>> +obj-$(CONFIG_PSTORE_BLK) += pstore_blk.o
>>>>>> +pstore_blk-y += blkzone.o
>>>>>
>>>>> Why this dance with files? I would just expect:
>>>>>
>>>>> obj-$(CONFIG_PSTORE_BLK)     += blkzone.o
>>>>>
>>>>
>>>> This makes the built module named blkzone.ko rather than
>>>> pstore_blk.ko.
>>>
>>> You can just do a regular build rule:
>>>
>>> obj-$(CONFIG_PSTORE_BLK) += blkzone.o
>>>
>>
>> I don't get it. If make it as your words, the built module will be
>> blkzone.ko.
>> The module is named pstore/blk, however it built out blkzone.ko. I think
>> it's confusing.
> 
> I mean just pick whatever filename you want it to be named. The Makefile
> case for ramoops was that ramoops got renamed but we wanted to keep the
> old API name.
> 
> So, if you want it named pstore-blk.ko, just rename blkzone.c to
> pstore-blk.c.
> 

How about rename blkzone.c to psotre_zone.c and blkoops.c to pstore_blk.c?

Please refer to my reply email for patch 2.

>>>>> If you're expecting concurrent writers (use of atomic_set(), I would
>>>>> expect the whole write to be locked instead. (i.e. what happens if
>>>>> multiple callers call blkz_zone_write()?)
>>>>>
>>>>
>>>> I don't agree with it. The datalen will be updated everywhere. It's useless
>>>> to lock here.
>>>
>>> But there could be multiple writers; locking should be needed.
>>>
>>
>> All the recorders such as dmesg, pmsg, console and ftrace have been
>> locked on
>> pstore and upper layers. So, a recorder will not write in parallel and
>> different
>> recorders operate privately zone. They don't have any influence on each
>> other.
> 
> Yes, sorry, I was confusing myself about pmsg, and I forgot it had a
> global lock. Each are locked or split by CPU.
> 
>> The only parallel case I think is that recorder writes while dirty-flush
>> thread is
>> working. And the dirty-flusher will flush the whole zone rather than
>> part of it, so,
>> it is OK to call in parallel.
> 
> Okay, thanks for clarifying.
> 
>> Based on these reasons, I don't think locking should be needed.
> 
> Agreed.
> 

-- 
WeiXiong Liao