Re: [PATCH 6/7] zram: remove workqueue for freeing removed pending slot

[Sergey Senozhatsky <sergey.senozhatsky@gmail.com>]

On (01/13/14 20:19), Minchan Kim wrote:
> [1] introduced free request pending code to avoid scheduling
> by mutex under spinlock and it was a mess which made code
> lenghty and increased overhead.
> 
> Now, we don't need zram->lock any more to free slot so
> this patch reverts it and then, tb_lock should protect it.
> 
> [1] a0c516c, zram: don't grab mutex in zram_slot_free_noity
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  drivers/block/zram/zram_drv.c | 54 +++++--------------------------------------
>  drivers/block/zram/zram_drv.h | 10 --------
>  2 files changed, 6 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 24e6426..f1a3c95 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -522,20 +522,6 @@ out:
>  	return ret;
>  }
>  
> -static void handle_pending_slot_free(struct zram *zram)
> -{
> -	struct zram_slot_free *free_rq;
> -
> -	spin_lock(&zram->slot_free_lock);
> -	while (zram->slot_free_rq) {
> -		free_rq = zram->slot_free_rq;
> -		zram->slot_free_rq = free_rq->next;
> -		zram_free_page(zram, free_rq->index);
> -		kfree(free_rq);
> -	}
> -	spin_unlock(&zram->slot_free_lock);
> -}
> -
>  static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
>  			int offset, struct bio *bio, int rw)
>  {
> @@ -547,7 +533,6 @@ static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		up_read(&zram->lock);
>  	} else {
>  		down_write(&zram->lock);
> -		handle_pending_slot_free(zram);
>  		ret = zram_bvec_write(zram, bvec, index, offset);
>  		up_write(&zram->lock);
>  	}
> @@ -566,8 +551,6 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>  		return;
>  	}
>  
> -	flush_work(&zram->free_work);
> -
>  	meta = zram->meta;
>  	zram->init_done = 0;
>  
> @@ -769,40 +752,19 @@ error:
>  	bio_io_error(bio);
>  }
>  
> -static void zram_slot_free(struct work_struct *work)
> -{
> -	struct zram *zram;
> -
> -	zram = container_of(work, struct zram, free_work);
> -	down_write(&zram->lock);
> -	handle_pending_slot_free(zram);
> -	up_write(&zram->lock);
> -}
> -
> -static void add_slot_free(struct zram *zram, struct zram_slot_free *free_rq)
> -{
> -	spin_lock(&zram->slot_free_lock);
> -	free_rq->next = zram->slot_free_rq;
> -	zram->slot_free_rq = free_rq;
> -	spin_unlock(&zram->slot_free_lock);
> -}
> -
>  static void zram_slot_free_notify(struct block_device *bdev,
>  				unsigned long index)
>  {
>  	struct zram *zram;
> -	struct zram_slot_free *free_rq;
> +	struct zram_meta *meta;
>  
>  	zram = bdev->bd_disk->private_data;
> -	atomic64_inc(&zram->stats.notify_free);
> -
> -	free_rq = kmalloc(sizeof(struct zram_slot_free), GFP_ATOMIC);
> -	if (!free_rq)
> -		return;
> +	meta = zram->meta;
>  
> -	free_rq->index = index;
> -	add_slot_free(zram, free_rq);
> -	schedule_work(&zram->free_work);
> +	write_lock(&meta->tb_lock);
> +	zram_free_page(zram, index);
> +	write_unlock(&meta->tb_lock);
> +	atomic64_inc(&zram->stats.notify_free);
>  }
>  

Hello Minchan,
I think we need to down_write init_lock in zram_slot_free_notify(),
and thus can avoid locking meta->tb_lock. otherwise, I think,
there is a chance that zram_slot_free_notify() can race with
device reset, e.g.
	
	zram_slot_free_notify()			zram_reset_device()
						down_write(&zram->init_lock);
	meta = zram->meta
						zram_meta_free(zram->meta);
						zram->meta = NULL;
	write_lock(&meta->tb_lock);
	[...]
	write_unlock(&meta->tb_lock);
						[..]
						up_write(&zram->init_lock);

	-ss

>  static const struct block_device_operations zram_devops = {
> @@ -849,10 +811,6 @@ static int create_device(struct zram *zram, int device_id)
>  	init_rwsem(&zram->lock);
>  	init_rwsem(&zram->init_lock);
>  
> -	INIT_WORK(&zram->free_work, zram_slot_free);
> -	spin_lock_init(&zram->slot_free_lock);
> -	zram->slot_free_rq = NULL;
> -
>  	zram->queue = blk_alloc_queue(GFP_KERNEL);
>  	if (!zram->queue) {
>  		pr_err("Error allocating disk queue for device %d\n",
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index c3f453f..d876300 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -90,20 +90,11 @@ struct zram_meta {
>  	struct zs_pool *mem_pool;
>  };
>  
> -struct zram_slot_free {
> -	unsigned long index;
> -	struct zram_slot_free *next;
> -};
> -
>  struct zram {
>  	struct zram_meta *meta;
>  	struct rw_semaphore lock; /* protect compression buffers,
>  				   * reads and writes
>  				   */
> -
> -	struct work_struct free_work;  /* handle pending free request */
> -	struct zram_slot_free *slot_free_rq; /* list head of free request */
> -
>  	struct request_queue *queue;
>  	struct gendisk *disk;
>  	int init_done;
> @@ -114,7 +105,6 @@ struct zram {
>  	 * we can store in a disk.
>  	 */
>  	u64 disksize;	/* bytes */
> -	spinlock_t slot_free_lock;
>  
>  	struct zram_stats stats;
>  };
> -- 
> 1.8.4.3
>