| Message ID | 20221006224212.569555-5-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some pstore improvements | expand |
On Thu, Oct 06, 2022 at 07:42:08PM -0300, Guilherme G. Piccoli wrote: > The pstore dump function doesn't alert at all on errors - despite > pstore is usually a last resource and if it fails users won't be > able to read the kernel log, this is not the case for server users > with serial access, for example. > > So, let's at least attempt to inform such advanced users on the first > backend writing error detected during the kmsg dump - this is also > very useful for pstore debugging purposes. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > --- > fs/pstore/platform.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > index 06c2c66af332..ee50812fdd2e 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper, > if (ret == 0 && reason == KMSG_DUMP_OOPS) { > pstore_new_entry = 1; > pstore_timer_kick(); > + } else { > + pr_err_once("backend (%s) writing error (%d)\n", > + psinfo->name, ret); We're holding a spinlock here, so doing a pr_*() call isn't a great idea. It's kind of not a great idea to try to write to the log in the middle of a dump either, but we do attempt it at the start. Perhaps keep a saved_ret or something and send it after the spin lock is released?
On 06/10/2022 20:27, Kees Cook wrote: > [...] >> --- a/fs/pstore/platform.c >> +++ b/fs/pstore/platform.c >> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper, >> if (ret == 0 && reason == KMSG_DUMP_OOPS) { >> pstore_new_entry = 1; >> pstore_timer_kick(); >> + } else { >> + pr_err_once("backend (%s) writing error (%d)\n", >> + psinfo->name, ret); > > We're holding a spinlock here, so doing a pr_*() call isn't a great > idea. It's kind of not a great idea to try to write to the log in the > middle of a dump either, but we do attempt it at the start. > > Perhaps keep a saved_ret or something and send it after the spin lock is > released? > Hi Kees, thanks a lot for the very quick review!! Agree with you, I'll rework this one. Do you agree with showing only a single error? For me makes sense since we just wanna hint advanced users (+ people-debugging-pstore heh) that something went wrong. Cheers, Guilherme
On Thu, Oct 06, 2022 at 08:34:44PM -0300, Guilherme G. Piccoli wrote: > On 06/10/2022 20:27, Kees Cook wrote: > > [...] > >> --- a/fs/pstore/platform.c > >> +++ b/fs/pstore/platform.c > >> @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper, > >> if (ret == 0 && reason == KMSG_DUMP_OOPS) { > >> pstore_new_entry = 1; > >> pstore_timer_kick(); > >> + } else { > >> + pr_err_once("backend (%s) writing error (%d)\n", > >> + psinfo->name, ret); > > > > We're holding a spinlock here, so doing a pr_*() call isn't a great > > idea. It's kind of not a great idea to try to write to the log in the > > middle of a dump either, but we do attempt it at the start. > > > > Perhaps keep a saved_ret or something and send it after the spin lock is > > released? > > > > Hi Kees, thanks a lot for the very quick review!! > > Agree with you, I'll rework this one. > Do you agree with showing only a single error? For me makes sense since > we just wanna hint advanced users (+ people-debugging-pstore heh) that > something went wrong. Yeah, I agree -- it's going to be for folks working on pstore code. :)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 06c2c66af332..ee50812fdd2e 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -463,6 +463,9 @@ static void pstore_dump(struct kmsg_dumper *dumper, if (ret == 0 && reason == KMSG_DUMP_OOPS) { pstore_new_entry = 1; pstore_timer_kick(); + } else { + pr_err_once("backend (%s) writing error (%d)\n", + psinfo->name, ret); } total += record.size;
The pstore dump function doesn't alert at all on errors - despite pstore is usually a last resource and if it fails users won't be able to read the kernel log, this is not the case for server users with serial access, for example. So, let's at least attempt to inform such advanced users on the first backend writing error detected during the kmsg dump - this is also very useful for pstore debugging purposes. Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- fs/pstore/platform.c | 3 +++ 1 file changed, 3 insertions(+)