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

[Dan Streetman]

On Fri, May 2, 2014 at 4:01 PM, Seth Jennings <sjennings@xxxxxxxxxxxxxx> wrote:
> 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.

fair enough - I'm fairly sure it's doable with only minimal changes to
the current patch, but it's certainly true that there's no reason it
has to be done in the same patchset as the generic layer.

I'll remove it from the v2 patchset.

> 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.

ok, makes sense, certainly when there's currently only 1 user and 2 backends ;-)

>
> 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/