Re: [PATCH v2 08/11] blkoops: respect for device to pick recorders

[WeiXiong Liao]

hi Kees Cook,

On 2020/3/19 AM 2:42, Kees Cook wrote:
> In the subject and through-out:
> 
> s/recorders/pstore front-ends/
> 

OK.

> On Fri, Feb 07, 2020 at 08:25:52PM +0800, WeiXiong Liao wrote:
>> It's one of a series of patches for adaptive to MTD device.
> 
> typo: adapting
> 

Fixed.

>>
>> MTD device is not block device. The sector of flash (MTD device) will be
>> broken if erase over limited cycles. Avoid damaging block so fast, we
>> can not write to a sector frequently. So, the recorders of pstore/blk
>> like console and ftrace recorder should not be supported.
>>
>> Besides, mtd device need aligned write/erase size. To avoid
>> over-erasing/writing flash, we should keep a aligned cache and read old
>> data to cache before write/erase, which make codes more complex. So,
>> pmsg do not be supported now because it writes misaligned.
>>
>> How about dmesg? Luckly, pstore/blk keeps several aligned chunks for
>> dmesg and uses one by one for wear balance.
>>
>> So, MTD device for pstore should pick recorders, that is why the patch
>> here.
>>
>> Signed-off-by: WeiXiong Liao <liaoweixiong@xxxxxxxxxxxxxxxxx>
>> ---
>>  Documentation/admin-guide/pstore-block.rst |  9 +++++++++
>>  fs/pstore/blkoops.c                        | 29 +++++++++++++++++++++--------
>>  include/linux/blkoops.h                    | 14 +++++++++++++-
>>  3 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/pstore-block.rst b/Documentation/admin-guide/pstore-block.rst
>> index be865dfc1a28..299142b3d8e6 100644
>> --- a/Documentation/admin-guide/pstore-block.rst
>> +++ b/Documentation/admin-guide/pstore-block.rst
>> @@ -166,6 +166,15 @@ It is only required by block device which is registered by
>>  ``blkoops_register_blkdev``.  It's the major device number of registered
>>  devices, by which blkoops can get the matching driver for @blkdev.
>>  
>> +flags
>> +~~~~~
>> +
>> +Refer to macro starting with *BLKOOPS_DEV_SUPPORT_* which is defined in
>> +*linux/blkoops.h*. They tell us that which pstore/blk recorders this device
>> +supports. Default zero means all recorders for compatible, witch is the same
> 
> typo: witch -> which
> 

Fixed.

>> +as BLKOOPS_DEV_SUPPORT_ALL. Recorder works only when chunk size is not zero
>> +and device support.
> 
> There are already flags for this, please see "Supported frontends"
> in include/linux/pstore.h
> 

yes. You are right. I will change it.

>> +
>>  total_size
>>  ~~~~~~~~~~
>>  
>> diff --git a/fs/pstore/blkoops.c b/fs/pstore/blkoops.c
>> index c76bab671b0b..01170b344f00 100644
>> --- a/fs/pstore/blkoops.c
>> +++ b/fs/pstore/blkoops.c
>> @@ -128,9 +128,16 @@ int blkoops_register_device(struct blkoops_device *bo_dev)
>>  		return -ENOMEM;
>>  	}
>>  
>> -#define verify_size(name, defsize, alignsize) {				\
>> -		long _##name_ = (name);					\
>> -		if (_##name_ < 0)					\
>> +	/* zero means all recorders for compatible */
>> +	if (bo_dev->flags == BLKOOPS_DEV_SUPPORT_DEFAULT)
>> +		bo_dev->flags = BLKOOPS_DEV_SUPPORT_ALL;
>> +#define verify_size(name, defsize, alignsize, enable) {			\
>> +		long _##name_;						\
>> +		if (!(enable))						\
>> +			_##name_ = 0;					\
>> +		else if ((name) >= 0)					\
>> +			_##name_ = (name);				\
>> +		else							\
>>  			_##name_ = (defsize);				\
>>  		_##name_ = _##name_ <= 0 ? 0 : (_##name_ * 1024);	\
>>  		if (_##name_ & ((alignsize) - 1)) {			\
>> @@ -142,10 +149,14 @@ int blkoops_register_device(struct blkoops_device *bo_dev)
>>  		bzinfo->name = _##name_;				\
>>  	}
>>  
>> -	verify_size(dmesg_size, DEFAULT_DMESG_SIZE, 4096);
>> -	verify_size(pmsg_size, DEFAULT_PMSG_SIZE, 4096);
>> -	verify_size(console_size, DEFAULT_CONSOLE_SIZE, 4096);
>> -	verify_size(ftrace_size, DEFAULT_FTRACE_SIZE, 4096);
>> +	verify_size(dmesg_size, DEFAULT_DMESG_SIZE, 4096,
>> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_DMESG);
>> +	verify_size(pmsg_size, DEFAULT_PMSG_SIZE, 4096,
>> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_PMSG);
>> +	verify_size(console_size, DEFAULT_CONSOLE_SIZE, 4096,
>> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_CONSOLE);
>> +	verify_size(ftrace_size, DEFAULT_FTRACE_SIZE, 4096,
>> +			bo_dev->flags & BLKOOPS_DEV_SUPPORT_FTRACE);
> 
> I'd kind of prefer this patch be moved much earlier in the series so
> that the later additions of front-end support doesn't have to be touched
> twice. i.e. when PMSG support is added, it is added as a whole here and
> does the flag check in that patch, etc.
> 

OK.

>>  #undef verify_size
>>  	dump_oops = !!(dump_oops < 0 ? DEFAULT_DUMP_OOPS : dump_oops);
>>  
>> @@ -336,6 +347,7 @@ static ssize_t blkoops_blk_panic_write(const char *buf, size_t size,
>>   * register block device to blkoops
>>   * @major: the major device number of registering device
>>   * @panic_write: the write interface for panic case.
>> + * @flags: Refer to macro starting with BLKOOPS_DEV_SUPPORT.
>>   *
>>   * It is ONLY used for block device to register to blkoops. In this case,
>>   * the module parameter @blkdev must be valid. Generic read/write interfaces
>> @@ -349,7 +361,7 @@ static ssize_t blkoops_blk_panic_write(const char *buf, size_t size,
>>   * panic occurs but pstore/blk does not recover yet, the first zone of dmesg
>>   * will be used.
>>   */
>> -int blkoops_register_blkdev(unsigned int major,
>> +int blkoops_register_blkdev(unsigned int major, unsigned int flags,
>>  		blkoops_blk_panic_write_op panic_write)
>>  {
>>  	struct block_device *bdev;
>> @@ -372,6 +384,7 @@ int blkoops_register_blkdev(unsigned int major,
>>  	if (bo_dev.total_size == 0)
>>  		goto err_put_bdev;
>>  	bo_dev.panic_write = panic_write ? blkoops_blk_panic_write : NULL;
>> +	bo_dev.flags = flags;
>>  	bo_dev.read = blkoops_generic_blk_read;
>>  	bo_dev.write = blkoops_generic_blk_write;
>>  
>> diff --git a/include/linux/blkoops.h b/include/linux/blkoops.h
>> index 71c596fd4cc8..bc7665d14a98 100644
>> --- a/include/linux/blkoops.h
>> +++ b/include/linux/blkoops.h
>> @@ -6,6 +6,7 @@
>>  #include <linux/types.h>
>>  #include <linux/blkdev.h>
>>  #include <linux/pstore_blk.h>
>> +#include <linux/bitops.h>
>>  
>>  /**
>>   * struct blkoops_device - backend blkoops driver structure.
>> @@ -14,6 +15,10 @@
>>   * blkoops_register_device(). If block device, you are strongly recommended
>>   * to use blkoops_register_blkdev().
>>   *
>> + * @flags:
>> + *	Refer to macro starting with BLKOOPS_DEV_SUPPORT_. These macros tell
>> + *	us that which pstore/blk recorders this device supports. Zero means
>> + *	all recorders for compatible.
>>   * @total_size:
>>   *	The total size in bytes pstore/blk can use. It must be greater than
>>   *	4096 and be multiple of 4096.
>> @@ -38,6 +43,13 @@
>>   *	On error, negative number should be returned.
>>   */
>>  struct blkoops_device {
>> +	unsigned int flags;
>> +#define BLKOOPS_DEV_SUPPORT_ALL		UINT_MAX
>> +#define BLKOOPS_DEV_SUPPORT_DEFAULT	(0)
>> +#define BLKOOPS_DEV_SUPPORT_DMESG	BIT(0)
>> +#define BLKOOPS_DEV_SUPPORT_PMSG	BIT(1)
>> +#define BLKOOPS_DEV_SUPPORT_CONSOLE	BIT(2)
>> +#define BLKOOPS_DEV_SUPPORT_FTRACE	BIT(3)
>>  	unsigned long total_size;
>>  	blkz_read_op read;
>>  	blkz_write_op write;
>> @@ -54,7 +66,7 @@ typedef int (*blkoops_blk_panic_write_op)(const char *buf, sector_t start_sect,
>>  
>>  int  blkoops_register_device(struct blkoops_device *bo_dev);
>>  void blkoops_unregister_device(struct blkoops_device *bo_dev);
>> -int  blkoops_register_blkdev(unsigned int major,
>> +int  blkoops_register_blkdev(unsigned int major, unsigned int flags,
>>  		blkoops_blk_panic_write_op panic_write);
>>  void blkoops_unregister_blkdev(unsigned int major);
>>  int  blkoops_blkdev_info(dev_t *devt, sector_t *nr_sects, sector_t *start_sect);
>> -- 
>> 1.9.1
>>
> 

-- 
WeiXiong Liao