Messages in this thread Patch in this message | | | Date | Thu, 5 Jul 2012 09:32:01 -0400 | From | Don Zickus <> | Subject | Re: [RFC][PATCH 2/2] write callback: Check if existing entry is erasable |
| |
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 > >
| |