* [PATCH 2/2] staging: zram: remove init_done from zram struct (v2)
@ 2013-09-06 15:21 Sergey Senozhatsky
2013-09-09 12:34 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Sergey Senozhatsky @ 2013-09-06 15:21 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jerome Marchand, Minchan Kim, devel, linux-kernel
`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 <sergey.senozhatsky@gmail.com>
---
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;
/*
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] staging: zram: remove init_done from zram struct (v2)
2013-09-06 15:21 [PATCH 2/2] staging: zram: remove init_done from zram struct (v2) Sergey Senozhatsky
@ 2013-09-09 12:34 ` Dan Carpenter
2013-09-09 13:51 ` Jerome Marchand
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2013-09-09 12:34 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Greg Kroah-Hartman, devel, Minchan Kim, Jerome Marchand,
linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] staging: zram: remove init_done from zram struct (v2)
2013-09-09 12:34 ` Dan Carpenter
@ 2013-09-09 13:51 ` Jerome Marchand
0 siblings, 0 replies; 3+ messages in thread
From: Jerome Marchand @ 2013-09-09 13:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sergey Senozhatsky, Greg Kroah-Hartman, devel, Minchan Kim,
linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-09-09 13:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-06 15:21 [PATCH 2/2] staging: zram: remove init_done from zram struct (v2) Sergey Senozhatsky
2013-09-09 12:34 ` Dan Carpenter
2013-09-09 13:51 ` Jerome Marchand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox