[PATCH 0/3] zram: use frontswap for zram swap usecase

[PATCH 0/3] zram: use frontswap for zram swap usecase

[Minchan Kim]

This patchset uses frontswap for zram swap usecase and remove
swap_slot_free_notify swap specific operation in block device.
It shows 13% swapout improvement for MADV_PAGEOUT.

Minchan Kim (3):
  frontswap: support backing_dev
  zram: support frontswap
  zram: remove swap_slot_free_notify

 Documentation/filesystems/locking.rst |   5 --
 drivers/block/zram/Kconfig            |   1 +
 drivers/block/zram/zram_drv.c         | 116 ++++++++++++++++++++++----
 drivers/block/zram/zram_drv.h         |   1 +
 include/linux/blkdev.h                |   2 -
 include/linux/frontswap.h             |   7 +-
 mm/frontswap.c                        |   4 +-
 mm/swapfile.c                         |  11 +--
 mm/zswap.c                            |   2 +-
 9 files changed, 110 insertions(+), 39 deletions(-)

-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 1/3] frontswap: support backing_dev

[Minchan Kim]

Modify frontswap to be used as a block device driver for swap
partitions.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/frontswap.h | 7 +++++--
 mm/frontswap.c            | 4 ++--
 mm/swapfile.c             | 3 ++-
 mm/zswap.c                | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/linux/frontswap.h b/include/linux/frontswap.h
index eaa0ac5f9003..466d617dc3c7 100644
--- a/include/linux/frontswap.h
+++ b/include/linux/frontswap.h
@@ -7,8 +7,10 @@
 #include <linux/bitops.h>
 #include <linux/jump_label.h>
 
+struct block_device;
+
 struct frontswap_ops {
-	void (*init)(unsigned); /* this swap type was just swapon'ed */
+	void (*init)(unsigned, struct block_device *); /* this swap type was just swapon'ed */
 	int (*store)(unsigned, pgoff_t, struct page *); /* store a page */
 	int (*load)(unsigned, pgoff_t, struct page *, bool *); /* load a page */
 	void (*invalidate_page)(unsigned, pgoff_t); /* page no longer needed */
@@ -17,7 +19,8 @@ struct frontswap_ops {
 
 int frontswap_register_ops(const struct frontswap_ops *ops);
 
-extern void frontswap_init(unsigned type, unsigned long *map);
+extern void frontswap_init(unsigned type, unsigned long *map,
+			   struct block_device *bdev);
 extern int __frontswap_store(struct page *page);
 extern int __frontswap_load(struct page *page);
 extern void __frontswap_invalidate_page(unsigned, pgoff_t);
diff --git a/mm/frontswap.c b/mm/frontswap.c
index 2fb5df3384b8..967ce610a1de 100644
--- a/mm/frontswap.c
+++ b/mm/frontswap.c
@@ -107,7 +107,7 @@ int frontswap_register_ops(const struct frontswap_ops *ops)
 /*
  * Called when a swap device is swapon'd.
  */
-void frontswap_init(unsigned type, unsigned long *map)
+void frontswap_init(unsigned type, unsigned long *map, struct block_device *bdev)
 {
 	struct swap_info_struct *sis = swap_info[type];
 
@@ -128,7 +128,7 @@ void frontswap_init(unsigned type, unsigned long *map)
 
 	if (!frontswap_enabled())
 		return;
-	frontswap_ops->init(type);
+	frontswap_ops->init(type, bdev);
 }
 
 static bool __frontswap_test(struct swap_info_struct *sis,
diff --git a/mm/swapfile.c b/mm/swapfile.c
index a6945c2e0d03..a9424fd226bc 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2339,7 +2339,8 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
 				unsigned long *frontswap_map)
 {
 	if (IS_ENABLED(CONFIG_FRONTSWAP))
-		frontswap_init(p->type, frontswap_map);
+		frontswap_init(p->type, frontswap_map, p->bdev);
+
 	spin_lock(&swap_lock);
 	spin_lock(&p->lock);
 	setup_swap_info(p, prio, swap_map, cluster_info);
diff --git a/mm/zswap.c b/mm/zswap.c
index c122f042a49d..9247e3adc21f 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1510,7 +1510,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
 	zswap_trees[type] = NULL;
 }
 
-static void zswap_frontswap_init(unsigned type)
+static void zswap_frontswap_init(unsigned type, struct block_device *bdev)
 {
 	struct zswap_tree *tree;
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 2/3] zram: support frontswap

[Minchan Kim]

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);
+		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;
+}
+
+static void zram_frontswap_init(unsigned int type, struct block_device *bdev)
+{
+	struct zram *zram;
+
+	spin_lock(&zram_list_lock);
+	list_for_each_entry(zram, &zram_list, list) {
+		if (&zram_devops == bdev->bd_disk->fops) {
+			zram_swaps[type] = zram;
+			break;
+		}
+	}
+	spin_unlock(&zram_list_lock);
+}
+
+static void zram_frontswap_invalidate_page(unsigned int type, pgoff_t offset)
+{
+	struct zram *zram = zram_swaps[type];
+
+	if (!zram_slot_trylock(zram, offset)) {
+		atomic64_inc(&zram->stats.miss_free);
+		return;
+	}
+
+	zram_free_page(zram, offset);
+	zram_slot_unlock(zram, offset);
+}
+
+static const struct frontswap_ops zram_frontswap_ops = {
+	.store = zram_frontswap_store,
+	.load = zram_frontswap_load,
+	.init = zram_frontswap_init,
+	.invalidate_area = zram_frontswap_invalidate_area,
+	.invalidate_page = zram_frontswap_invalidate_page,
+	.init = zram_frontswap_init
+};
+
 /*
  * Allocate and initialize new zram device. the function returns
  * '>= 0' device_id upon success, and negative value otherwise.
@@ -2246,6 +2332,10 @@ static int zram_add(void)
 
 	comp_algorithm_set(zram, ZRAM_PRIMARY_COMP, default_compressor);
 
+	spin_lock(&zram_list_lock);
+	list_add(&zram->list, &zram_list);
+	spin_unlock(&zram_list_lock);
+
 	zram_debugfs_register(zram);
 	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return device_id;
@@ -2303,6 +2393,11 @@ static int zram_remove(struct zram *zram)
 	zram_reset_device(zram);
 
 	put_disk(zram->disk);
+
+	spin_lock(&zram_list_lock);
+	list_del(&zram->list);
+	spin_unlock(&zram_list_lock);
+
 	kfree(zram);
 	return 0;
 }
@@ -2428,6 +2523,9 @@ static int __init zram_init(void)
 		num_devices--;
 	}
 
+	if (frontswap_register_ops(&zram_frontswap_ops))
+		pr_info("Frontswap is not used, which is suboptimal for zram swap.\n");
+
 	return 0;
 
 out_error:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index ca7a15bd4845..0f52d2da8512 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -139,5 +139,6 @@ struct zram {
 #ifdef CONFIG_ZRAM_MEMORY_TRACKING
 	struct dentry *debugfs_dir;
 #endif
+	struct list_head list;
 };
 #endif
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 3/3] zram: remove swap_slot_free_notify

[Minchan Kim]

Frontswap replaces the need for swap_slot_free_notify in block_device.
Therefore, we can remove it.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/filesystems/locking.rst |  5 -----
 drivers/block/zram/zram_drv.c         | 20 +-------------------
 include/linux/blkdev.h                |  2 --
 mm/swapfile.c                         |  8 --------
 4 files changed, 1 insertion(+), 34 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index aa1a233b0fa8..c94ef7d9fc6e 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -477,7 +477,6 @@ block_device_operations
 				unsigned long *);
 	void (*unlock_native_capacity) (struct gendisk *);
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
-	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
 
 locking rules:
 
@@ -491,12 +490,8 @@ compat_ioctl:		no
 direct_access:		no
 unlock_native_capacity:	no
 getgeo:			no
-swap_slot_free_notify:	no	(see below)
 ======================= ===================
 
-swap_slot_free_notify is called with swap_lock and sometimes the page lock
-held.
-
 
 file_operations
 ===============
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5e973c982235..ec040ab3ab91 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1947,23 +1947,6 @@ static void zram_submit_bio(struct bio *bio)
 	}
 }
 
-static void zram_slot_free_notify(struct block_device *bdev,
-				unsigned long index)
-{
-	struct zram *zram;
-
-	zram = bdev->bd_disk->private_data;
-
-	atomic64_inc(&zram->stats.notify_free);
-	if (!zram_slot_trylock(zram, index)) {
-		atomic64_inc(&zram->stats.miss_free);
-		return;
-	}
-
-	zram_free_page(zram, index);
-	zram_slot_unlock(zram, index);
-}
-
 static void zram_destroy_comps(struct zram *zram)
 {
 	u32 prio;
@@ -2117,7 +2100,6 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 static const struct block_device_operations zram_devops = {
 	.open = zram_open,
 	.submit_bio = zram_submit_bio,
-	.swap_slot_free_notify = zram_slot_free_notify,
 	.owner = THIS_MODULE
 };
 
@@ -2206,7 +2188,7 @@ static int zram_frontswap_load(unsigned int type, pgoff_t index,
 
 	err = zram_read_from_zspool(zram, page, index);
 	if (!err)
-		zram_accessed(zram, index);
+		zram_free_page(zram, index);
 	zram_slot_unlock(zram, index);
 
 	return err;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c0ffe203a602..49e1843e44ea 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1390,8 +1390,6 @@ struct block_device_operations {
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
 	int (*set_read_only)(struct block_device *bdev, bool ro);
 	void (*free_disk)(struct gendisk *disk);
-	/* this callback is with swap_lock and sometimes page table lock held */
-	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
 	int (*report_zones)(struct gendisk *, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data);
 	char *(*devnode)(struct gendisk *disk, umode_t *mode);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index a9424fd226bc..adc16d8883c8 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -726,7 +726,6 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
 {
 	unsigned long begin = offset;
 	unsigned long end = offset + nr_entries - 1;
-	void (*swap_slot_free_notify)(struct block_device *, unsigned long);
 
 	if (offset < si->lowest_bit)
 		si->lowest_bit = offset;
@@ -739,16 +738,9 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
 	}
 	atomic_long_add(nr_entries, &nr_swap_pages);
 	WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries);
-	if (si->flags & SWP_BLKDEV)
-		swap_slot_free_notify =
-			si->bdev->bd_disk->fops->swap_slot_free_notify;
-	else
-		swap_slot_free_notify = NULL;
 	while (offset <= end) {
 		arch_swap_invalidate_page(si->type, offset);
 		frontswap_invalidate_page(si->type, offset);
-		if (swap_slot_free_notify)
-			swap_slot_free_notify(si->bdev, offset);
 		offset++;
 	}
 	clear_shadow_from_swap_cache(si->type, begin, end);
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH 0/3] zram: use frontswap for zram swap usecase

[Christoph Hellwig]

On Mon, Jul 10, 2023 at 03:16:56PM -0700, Minchan Kim wrote:
> This patchset uses frontswap for zram swap usecase and remove
> swap_slot_free_notify swap specific operation in block device.
> It shows 13% swapout improvement for MADV_PAGEOUT.

Err, no.  frontswap needs to go away, and not be tried to a block
driver.  If you want compressed swap without a block driver please use
zswap and help improvig it and the swap abstraction to not require
a backing allocation for it.

We need to fix swap and not pile hacks on top of hacks.

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

[Alexey Romanov]

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.

> +		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?

> +
> +static void zram_frontswap_init(unsigned int type, struct block_device *bdev)
> +{
> +	struct zram *zram;
> +
> +	spin_lock(&zram_list_lock);
> +	list_for_each_entry(zram, &zram_list, list) {
> +		if (&zram_devops == bdev->bd_disk->fops) {
> +			zram_swaps[type] = zram;
> +			break;
> +		}
> +	}
> +	spin_unlock(&zram_list_lock);
> +}
> +
> +static void zram_frontswap_invalidate_page(unsigned int type, pgoff_t offset)
> +{
> +	struct zram *zram = zram_swaps[type];
> +
> +	if (!zram_slot_trylock(zram, offset)) {
> +		atomic64_inc(&zram->stats.miss_free);
> +		return;
> +	}
> +
> +	zram_free_page(zram, offset);
> +	zram_slot_unlock(zram, offset);
> +}
> +
> +static const struct frontswap_ops zram_frontswap_ops = {
> +	.store = zram_frontswap_store,
> +	.load = zram_frontswap_load,
> +	.init = zram_frontswap_init,
> +	.invalidate_area = zram_frontswap_invalidate_area,
> +	.invalidate_page = zram_frontswap_invalidate_page,
> +	.init = zram_frontswap_init
> +};
> +
>  /*
>   * Allocate and initialize new zram device. the function returns
>   * '>= 0' device_id upon success, and negative value otherwise.
> @@ -2246,6 +2332,10 @@ static int zram_add(void)
>  
>  	comp_algorithm_set(zram, ZRAM_PRIMARY_COMP, default_compressor);
>  
> +	spin_lock(&zram_list_lock);
> +	list_add(&zram->list, &zram_list);
> +	spin_unlock(&zram_list_lock);
> +
>  	zram_debugfs_register(zram);
>  	pr_info("Added device: %s\n", zram->disk->disk_name);
>  	return device_id;
> @@ -2303,6 +2393,11 @@ static int zram_remove(struct zram *zram)
>  	zram_reset_device(zram);
>  
>  	put_disk(zram->disk);
> +
> +	spin_lock(&zram_list_lock);
> +	list_del(&zram->list);
> +	spin_unlock(&zram_list_lock);
> +
>  	kfree(zram);
>  	return 0;
>  }
> @@ -2428,6 +2523,9 @@ static int __init zram_init(void)
>  		num_devices--;
>  	}
>  
> +	if (frontswap_register_ops(&zram_frontswap_ops))
> +		pr_info("Frontswap is not used, which is suboptimal for zram swap.\n");
> +
>  	return 0;
>  
>  out_error:
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index ca7a15bd4845..0f52d2da8512 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -139,5 +139,6 @@ struct zram {
>  #ifdef CONFIG_ZRAM_MEMORY_TRACKING
>  	struct dentry *debugfs_dir;
>  #endif
> +	struct list_head list;
>  };
>  #endif
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 
> 

-- 
Thank you,
Alexey

Re: [PATCH 3/3] zram: remove swap_slot_free_notify

[Alexey Romanov]

Hello,

On Mon, Jul 10, 2023 at 03:16:59PM -0700, Minchan Kim wrote:
> Frontswap replaces the need for swap_slot_free_notify in block_device.
> Therefore, we can remove it.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  Documentation/filesystems/locking.rst |  5 -----
>  drivers/block/zram/zram_drv.c         | 20 +-------------------
>  include/linux/blkdev.h                |  2 --
>  mm/swapfile.c                         |  8 --------
>  4 files changed, 1 insertion(+), 34 deletions(-)
> 
> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index aa1a233b0fa8..c94ef7d9fc6e 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -477,7 +477,6 @@ block_device_operations
>  				unsigned long *);
>  	void (*unlock_native_capacity) (struct gendisk *);
>  	int (*getgeo)(struct block_device *, struct hd_geometry *);
> -	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
>  
>  locking rules:
>  
> @@ -491,12 +490,8 @@ compat_ioctl:		no
>  direct_access:		no
>  unlock_native_capacity:	no
>  getgeo:			no
> -swap_slot_free_notify:	no	(see below)
>  ======================= ===================
>  
> -swap_slot_free_notify is called with swap_lock and sometimes the page lock
> -held.
> -
>  
>  file_operations
>  ===============
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 5e973c982235..ec040ab3ab91 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1947,23 +1947,6 @@ static void zram_submit_bio(struct bio *bio)
>  	}
>  }
>  
> -static void zram_slot_free_notify(struct block_device *bdev,
> -				unsigned long index)
> -{
> -	struct zram *zram;
> -
> -	zram = bdev->bd_disk->private_data;
> -
> -	atomic64_inc(&zram->stats.notify_free);
> -	if (!zram_slot_trylock(zram, index)) {
> -		atomic64_inc(&zram->stats.miss_free);
> -		return;
> -	}
> -
> -	zram_free_page(zram, index);
> -	zram_slot_unlock(zram, index);
> -}
> -
>  static void zram_destroy_comps(struct zram *zram)
>  {
>  	u32 prio;
> @@ -2117,7 +2100,6 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
>  static const struct block_device_operations zram_devops = {
>  	.open = zram_open,
>  	.submit_bio = zram_submit_bio,
> -	.swap_slot_free_notify = zram_slot_free_notify,
>  	.owner = THIS_MODULE
>  };
>  
> @@ -2206,7 +2188,7 @@ static int zram_frontswap_load(unsigned int type, pgoff_t index,
>  
>  	err = zram_read_from_zspool(zram, page, index);
>  	if (!err)
> -		zram_accessed(zram, index);
> +		zram_free_page(zram, index);

Probably it should be in the previous commit.

>  	zram_slot_unlock(zram, index);
>  
>  	return err;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c0ffe203a602..49e1843e44ea 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1390,8 +1390,6 @@ struct block_device_operations {
>  	int (*getgeo)(struct block_device *, struct hd_geometry *);
>  	int (*set_read_only)(struct block_device *bdev, bool ro);
>  	void (*free_disk)(struct gendisk *disk);
> -	/* this callback is with swap_lock and sometimes page table lock held */
> -	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
>  	int (*report_zones)(struct gendisk *, sector_t sector,
>  			unsigned int nr_zones, report_zones_cb cb, void *data);
>  	char *(*devnode)(struct gendisk *disk, umode_t *mode);
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index a9424fd226bc..adc16d8883c8 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -726,7 +726,6 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
>  {
>  	unsigned long begin = offset;
>  	unsigned long end = offset + nr_entries - 1;
> -	void (*swap_slot_free_notify)(struct block_device *, unsigned long);
>  
>  	if (offset < si->lowest_bit)
>  		si->lowest_bit = offset;
> @@ -739,16 +738,9 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
>  	}
>  	atomic_long_add(nr_entries, &nr_swap_pages);
>  	WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries);
> -	if (si->flags & SWP_BLKDEV)
> -		swap_slot_free_notify =
> -			si->bdev->bd_disk->fops->swap_slot_free_notify;
> -	else
> -		swap_slot_free_notify = NULL;
>  	while (offset <= end) {
>  		arch_swap_invalidate_page(si->type, offset);
>  		frontswap_invalidate_page(si->type, offset);
> -		if (swap_slot_free_notify)
> -			swap_slot_free_notify(si->bdev, offset);
>  		offset++;
>  	}
>  	clear_shadow_from_swap_cache(si->type, begin, end);
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 
> 

-- 
Thank you,
Alexey

Re: [PATCH 0/3] zram: use frontswap for zram swap usecase

[Nhat Pham]

On Mon, Jul 10, 2023 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jul 10, 2023 at 03:16:56PM -0700, Minchan Kim wrote:
> > This patchset uses frontswap for zram swap usecase and remove
> > swap_slot_free_notify swap specific operation in block device.
> > It shows 13% swapout improvement for MADV_PAGEOUT.
>
> Err, no.  frontswap needs to go away, and not be tried to a block
> driver.  If you want compressed swap without a block driver please use
> zswap and help improvig it and the swap abstraction to not require
> a backing allocation for it.
>
> We need to fix swap and not pile hacks on top of hacks.
>

+1. Based on earlier discussions, it seems like the agreed upon way forward
is to remove the frontswap interface:

https://lore.kernel.org/linux-mm/20230530235447.GB102494@cmpxchg.org/

Re: [PATCH 0/3] zram: use frontswap for zram swap usecase

[Minchan Kim]

On Tue, Jul 11, 2023 at 10:52:02AM -0700, Nhat Pham wrote:
> On Mon, Jul 10, 2023 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Jul 10, 2023 at 03:16:56PM -0700, Minchan Kim wrote:
> > > This patchset uses frontswap for zram swap usecase and remove
> > > swap_slot_free_notify swap specific operation in block device.
> > > It shows 13% swapout improvement for MADV_PAGEOUT.
> >
> > Err, no.  frontswap needs to go away, and not be tried to a block
> > driver.  If you want compressed swap without a block driver please use
> > zswap and help improvig it and the swap abstraction to not require
> > a backing allocation for it.
> >
> > We need to fix swap and not pile hacks on top of hacks.
> >
> 
> +1. Based on earlier discussions, it seems like the agreed upon way forward
> is to remove the frontswap interface:
> 
> https://lore.kernel.org/linux-mm/20230530235447.GB102494@cmpxchg.org/

Thanks for feedback.

Understood. I also agree frontswap is hack so let me this patchset.
I hope swap abstraction goes to the way Chris Li suggested in the end.

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

[Minchan Kim]

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.