Re: [RFC PATCHv5 4/6] zram: drop wb_limit_lock

[Brian Geffon <bgeffon@google.com>]

On Thu, Nov 20, 2025 at 10:22 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> We don't need wb_limit_lock.  Writeback limit setters take an
> exclusive write zram init_lock, while wb_limit modifications
> happen only from a single task and under zram read init_lock.
> No concurrent wb_limit modifications are possible (we permit
> only one post-processing task at a time).  Add lockdep
> assertions to wb_limit mutators.
>
> While at it, fixup coding styles.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Reviewed-by: Brian Geffon <bgeffon@google.com>

> ---
>  drivers/block/zram/zram_drv.c | 22 +++++-----------------
>  drivers/block/zram/zram_drv.h |  1 -
>  2 files changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 71f37b960812..671ef2ec9b11 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -530,9 +530,7 @@ static ssize_t writeback_limit_enable_store(struct device *dev,
>                 return ret;
>
>         down_write(&zram->init_lock);
> -       spin_lock(&zram->wb_limit_lock);
>         zram->wb_limit_enable = val;
> -       spin_unlock(&zram->wb_limit_lock);
>         up_write(&zram->init_lock);
>         ret = len;
>
> @@ -547,9 +545,7 @@ static ssize_t writeback_limit_enable_show(struct device *dev,
>         struct zram *zram = dev_to_zram(dev);
>
>         down_read(&zram->init_lock);
> -       spin_lock(&zram->wb_limit_lock);
>         val = zram->wb_limit_enable;
> -       spin_unlock(&zram->wb_limit_lock);
>         up_read(&zram->init_lock);
>
>         return sysfs_emit(buf, "%d\n", val);
> @@ -567,9 +563,7 @@ static ssize_t writeback_limit_store(struct device *dev,
>                 return ret;
>
>         down_write(&zram->init_lock);
> -       spin_lock(&zram->wb_limit_lock);
>         zram->bd_wb_limit = val;
> -       spin_unlock(&zram->wb_limit_lock);
>         up_write(&zram->init_lock);
>         ret = len;
>
> @@ -577,15 +571,13 @@ static ssize_t writeback_limit_store(struct device *dev,
>  }
>
>  static ssize_t writeback_limit_show(struct device *dev,
> -               struct device_attribute *attr, char *buf)
> +                                   struct device_attribute *attr, char *buf)
>  {
>         u64 val;
>         struct zram *zram = dev_to_zram(dev);
>
>         down_read(&zram->init_lock);
> -       spin_lock(&zram->wb_limit_lock);
>         val = zram->bd_wb_limit;
> -       spin_unlock(&zram->wb_limit_lock);
>         up_read(&zram->init_lock);
>
>         return sysfs_emit(buf, "%llu\n", val);
> @@ -869,18 +861,18 @@ static struct zram_wb_ctl *init_wb_ctl(struct zram *zram)
>
>  static void zram_account_writeback_rollback(struct zram *zram)
>  {
> -       spin_lock(&zram->wb_limit_lock);
> +       lockdep_assert_held_read(&zram->init_lock);
> +
>         if (zram->wb_limit_enable)
>                 zram->bd_wb_limit +=  1UL << (PAGE_SHIFT - 12);
> -       spin_unlock(&zram->wb_limit_lock);
>  }
>
>  static void zram_account_writeback_submit(struct zram *zram)
>  {
> -       spin_lock(&zram->wb_limit_lock);
> +       lockdep_assert_held_read(&zram->init_lock);
> +
>         if (zram->wb_limit_enable && zram->bd_wb_limit > 0)
>                 zram->bd_wb_limit -=  1UL << (PAGE_SHIFT - 12);
> -       spin_unlock(&zram->wb_limit_lock);
>  }
>
>  static int zram_writeback_complete(struct zram *zram, struct zram_wb_req *req)
> @@ -1004,13 +996,10 @@ static int zram_writeback_slots(struct zram *zram,
>         u32 index = 0;
>
>         while ((pps = select_pp_slot(ctl))) {
> -               spin_lock(&zram->wb_limit_lock);
>                 if (zram->wb_limit_enable && !zram->bd_wb_limit) {
> -                       spin_unlock(&zram->wb_limit_lock);
>                         ret = -EIO;
>                         break;
>                 }
> -               spin_unlock(&zram->wb_limit_lock);
>
>                 while (!req) {
>                         req = zram_select_idle_req(wb_ctl);
> @@ -2961,7 +2950,6 @@ static int zram_add(void)
>         init_rwsem(&zram->init_lock);
>  #ifdef CONFIG_ZRAM_WRITEBACK
>         zram->wb_batch_size = 32;
> -       spin_lock_init(&zram->wb_limit_lock);
>  #endif
>
>         /* gendisk structure */
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 1a647f42c1a4..c6d94501376c 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -127,7 +127,6 @@ struct zram {
>         bool claim; /* Protected by disk->open_mutex */
>  #ifdef CONFIG_ZRAM_WRITEBACK
>         struct file *backing_dev;
> -       spinlock_t wb_limit_lock;
>         bool wb_limit_enable;
>         u32 wb_batch_size;
>         u64 bd_wb_limit;
> --
> 2.52.0.rc1.455.g30608eb744-goog
>