Re: [RFC][PATCH 2/2] write callback: Check if existing entry is erasable

[Don Zickus <dzickus@redhat.com>]

On Tue, Jul 03, 2012 at 11:35:47PM +0000, Seiji Aguchi wrote:
> This patch introduces a following rule checking if an existing entry in NVRAM are erasable to avoid missing 
> a critical message ,such as panic, before an user check it.

I am not a big fan of this policy.

> 
>     - In case where an existing entry is panic or emergency
>       -  It is not erasable because if panic/emergency event is lost, we have no way to detect 
>          the root cause. We shouldn't overwrite them for any reason.

Ok.

> 
>     - In case where an existing entry is oops/shutdown/halt/poweroff
>       -  It is erasable if an error ,panic, emergency or oops, happen in new event.
>            - Oops is erasable because system may still be running and its message may be saved 
>              into /var/log/messages.

Then why save it to begin with?

>            - Also, normal event ,shutdown/halt/poweroff, is erasable because error message is 
>              more important than normal message.

Then why happens if you are using these messages to debug a problem on
your system?  It would seem one could be misled unless you set a flag
stating that a log was overwritten somewhere.

> 
>     - In case where an existing entry is unknown event
>       -  It is erasable because any event supported by pstore outweighs unsupported events.

Again, how does an 'unknown' event get recorded?  Shouldn't all events be
known?

I would rather see no records overwritten and just make sure there is
enough space for a dozen or so records to buffer multiple panics before
userspace can run.

Implementing policy like this in the kernel seems like it would be a
constant battle between everyone's view point of what is important and not
important.

I would rather take the viewpoint, if it is important to log it in a space
limited NVRAM, then it is important enough not to overwrite until
userspace explicitly asks it to be deleted.  Otherwise why log it, if it
is not important?

Cheers,
Don

> 
> Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
> ---
>  drivers/firmware/efivars.c |   48 +++++++++++++++++++++++++++++++++++++++++++-
>  fs/pstore/platform.c       |    4 +-
>  include/linux/pstore.h     |    5 ++++
>  3 files changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 4929254..54d9bc6 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -685,6 +685,45 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
>  	return 0;
>  }
>  
> +
> +static bool can_erase_entry(struct efivar_entry *entry, enum kmsg_dump_reason
> +			   new_reason)
> +{
> +	int prev_reason = 0;
> +	const char *prev_why;
> +	bool is_erasable = 0;
> +
> +	/* Get a reason of previous message */
> +	while (1) {
> +		prev_why =  pstore_get_reason_str(prev_reason);
> +		if (!strncmp(entry->var.Data, prev_why, strlen(prev_why)))
> +			break;
> +		prev_reason++;
> +	}
> +
> +	/* check if exisiting message is erasable */
> +
> +	switch (prev_reason) {
> +	case KMSG_DUMP_PANIC:
> +	case KMSG_DUMP_EMERG:
> +		/* Never erase panic or emergency message */
> +		break;
> +	case KMSG_DUMP_OOPS:
> +	case KMSG_DUMP_RESTART:
> +	case KMSG_DUMP_HALT:
> +	case KMSG_DUMP_POWEROFF:
> +		/* Can erase if new one is error message */
> +		if (new_reason <= KMSG_DUMP_EMERG)
> +			is_erasable = 1;
> +		break;
> +	default:
> +		/* Can erase unknown message */
> +		is_erasable = 1;
> +	}
> +
> +	return is_erasable;
> +}
> +
>  static int efi_pstore_write(enum pstore_type_id type,
>  		enum kmsg_dump_reason reason, u64 *id,
>  		unsigned int part, size_t size, struct pstore_info *psi) @@ -706,7 +745,7 @@ static int efi_pstore_write(enum pstore_type_id type,
>  		efi_name[i] = stub_name[i];
>  
>  	/*
> -	 * Clean up any entries with the same name
> +	 * Search for erasable entry
>  	 */
>  
>  	list_for_each_entry(entry, &efivars->list, list) { @@ -721,6 +760,13 @@ static int efi_pstore_write(enum pstore_type_id type,
>  		if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
>  			continue;
>  
> +		if (!can_erase_entry(entry, reason)) {
> +			/* return without writing new entry */
> +			spin_unlock(&efivars->lock);
> +			*id = part;
> +			return ret;
> +		}
> +
>  		/* found */
>  		found = entry;
>  		efivars->ops->set_variable(entry->var.VariableName,
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 03ce7a9..32715eb 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -68,7 +68,7 @@ void pstore_set_kmsg_bytes(int bytes)
>  /* Tag each group of saved records with a sequence number */
>  static int	oopscount;
>  
> -static const char *get_reason_str(enum kmsg_dump_reason reason)
> +const char *pstore_get_reason_str(enum kmsg_dump_reason reason)
>  {
>  	switch (reason) {
>  	case KMSG_DUMP_PANIC:
> @@ -104,7 +104,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>  	int		is_locked = 0;
>  	int		ret;
>  
> -	why = get_reason_str(reason);
> +	why = pstore_get_reason_str(reason);
>  
>  	if (in_nmi()) {
>  		is_locked = spin_trylock(&psinfo->buf_lock); diff --git a/include/linux/pstore.h b/include/linux/pstore.h index e1461e1..e9347e9 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -54,12 +54,17 @@ struct pstore_info {
>  
>  #ifdef CONFIG_PSTORE
>  extern int pstore_register(struct pstore_info *);
> +extern const char *pstore_get_reason_str(enum kmsg_dump_reason reason);
>  #else
>  static inline int
>  pstore_register(struct pstore_info *psi)  {
>  	return -ENODEV;
>  }
> +static const char *pstore_get_reason_str(enum kmsg_dump_reason reason) 
> +{
> +	return NULL;
> +}
>  #endif
>  
>  #endif /*_LINUX_PSTORE_H*/
> --
> 1.7.1
> 
>