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

[Dan Streetman]

On Sat, Apr 26, 2014 at 4:37 AM, Weijie Yang <weijie.yang.kh@xxxxxxxxx> 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

thanks for catching that!

>
> The 3/4 patch has a lot of #ifdef, I don't think it's a good kind of
> abstract way.

it has the #ifdef's because there's no point in compiling in code to
use zbud/zsmalloc if zbud/zsmalloc isn't compiled...what alternative
to #ifdef's would you suggest?  Or are there just specific #ifdefs you
suggest to remove?

>
> What about just disable zswap reclaim when using zsmalloc?
> There is a long way to optimize writeback reclaim(both zswap and zram) ,
> Maybe a small and simple step forward is better.

I think it's possible to just remove the zspage from the class while
under lock, then unlock and reclaim it.  As long as there's a
guarantee that zswap (or any zpool/zsmalloc reclaim user) doesn't
map/access the handle after evict() completes successfully, that
should work.  There does need to be some synchronization between
zs_free() and each handle's eviction though, similar to zbud's
under_reclaim flag.  I'll work on a v2 patch.
--
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/