On Thu, Aug 4, 2016 at 7:58 AM, Sergey Senozhatsky <
sergey.senozhatsky.work@gmail.com> wrote:

> Hello,
>
> Cc Seth, Dan
>
> On (08/01/16 11:03), Marcin Mirosław wrote:
> > [  429.722411] ------------[ cut here ]------------
> > [  429.723476] WARNING: CPU: 0 PID: 5140 at mm/zswap.c:503
> __zswap_pool_current+0x56/0x60
> > [  429.740048] Call Trace:
> > [  429.740048]  [<ffffffffad255d43>] dump_stack+0x63/0x90
> > [  429.740048]  [<ffffffffad04c997>] __warn+0xc7/0xf0
> > [  429.740048]  [<ffffffffad04cac8>] warn_slowpath_null+0x18/0x20
> > [  429.740048]  [<ffffffffad1250c6>] __zswap_pool_current+0x56/0x60
> > [  429.740048]  [<ffffffffad1250e3>] zswap_pool_current+0x13/0x20
> > [  429.740048]  [<ffffffffad125efb>] __zswap_param_set+0x1db/0x2f0
> > [  429.740048]  [<ffffffffad126042>] zswap_zpool_param_set+0x12/0x20
> > [  429.740048]  [<ffffffffad06645f>] param_attr_store+0x5f/0xc0
> > [  429.740048]  [<ffffffffad065b69>] module_attr_store+0x19/0x30
> > [  429.740048]  [<ffffffffad1b0b02>] sysfs_kf_write+0x32/0x40
> > [  429.740048]  [<ffffffffad1b0663>] kernfs_fop_write+0x113/0x190
> > [  429.740048]  [<ffffffffad13fc52>] __vfs_write+0x32/0x150
> > [  429.740048]  [<ffffffffad15f0ae>] ? __fd_install+0x2e/0xe0
> > [  429.740048]  [<ffffffffad15ef11>] ? __alloc_fd+0x41/0x180
> > [  429.740048]  [<ffffffffad0838dd>] ? percpu_down_read+0xd/0x50
> > [  429.740048]  [<ffffffffad140d33>] vfs_write+0xb3/0x1a0
> > [  429.740048]  [<ffffffffad13db81>] ? filp_close+0x51/0x70
> > [  429.740048]  [<ffffffffad142140>] SyS_write+0x50/0xc0
> > [  429.740048]  [<ffffffffad413836>] entry_SYSCALL_64_fastpath+0x1e/0xa8
> > [  429.764069] ---[ end trace ff7835fbf4d983b9 ]---
>
> I think it's something like this.
>
> suppose there are no pools available - the list is empty (see later).
> __zswap_param_set():
>
>         pool = zswap_pool_find_get(type, compressor);
>
> gives NULL. so it creates a new one
>
>         pool = zswap_pool_create(type, compressor);
>
> then it does
>
>         ret = param_set_charp(s, kp);
>
> which gives 0 -- all ok. so it goes to
>
>         if (!ret) {
>                 put_pool = zswap_pool_current();
>         }
>
> which gives WARN_ON(), as the list is still empty.
>
>
>
> now, how is this possible. for example, we init a zswap with the default
> configuration; but zbud is not available (can it be?). so the pool creation
> fails, but init_zswap() does not set zswap_init_started back to false. it
> either must clear it at the error path, or set it to true right before
> 'return 0'.
>

yep that's exactly right.  I reproduced it with zbud compiled out.



>
> one more problem here is that param_set_charp() does GFP_KERNEL
> under zswap_pools_lock.
>

yep that's true as well.

i can get patches going for both these, unless you're already working on it?




>
>         -ss
>