Re: [PATCH 2/4] mm: zpool: implement zsmalloc shrinking

[Seth Jennings]

On Sat, Apr 26, 2014 at 04:37:31PM +0800, Weijie Yang wrote:
> On Sat, Apr 19, 2014 at 11:52 PM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
> > Add zs_shrink() and helper functions to zsmalloc.  Update zsmalloc
> > zs_create_pool() creation function to include ops param that provides
> > an evict() function for use during shrinking.  Update helper function
> > fix_fullness_group() to always reinsert changed zspages even if the
> > fullness group did not change, so they are updated in the fullness
> > group lru.  Also update zram to use the new zsmalloc pool creation
> > function but pass NULL as the ops param, since zram does not use
> > pool shrinking.
> >
> 
> I only review the code without test, however, I think this patch is
> not acceptable.
> 
> The biggest problem is it will call zswap_writeback_entry() under lock,
> zswap_writeback_entry() may sleep, so it is a bug. see below
> 
> The 3/4 patch has a lot of #ifdef, I don't think it's a good kind of
> abstract way.
> 
> What about just disable zswap reclaim when using zsmalloc?

I agree here.  Making a generic allocator layer and zsmalloc reclaim
support should be two different efforts, since zsmalloc reclaim is
fraught with peril.

The generic layer can be done though, as long as you provide a way for
the backend to indicate that it doesn't support reclaim, which just
results in lru-inverse overflow to the swap device at the zswap layer.
Hopefully, if the user overrides the default to use zsmalloc, they
understand the implications and have sized their workload properly.

Also, the fallback logic shouldn't be in this generic layer.  It should
not be transparent to the user.  The user (in this case zswap) should
implement the fallback if they care to have it.  The generic allocator
layer makes it trivial for the user to implement.

Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/