Re: [PATCH 2/3] zram: support frontswap

[Minchan Kim <minchan@kernel.org>]

On Tue, Jul 11, 2023 at 10:08:46AM +0000, Alexey Romanov wrote:
> Hello,
> 
> On Mon, Jul 10, 2023 at 03:16:58PM -0700, Minchan Kim wrote:
> > With frontswap, zram can perform swapout 13% faster, which restores
> > its original performance with rw_page.
> > 
> > 200M swapout using MADV_PAGEOUT:
> > Before: 4355ms After: 3786ms
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  drivers/block/zram/Kconfig    |  1 +
> >  drivers/block/zram/zram_drv.c | 98 +++++++++++++++++++++++++++++++++++
> >  drivers/block/zram/zram_drv.h |  1 +
> >  3 files changed, 100 insertions(+)
> > 
> > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > index 0386b7da02aa..a841d6d14c74 100644
> > --- a/drivers/block/zram/Kconfig
> > +++ b/drivers/block/zram/Kconfig
> > @@ -4,6 +4,7 @@ config ZRAM
> >  	depends on BLOCK && SYSFS && MMU
> >  	depends on CRYPTO_LZO || CRYPTO_ZSTD || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842
> >  	select ZSMALLOC
> > +	select FRONTSWAP
> >  	help
> >  	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
> >  	  Pages written to these disks are compressed and stored in memory
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 5d258a28ec43..5e973c982235 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -33,6 +33,8 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/cpuhotplug.h>
> >  #include <linux/part_stat.h>
> > +#include <linux/swap.h>
> > +#include <linux/frontswap.h>
> >  
> >  #include "zram_drv.h"
> >  
> > @@ -2170,6 +2172,90 @@ static struct attribute *zram_disk_attrs[] = {
> >  
> >  ATTRIBUTE_GROUPS(zram_disk);
> >  
> > +static struct zram *zram_swaps[MAX_SWAPFILES];
> > +static LIST_HEAD(zram_list);
> > +static DEFINE_SPINLOCK(zram_list_lock);
> > +
> > +static int zram_frontswap_store(unsigned int type, pgoff_t index,
> > +				struct page *page)
> > +{
> > +	int err;
> > +	struct zram *zram = zram_swaps[type];
> > +
> > +	err = zram_write_page(zram, page, index);
> > +	if (!err) {
> > +		zram_slot_lock(zram, index);
> > +		zram_accessed(zram, index);
> > +		zram_slot_unlock(zram, index);
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static int zram_frontswap_load(unsigned int type, pgoff_t index,
> > +			       struct page *page, bool *exclusive)
> > +{
> > +	int err;
> > +	struct zram *zram = zram_swaps[type];
> > +
> > +	zram_slot_lock(zram, index);
> > +	if (zram_test_flag(zram, index, ZRAM_WB)) {
> > +		zram_slot_lock(zram, index);
> 
> Looks like zram_slot_unlock should be here instead of zram_slot_lock.

me/ slabs self.

> 
> > +		return -1;
> > +	}
> > +
> > +	err = zram_read_from_zspool(zram, page, index);
> > +	if (!err)
> > +		zram_accessed(zram, index);
> > +	zram_slot_unlock(zram, index);
> > +
> > +	return err;
> > +}
> > +
> > +static void zram_frontswap_invalidate_area(unsigned int type)
> > +{
> > +	struct zram *zram = zram_swaps[type];
> > +
> > +	if (!zram)
> > +		return;
> > +}
> 
> It makes sense? Maybe we just leave this function empty?

Sorry, it was wrong version from my git tree.
I had destroy zram in the case originally
but since I decide to drop this patchset, never mind.

Thanks for spending time for review, Alexey.