lkml.org 
[lkml]   [2011]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH -next] pstore: replace spin_lock with spin_trylock_irqsave in panic path
On Tue, Sep 27, 2011 at 01:14:59PM -0400, Seiji Aguchi wrote:
> Hi,
>
> [Problem]
> Currently, pstore takes spin_trylock(&psinfo->buf_lock) in NMI context.
> And it takes spin_lock(&psinfo->buf_lock) in other cases.
>
> If there are some bugs in pstore and kernel panics, spin_lock(&psinfo->buf_lock) causes deadlock
> and panic_notifier_chain will not work.

Ok, so I missed your 'return' first time through and originally had a
bunch of comments. So I would suggest adding a comment explaining why we
are returning in that failure.

Personally, I am not sure we want to abort here at the pstore layer, it
should probably be aborted lower. There isn't any reason why we can't
continue from a pstore perspective (we can just bust the spinlock).

From an ERST perspective, the state machine might be screwed up, hence
aborting in that layer could make sense. But I don't think I agree with
the 'return' statement.

So I am opposed to it for now.

Cheers,
Don


>
> [Patch Description]
> For solving this problem, this patch replaces spin_lock with spin_trylock_irqsave in panic path.
>
> Dead lock in panic path will not happen by applying this patch.
>
> Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
>
> ---
> fs/pstore/platform.c | 17 ++++++++---------
> 1 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 0472924..9882892 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -97,12 +97,15 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> else
> why = "Unknown";
>
> - if (in_nmi()) {
> - is_locked = spin_trylock(&psinfo->buf_lock);
> - if (!is_locked)
> - pr_err("pstore dump routine blocked in NMI, may corrupt error record\n");
> + if (reason == KMSG_DUMP_PANIC) {
> + is_locked = spin_trylock_irqsave(&psinfo->buf_lock, flags);
> + if (!is_locked) {
> + pr_err("pstore dump routine skipped in panic path\n");
> + return;
> + }
> } else
> spin_lock_irqsave(&psinfo->buf_lock, flags);
> +
> oopscount++;
> while (total < kmsg_bytes) {
> dst = psinfo->buf;
> @@ -131,11 +134,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> total += l1_cpy + l2_cpy;
> part++;
> }
> - if (in_nmi()) {
> - if (is_locked)
> - spin_unlock(&psinfo->buf_lock);
> - } else
> - spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> + spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> }
>
> static struct kmsg_dumper pstore_dumper = {
> --
> 1.7.1
>


\
 
 \ /
  Last update: 2011-09-27 19:37    [from the cache]
©2003-2011 Jasper Spaans