`zram->init_done' in fact mimics `zram->meta != NULL' value. Introduce
init_done() function that checks zram->meta (iow, checks if initialisation
was performed), so `zram->init_done' can be removed.
v2: introduce init_done()
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/staging/zram/zram_drv.c | 21 +++++++++++----------
drivers/staging/zram/zram_drv.h | 1 -
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 5bfbe0e..f1b5dbf 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -42,6 +42,11 @@ static struct zram *zram_devices;
/* Module params (documentation at end) */
static unsigned int num_devices = 1;
+static inline int init_done(struct zram *zram)
+{
+ return zram->meta != NULL;
+}
+
static inline struct zram *dev_to_zram(struct device *dev)
{
return (struct zram *)dev_to_disk(dev)->private_data;
@@ -60,7 +65,7 @@ static ssize_t initstate_show(struct device *dev,
{
struct zram *zram = dev_to_zram(dev);
- return sprintf(buf, "%u\n", zram->init_done);
+ return sprintf(buf, "%u\n", init_done(zram));
}
static ssize_t num_reads_show(struct device *dev,
@@ -133,7 +138,7 @@ static ssize_t mem_used_total_show(struct device *dev,
struct zram_meta *meta = zram->meta;
down_read(&zram->init_lock);
- if (zram->init_done)
+ if (init_done(zram))
val = zs_get_total_size_bytes(meta->mem_pool);
up_read(&zram->init_lock);
@@ -558,14 +563,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
flush_work(&zram->free_work);
down_write(&zram->init_lock);
- if (!zram->init_done) {
+ if (!init_done(zram)) {
up_write(&zram->init_lock);
return;
}
meta = zram->meta;
- zram->init_done = 0;
-
/* Free all pages that are still in this zram device */
for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
unsigned long handle = meta->table[index].handle;
@@ -604,9 +607,7 @@ static void zram_init_device(struct zram *zram, struct zram_meta *meta)
/* zram devices sort of resembles non-rotational disks */
queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
-
zram->meta = meta;
- zram->init_done = 1;
pr_debug("Initialization done!\n");
}
@@ -625,7 +626,7 @@ static ssize_t disksize_store(struct device *dev,
disksize = PAGE_ALIGN(disksize);
meta = zram_meta_alloc(disksize);
down_write(&zram->init_lock);
- if (zram->init_done) {
+ if (init_done(zram)) {
up_write(&zram->init_lock);
zram_meta_free(meta);
pr_info("Cannot change disksize for initialized device\n");
@@ -733,7 +734,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
struct zram *zram = queue->queuedata;
down_read(&zram->init_lock);
- if (unlikely(!zram->init_done))
+ if (unlikely(!init_done(zram)))
goto error;
if (!valid_io_request(zram, bio)) {
@@ -882,7 +883,7 @@ static int create_device(struct zram *zram, int device_id)
goto out_free_disk;
}
- zram->init_done = 0;
+ zram->meta = NULL;
return 0;
out_free_disk:
diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index 97a3acf..b1100cf 100644
--- a/drivers/staging/zram/zram_drv.h
+++ b/drivers/staging/zram/zram_drv.h
@@ -110,7 +110,6 @@ struct zram {
struct request_queue *queue;
struct gendisk *disk;
- int init_done;
/* Prevent concurrent execution of device init, reset and R/W request */
struct rw_semaphore init_lock;
/*
On Fri, Sep 06, 2013 at 06:21:20PM +0300, Sergey Senozhatsky wrote:
> @@ -558,14 +563,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> flush_work(&zram->free_work);
>
> down_write(&zram->init_lock);
> - if (!zram->init_done) {
> + if (!init_done(zram)) {
> up_write(&zram->init_lock);
> return;
> }
>
> meta = zram->meta;
> - zram->init_done = 0;
> -
> /* Free all pages that are still in this zram device */
> for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
> unsigned long handle = meta->table[index].handle;
> @@ -604,9 +607,7 @@ static void zram_init_device(struct zram *zram, struct zram_meta *meta)
>
> /* zram devices sort of resembles non-rotational disks */
> queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> -
> zram->meta = meta;
> - zram->init_done = 1;
>
> pr_debug("Initialization done!\n");
> }
I am uncomfortable with the locking in zram_reset_device(). There
should be a check for init_done() in zram_slot_free_notify() otherwise
we could add more work at the same time we are calling flush_work().
It should be that as soon as we start to reset then we say init is not
done, we stop loading more work, we any existing work and then clean up.
(There are details involved that I haven't looked at, but the original
code looks racy to me).
regards,
dan carpenter
On 09/09/2013 02:34 PM, Dan Carpenter wrote:
> On Fri, Sep 06, 2013 at 06:21:20PM +0300, Sergey Senozhatsky wrote:
>> @@ -558,14 +563,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>> flush_work(&zram->free_work);
>>
>> down_write(&zram->init_lock);
>> - if (!zram->init_done) {
>> + if (!init_done(zram)) {
>> up_write(&zram->init_lock);
>> return;
>> }
>>
>> meta = zram->meta;
>> - zram->init_done = 0;
>> -
>> /* Free all pages that are still in this zram device */
>> for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
>> unsigned long handle = meta->table[index].handle;
>> @@ -604,9 +607,7 @@ static void zram_init_device(struct zram *zram, struct zram_meta *meta)
>>
>> /* zram devices sort of resembles non-rotational disks */
>> queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
>> -
>> zram->meta = meta;
>> - zram->init_done = 1;
>>
>> pr_debug("Initialization done!\n");
>> }
>
> I am uncomfortable with the locking in zram_reset_device(). There
> should be a check for init_done() in zram_slot_free_notify() otherwise
> we could add more work at the same time we are calling flush_work().
>
> It should be that as soon as we start to reset then we say init is not
> done, we stop loading more work, we any existing work and then clean up.
> (There are details involved that I haven't looked at, but the original
> code looks racy to me).
Good point! I wonder why flush_work() isn't protected by init_lock.
Minchan, any reason why you did it that way?
Jerome
>
> regards,
> dan carpenter
>
>