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 <[email protected]>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
