* [PATCH] zram: move init_done check to the beginning of disksize_store
@ 2015-10-31 14:50 Geliang Tang
2015-11-01 23:43 ` Sergey Senozhatsky
0 siblings, 1 reply; 4+ messages in thread
From: Geliang Tang @ 2015-10-31 14:50 UTC (permalink / raw)
To: Minchan Kim, Nitin Gupta, Sergey Senozhatsky; +Cc: Geliang Tang, linux-kernel
If we set disksize when disksize has been set, we will get the following
error report: "write error: Cannot allocate memory". This is because
disksize_store fails at zram_meta_alloc. This is not what we expect.
To solve this problem, this patch moves init_done check to the beginning
of disksize_store.
Signed-off-by: Geliang Tang <geliangtang@163.com>
---
drivers/block/zram/zram_drv.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 81a557c..609fc2b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1028,6 +1028,14 @@ static ssize_t disksize_store(struct device *dev,
struct zram *zram = dev_to_zram(dev);
int err;
+ down_read(&zram->init_lock);
+ if (init_done(zram)) {
+ up_read(&zram->init_lock);
+ pr_info("Cannot change disksize for initialized device\n");
+ return -EBUSY;
+ }
+ up_read(&zram->init_lock);
+
disksize = memparse(buf, NULL);
if (!disksize)
return -EINVAL;
@@ -1046,12 +1054,6 @@ static ssize_t disksize_store(struct device *dev,
}
down_write(&zram->init_lock);
- if (init_done(zram)) {
- pr_info("Cannot change disksize for initialized device\n");
- err = -EBUSY;
- goto out_destroy_comp;
- }
-
init_waitqueue_head(&zram->io_done);
atomic_set(&zram->refcount, 1);
zram->meta = meta;
@@ -1069,9 +1071,6 @@ static ssize_t disksize_store(struct device *dev,
return len;
-out_destroy_comp:
- up_write(&zram->init_lock);
- zcomp_destroy(comp);
out_free_meta:
zram_meta_free(meta, disksize);
return err;
--
2.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] zram: move init_done check to the beginning of disksize_store
2015-10-31 14:50 [PATCH] zram: move init_done check to the beginning of disksize_store Geliang Tang
@ 2015-11-01 23:43 ` Sergey Senozhatsky
2015-11-02 13:53 ` Geliang Tang
0 siblings, 1 reply; 4+ messages in thread
From: Sergey Senozhatsky @ 2015-11-01 23:43 UTC (permalink / raw)
To: Geliang Tang
Cc: Minchan Kim, Andrew Morton, Sergey Senozhatsky, linux-kernel,
Sergey Senozhatsky
On (10/31/15 22:50), Geliang Tang wrote:
>
> If we set disksize when disksize has been set, we will get the following
> error report: "write error: Cannot allocate memory". This is because
> disksize_store fails at zram_meta_alloc.
Those things are not connected, absolutely. zram_meta_alloc() can fail
even on un-initialized device. In any case disksize_store() does not end
up changing the state of the device and reports the error back, so I don't
see any real value in this change. Seems that we come across this change
something like once a year (this patch is not the first).
I believe it has to stay the way it is, see https://lkml.org/lkml/2014/2/27/674
-ss
> This is not what we expect.
> To solve this problem, this patch moves init_done check to the beginning
> of disksize_store.
>
> Signed-off-by: Geliang Tang <geliangtang@163.com>
> ---
> drivers/block/zram/zram_drv.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 81a557c..609fc2b 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1028,6 +1028,14 @@ static ssize_t disksize_store(struct device *dev,
> struct zram *zram = dev_to_zram(dev);
> int err;
>
> + down_read(&zram->init_lock);
> + if (init_done(zram)) {
> + up_read(&zram->init_lock);
> + pr_info("Cannot change disksize for initialized device\n");
> + return -EBUSY;
> + }
> + up_read(&zram->init_lock);
> +
> disksize = memparse(buf, NULL);
> if (!disksize)
> return -EINVAL;
> @@ -1046,12 +1054,6 @@ static ssize_t disksize_store(struct device *dev,
> }
>
> down_write(&zram->init_lock);
> - if (init_done(zram)) {
> - pr_info("Cannot change disksize for initialized device\n");
> - err = -EBUSY;
> - goto out_destroy_comp;
> - }
> -
> init_waitqueue_head(&zram->io_done);
> atomic_set(&zram->refcount, 1);
> zram->meta = meta;
> @@ -1069,9 +1071,6 @@ static ssize_t disksize_store(struct device *dev,
>
> return len;
>
> -out_destroy_comp:
> - up_write(&zram->init_lock);
> - zcomp_destroy(comp);
> out_free_meta:
> zram_meta_free(meta, disksize);
> return err;
> --
> 2.4.3
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] zram: move init_done check to the beginning of disksize_store
2015-11-01 23:43 ` Sergey Senozhatsky
@ 2015-11-02 13:53 ` Geliang Tang
2015-11-03 0:18 ` Sergey Senozhatsky
0 siblings, 1 reply; 4+ messages in thread
From: Geliang Tang @ 2015-11-02 13:53 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Minchan Kim, Nitin Gupta, linux-kernel, Geliang Tang
On Mon, Nov 02, 2015 at 08:43:58AM +0900, Sergey Senozhatsky wrote:
> On (10/31/15 22:50), Geliang Tang wrote:
> >
> > If we set disksize when disksize has been set, we will get the following
> > error report: "write error: Cannot allocate memory". This is because
> > disksize_store fails at zram_meta_alloc.
>
> Those things are not connected, absolutely. zram_meta_alloc() can fail
> even on un-initialized device. In any case disksize_store() does not end
> up changing the state of the device and reports the error back, so I don't
> see any real value in this change. Seems that we come across this change
> something like once a year (this patch is not the first).
Thanks for your reply.
zram_meta_alloc() always fails on initialized device at debugfs_create_dir():
debugfs dir <zram0> creation failed
zram: Error creating memory pool
I think we should deal with this error.
>
> I believe it has to stay the way it is, see https://lkml.org/lkml/2014/2/27/674
Unlike above, this patch did not call zram_meta_alloc() in lock. So it
would not cause the deed lock problem.
Geliang Tang
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] zram: move init_done check to the beginning of disksize_store
2015-11-02 13:53 ` Geliang Tang
@ 2015-11-03 0:18 ` Sergey Senozhatsky
0 siblings, 0 replies; 4+ messages in thread
From: Sergey Senozhatsky @ 2015-11-03 0:18 UTC (permalink / raw)
To: Geliang Tang
Cc: Minchan Kim, Andrew Morton, linux-kernel, Sergey Senozhatsky,
Sergey Senozhatsky
Hi,
[please don't trim the Cc list]
On (11/02/15 21:53), Geliang Tang wrote:
> > Those things are not connected, absolutely. zram_meta_alloc() can fail
> > even on un-initialized device. In any case disksize_store() does not end
> > up changing the state of the device and reports the error back, so I don't
> > see any real value in this change. Seems that we come across this change
> > something like once a year (this patch is not the first).
>
> Thanks for your reply.
>
> zram_meta_alloc() always fails on initialized device at debugfs_create_dir():
> debugfs dir <zram0> creation failed
Aha, so you are talking about calling disksize_store() on
- already initialized device that
- uses zsmalloc with
- CONFIG_ZSMALLOC_STAT as an alacator
(zbud is not affected). that should have been mentioned in the commit message.
hm, does the patch _actually_ make it better? splitting a ->init_lock regions
is *never* a good idea. consider an un-initialized device.
CPU0 CPU1
down_read(&zram->init_lock);
if (init_done(zram))
return -EBUSY;
up_read(&zram->init_lock);
down_read(&zram->init_lock);
if (init_done(zram))
return -EBUSY;
up_read(&zram->init_lock);
meta = zram_meta_alloc();
meta = zram_meta_alloc();
comp = zcomp_create();
comp = zcomp_create();
down_write(&zram->init_lock);
zram->meta = meta;
...
up_write(&zram->init_lock);
down_write(&zram->init_lock);
zram->meta = meta;
...
up_write(&zram->init_lock);
So the above code will return a "debugfs dir <zram0> creation failed"
IFF zsmalloc with CONFIG_ZSMALLOC_STAT enabled is being used (regardless the
effort). And it will totally succeed and lead to N (depending on the number of
concurrent disksize_store() calls) pool memory and zcomp_create memory leaks
otherwise. Because of this part:
---
down_write(&zram->init_lock);
- if (init_done(zram)) {
- pr_info("Cannot change disksize for initialized device\n");
- err = -EBUSY;
- goto out_destroy_comp;
- }
-
init_waitqueue_head(&zram->io_done);
atomic_set(&zram->refcount, 1);
zram->meta = meta;
---
> zram: Error creating memory pool
>
> I think we should deal with this error.
Is it so critical? I mean it does not break the existing device; we
print the error and keep the existing device fully functional. The
error line may be a bit misleading, which is another thing and can
be solved documenting the behavior, *for example*. (I just don't
really want to complicate the code for a corner case which doesn't
even break anything).
-ss
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-03 0:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-31 14:50 [PATCH] zram: move init_done check to the beginning of disksize_store Geliang Tang
2015-11-01 23:43 ` Sergey Senozhatsky
2015-11-02 13:53 ` Geliang Tang
2015-11-03 0:18 ` Sergey Senozhatsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.