Re: [PATCH 3.10] pstore: Make spinlock per zone instead of global

[Leo Yan]

On Fri, Jul 28, 2017 at 06:25:55AM +0200, Willy Tarreau wrote:
> Hi Leo,
> 
> There was no upstream commit ID here but I found it in mainline here :
> 
>   commit 109704492ef637956265ec2eb72ae7b3b39eb6f4
>   Author: Joel Fernandes <joelaf@xxxxxxxxxx>
>   Date:   Thu Oct 20 00:34:00 2016 -0700
> 
>     pstore: Make spinlock per zone instead of global
>     
> What worries me is that some later fixes were issued, apparently to fix
> an oops and a warning after this patch :

Yes, below two patches I also notices. But at least I have not
reproduce them on Android common kernel 4.4. I only faced the hang
issue and the first patch just fixes it.

BTW, I tried to port the second and third patch, but seems the
second patch is dependency on one extra patch; so avoid to introduce
complexity to resolve issue, I just port the first one for fixing
issues.

commit 663deb47880f2283809669563c5a52ac7c6aef1a
Author: Joel Fernandes <joelaf@xxxxxxxxxx>
Date:   Thu Oct 20 00:34:01 2016 -0700

    pstore: Allow prz to control need for locking

    In preparation of not locking at all for certain buffers depending on if
    there's contention, make locking optional depending on the initialization
    of the prz.

>   commit 76d5692a58031696e282384cbd893832bc92bd76
>   Author: Kees Cook <keescook@xxxxxxxxxxxx>
>   Date:   Thu Feb 9 15:43:44 2017 -0800
> 
>     pstore: Correctly initialize spinlock and flags
>     
>     The ram backend wasn't always initializing its spinlock correctly. Since
>     it was coming from kzalloc memory, though, it was harmless on
>     architectures that initialize unlocked spinlocks to 0 (at least x86 and
>     ARM). This also fixes a possibly ignored flag setting too.
> 
> and :
> 
>   commit e9a330c4289f2ba1ca4bf98c2b430ab165a8931b
>   Author: Kees Cook <keescook@xxxxxxxxxxxx>
>   Date:   Sun Mar 5 22:08:58 2017 -0800
> 
>     pstore: Use dynamic spinlock initializer
>     
>     The per-prz spinlock should be using the dynamic initializer so that
>     lockdep can correctly track it. Without this, under lockdep, we get a
>     warning at boot that the lock is in non-static memory.
> 
> So I'm fine with merging this patch as long as Kees is OK with this and
> we know what exact patch series needs to be merged.
> 
> Also, the information you added to the commit message references a trace
> on a 4.4 kernel. Do you confirm that you got the same issue on 3.10 ?

No, I only can confirm this on kernel 4.4. Now only kernel 4.4 are
avaliable on the board, and I verified mainline kernel can work well;
so this is why I can check difference between them and find the first
patch is critical.

> I just prefer to avoid blindly backporting sensitive patches if they're not
> absolutely needed.
> 
> > [   65.103905] hrtimer: interrupt took 2759375 ns
> > [   65.108721] BUG: spinlock recursion on CPU#0, kschedfreq:0/1246
> > [   65.108760]  lock: buffer_lock+0x0/0x38, .magic: dead4ead, .owner: kschedfreq:0/1246, .owner_cpu: 0
> > [   65.108779] CPU: 0 PID: 1246 Comm: kschedfreq:0 Not tainted 4.4.74-07294-g5c996a9-dirty #130
> 
> Thanks!
> willy