[PATCH] zram: rework reset and destroy path

[PATCH] zram: rework reset and destroy path

[Sergey Senozhatsky]

We need to return set_capacity(disk, 0) from reset_store() back
to zram_reset_device(), a catch by Ganesh Mahendran. Potentially,
we can race set_capacity() calls from init and reset paths.

The problem is that zram_reset_device() is also getting called
from zram_exit(), which performs operations in misleading
reversed order -- we first create_device() and then init it,
while zram_exit() perform destroy_device() first and then does
zram_reset_device(). This is done to remove sysfs group before
we reset device, so we can continue with device reset/destruction
not being raced by sysfs attr write (f.e. disksize).

Apart from that, destroy_device() releases zram->disk (but we
still have ->disk pointer), so we cannot acces zram->disk in
later zram_reset_device() call, which may cause additional
errors in the future.

So, this patch rework and cleanup destroy path.

1) remove several unneeded goto labels in zram_init()
2) factor out zram_init() error path and zram_exit() into
destroy_devices() function, which takes the number of devices
to destroy as its argument.
3) remove sysfs group in destroy_devices() first, so we can
reorder operations -- reset device (as expected) goes before
disk destroy and queue cleanup. So we can always access ->disk
in zram_reset_device().
4) and, finally, return set_capacity() back under ->init_lock.

Reported-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 71 ++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 41 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a32069f..7d2e86f 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -734,8 +734,9 @@ static void zram_reset_device(struct zram *zram)
 	zram->meta = NULL;
 	/* Reset stats */
 	memset(&zram->stats, 0, sizeof(zram->stats));
-
 	zram->disksize = 0;
+	set_capacity(zram->disk, 0);
+
 	up_write(&zram->init_lock);
 }
 
@@ -828,7 +829,6 @@ static ssize_t reset_store(struct device *dev,
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
 	zram_reset_device(zram);
-	set_capacity(zram->disk, 0);
 
 	mutex_unlock(&bdev->bd_mutex);
 	revalidate_disk(zram->disk);
@@ -1114,15 +1114,29 @@ out:
 	return ret;
 }
 
-static void destroy_device(struct zram *zram)
+static void destroy_devices(unsigned int nr)
 {
-	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
-			&zram_disk_attr_group);
+	struct zram *zram;
+	unsigned int i;
 
-	del_gendisk(zram->disk);
-	put_disk(zram->disk);
+	for (i = 0; i < nr; i++) {
+		zram = &zram_devices[i];
+		/* remove sysfs first, so no one will perform disksize
+		 * store while we destroying devices */
+		sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
+				&zram_disk_attr_group);
 
-	blk_cleanup_queue(zram->queue);
+		zram_reset_device(zram);
+
+		del_gendisk(zram->disk);
+		put_disk(zram->disk);
+
+		blk_cleanup_queue(zram->queue);
+	}
+
+	kfree(zram_devices);
+	unregister_blkdev(zram_major, "zram");
+	pr_debug("Destroyed %u device(s)\n", nr);
 }
 
 static int __init zram_init(void)
@@ -1132,64 +1146,39 @@ static int __init zram_init(void)
 	if (num_devices > max_num_devices) {
 		pr_warn("Invalid value for num_devices: %u\n",
 				num_devices);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	zram_major = register_blkdev(0, "zram");
 	if (zram_major <= 0) {
 		pr_warn("Unable to get major number\n");
-		ret = -EBUSY;
-		goto out;
+		return -EBUSY;
 	}
 
 	/* Allocate the device array and initialize each one */
 	zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
 	if (!zram_devices) {
 		ret = -ENOMEM;
-		goto unregister;
+		goto out_error;
 	}
 
 	for (dev_id = 0; dev_id < num_devices; dev_id++) {
 		ret = create_device(&zram_devices[dev_id], dev_id);
 		if (ret)
-			goto free_devices;
+			goto out_error;
 	}
 
-	pr_info("Created %u device(s) ...\n", num_devices);
-
+	pr_info("Created %u device(s)\n", num_devices);
 	return 0;
 
-free_devices:
-	while (dev_id)
-		destroy_device(&zram_devices[--dev_id]);
-	kfree(zram_devices);
-unregister:
-	unregister_blkdev(zram_major, "zram");
-out:
+out_error:
+	destroy_devices(dev_id);
 	return ret;
 }
 
 static void __exit zram_exit(void)
 {
-	int i;
-	struct zram *zram;
-
-	for (i = 0; i < num_devices; i++) {
-		zram = &zram_devices[i];
-
-		destroy_device(zram);
-		/*
-		 * Shouldn't access zram->disk after destroy_device
-		 * because destroy_device already released zram->disk.
-		 */
-		zram_reset_device(zram);
-	}
-
-	unregister_blkdev(zram_major, "zram");
-
-	kfree(zram_devices);
-	pr_debug("Cleanup done!\n");
+	destroy_devices(num_devices);
 }
 
 module_init(zram_init);
-- 
2.3.0.rc2

Re: [PATCH] zram: rework reset and destroy path

[Minchan Kim]

Hello, Sergey,

On Wed, Feb 04, 2015 at 01:15:06AM +0900, Sergey Senozhatsky wrote:
> We need to return set_capacity(disk, 0) from reset_store() back
> to zram_reset_device(), a catch by Ganesh Mahendran. Potentially,
> we can race set_capacity() calls from init and reset paths.
> 
> The problem is that zram_reset_device() is also getting called
> from zram_exit(), which performs operations in misleading
> reversed order -- we first create_device() and then init it,
> while zram_exit() perform destroy_device() first and then does
> zram_reset_device(). This is done to remove sysfs group before
> we reset device, so we can continue with device reset/destruction
> not being raced by sysfs attr write (f.e. disksize).
> 
> Apart from that, destroy_device() releases zram->disk (but we
> still have ->disk pointer), so we cannot acces zram->disk in
> later zram_reset_device() call, which may cause additional
> errors in the future.
> 
> So, this patch rework and cleanup destroy path.
> 
> 1) remove several unneeded goto labels in zram_init()
> 2) factor out zram_init() error path and zram_exit() into
> destroy_devices() function, which takes the number of devices
> to destroy as its argument.
> 3) remove sysfs group in destroy_devices() first, so we can
> reorder operations -- reset device (as expected) goes before
> disk destroy and queue cleanup. So we can always access ->disk
> in zram_reset_device().
> 4) and, finally, return set_capacity() back under ->init_lock.
> 
> Reported-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Looks good to me. Minor nit below.

> ---
>  drivers/block/zram/zram_drv.c | 71 ++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index a32069f..7d2e86f 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -734,8 +734,9 @@ static void zram_reset_device(struct zram *zram)
>  	zram->meta = NULL;
>  	/* Reset stats */
>  	memset(&zram->stats, 0, sizeof(zram->stats));
> -
>  	zram->disksize = 0;
> +	set_capacity(zram->disk, 0);
> +
>  	up_write(&zram->init_lock);
>  }
>  
> @@ -828,7 +829,6 @@ static ssize_t reset_store(struct device *dev,
>  	/* Make sure all pending I/O is finished */
>  	fsync_bdev(bdev);
>  	zram_reset_device(zram);
> -	set_capacity(zram->disk, 0);
>  
>  	mutex_unlock(&bdev->bd_mutex);
>  	revalidate_disk(zram->disk);
> @@ -1114,15 +1114,29 @@ out:
>  	return ret;
>  }
>  
> -static void destroy_device(struct zram *zram)
> +static void destroy_devices(unsigned int nr)
>  {
> -	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
> -			&zram_disk_attr_group);
> +	struct zram *zram;
> +	unsigned int i;
>  
> -	del_gendisk(zram->disk);
> -	put_disk(zram->disk);
> +	for (i = 0; i < nr; i++) {
> +		zram = &zram_devices[i];
> +		/* remove sysfs first, so no one will perform disksize
> +		 * store while we destroying devices */
> +		sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
> +				&zram_disk_attr_group);
>  
> -	blk_cleanup_queue(zram->queue);
> +		zram_reset_device(zram);
> +
> +		del_gendisk(zram->disk);
> +		put_disk(zram->disk);
> +
> +		blk_cleanup_queue(zram->queue);
> +	}
> +
> +	kfree(zram_devices);
> +	unregister_blkdev(zram_major, "zram");
> +	pr_debug("Destroyed %u device(s)\n", nr);

Create_device just shows the number of created device so I think
no worth to emit per-device information in destroy_devices.
Let's just emit clean up done like old in zram_exit but
use pr_info instead of pr_debug.

Another concern is I'd like to keep per-device interface(e,g.
create_device, destroy_device) because there was requirement
to add new zram device dynamically. I guess you could remember
that. Although I didn't have a enough time to response,
Alex finally convinced me so I hope a contributor who have time
will do it if he has an interest about that.
For it, per-device creating/destroy interface looks better.
https://lkml.org/lkml/2014/8/8/142
Anyway, I cannot expect it happens sooner so I'm not strong
against your patch(ie, create_device, destroy_devices)
because I think we could do refactoring it when we need it.

Thanks.


>  }
>  
>  static int __init zram_init(void)
> @@ -1132,64 +1146,39 @@ static int __init zram_init(void)
>  	if (num_devices > max_num_devices) {
>  		pr_warn("Invalid value for num_devices: %u\n",
>  				num_devices);
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>  	}
>  
>  	zram_major = register_blkdev(0, "zram");
>  	if (zram_major <= 0) {
>  		pr_warn("Unable to get major number\n");
> -		ret = -EBUSY;
> -		goto out;
> +		return -EBUSY;
>  	}
>  
>  	/* Allocate the device array and initialize each one */
>  	zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
>  	if (!zram_devices) {
>  		ret = -ENOMEM;
> -		goto unregister;
> +		goto out_error;
>  	}
>  
>  	for (dev_id = 0; dev_id < num_devices; dev_id++) {
>  		ret = create_device(&zram_devices[dev_id], dev_id);
>  		if (ret)
> -			goto free_devices;
> +			goto out_error;
>  	}
>  
> -	pr_info("Created %u device(s) ...\n", num_devices);
> -
> +	pr_info("Created %u device(s)\n", num_devices);
>  	return 0;
>  
> -free_devices:
> -	while (dev_id)
> -		destroy_device(&zram_devices[--dev_id]);
> -	kfree(zram_devices);
> -unregister:
> -	unregister_blkdev(zram_major, "zram");
> -out:
> +out_error:
> +	destroy_devices(dev_id);
>  	return ret;
>  }
>  
>  static void __exit zram_exit(void)
>  {
> -	int i;
> -	struct zram *zram;
> -
> -	for (i = 0; i < num_devices; i++) {
> -		zram = &zram_devices[i];
> -
> -		destroy_device(zram);
> -		/*
> -		 * Shouldn't access zram->disk after destroy_device
> -		 * because destroy_device already released zram->disk.
> -		 */
> -		zram_reset_device(zram);
> -	}
> -
> -	unregister_blkdev(zram_major, "zram");
> -
> -	kfree(zram_devices);
> -	pr_debug("Cleanup done!\n");
> +	destroy_devices(num_devices);
>  }
>  
>  module_init(zram_init);
> -- 
> 2.3.0.rc2
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: rework reset and destroy path

[Sergey Senozhatsky]

Hello Minchan,

On (02/04/15 08:42), Minchan Kim wrote:
> > +	kfree(zram_devices);
> > +	unregister_blkdev(zram_major, "zram");
> > +	pr_debug("Destroyed %u device(s)\n", nr);
> 
> Create_device just shows the number of created device so I think
> no worth to emit per-device information in destroy_devices.
> Let's just emit clean up done like old in zram_exit but
> use pr_info instead of pr_debug.

not critical let's keep it as is (it just mirrors the message from init()),
and I wouldn't say it's totally useless now. we allocate space for devices,
disk, queue, etc. and we destroy it here.

please see below.

> Another concern is I'd like to keep per-device interface(e,g.
> create_device, destroy_device) because there was requirement
> to add new zram device dynamically. I guess you could remember
> that. Although I didn't have a enough time to response,
> Alex finally convinced me so I hope a contributor who have time
> will do it if he has an interest about that.

yes, I was going to tell you that perhaps I'll do that. I had several
discussions on google+ and it seems that people want to see this
feature. so I was going to ask your opinion.

the solution I'm thinking about now is to replace zram devices fixed
size array with a list, protected by mutex or spin lock (doesn't
matter at this point). this will change destroy_devices() from array
iteration to destroy each list entry.

so:
a)	pr_debug("Destroyed %u device(s)\n", nr)
it will show the actual number of active devices by that time.

b) I'll refactor destroy_devices().

this rework will not make it into the upcoming merge window, so we'll
have enough time.

I haven't decided yet, if I wan't to keep released zram devices in the
list (idle zram devices list, picking up the first available device
when user requests new zram device) or I will destroy abondoned devices
and , thus, the list will represent only active devices.


I tend to select the latter one -- destroy unused zram devices. I don't
want to give additional sysfs knob to limit the number of idle devices,
waste memory for idle devices, etc. We already  have a number of quite
complicated knobs.


	-ss

> For it, per-device creating/destroy interface looks better.
> https://lkml.org/lkml/2014/8/8/142
> Anyway, I cannot expect it happens sooner so I'm not strong
> against your patch(ie, create_device, destroy_devices)
> because I think we could do refactoring it when we need it.
> 

Re: [PATCH] zram: rework reset and destroy path

[Minchan Kim]

On Wed, Feb 04, 2015 at 09:24:51AM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (02/04/15 08:42), Minchan Kim wrote:
> > > +	kfree(zram_devices);
> > > +	unregister_blkdev(zram_major, "zram");
> > > +	pr_debug("Destroyed %u device(s)\n", nr);
> > 
> > Create_device just shows the number of created device so I think
> > no worth to emit per-device information in destroy_devices.
> > Let's just emit clean up done like old in zram_exit but
> > use pr_info instead of pr_debug.
> 
> not critical let's keep it as is (it just mirrors the message from init()),
> and I wouldn't say it's totally useless now. we allocate space for devices,
> disk, queue, etc. and we destroy it here.

As I said, it is just minor but at least want to fix pr_debug into
pr_info.

> 
> please see below.
> 
> > Another concern is I'd like to keep per-device interface(e,g.
> > create_device, destroy_device) because there was requirement
> > to add new zram device dynamically. I guess you could remember
> > that. Although I didn't have a enough time to response,
> > Alex finally convinced me so I hope a contributor who have time
> > will do it if he has an interest about that.
> 
> yes, I was going to tell you that perhaps I'll do that. I had several
> discussions on google+ and it seems that people want to see this
> feature. so I was going to ask your opinion.
> 
> the solution I'm thinking about now is to replace zram devices fixed
> size array with a list, protected by mutex or spin lock (doesn't
> matter at this point). this will change destroy_devices() from array
> iteration to destroy each list entry.

Yeb.

> 
> so:
> a)	pr_debug("Destroyed %u device(s)\n", nr)
> it will show the actual number of active devices by that time.
> 
> b) I'll refactor destroy_devices().
> 
> this rework will not make it into the upcoming merge window, so we'll
> have enough time.
> 
> I haven't decided yet, if I wan't to keep released zram devices in the
> list (idle zram devices list, picking up the first available device
> when user requests new zram device) or I will destroy abondoned devices
> and , thus, the list will represent only active devices.

I perfer destroy abandoned devices.

> 
> 
> I tend to select the latter one -- destroy unused zram devices. I don't
> want to give additional sysfs knob to limit the number of idle devices,
> waste memory for idle devices, etc. We already  have a number of quite
> complicated knobs.

True.

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: rework reset and destroy path

[Sergey Senozhatsky]

On (02/04/15 09:35), Minchan Kim wrote:
> > > Create_device just shows the number of created device so I think
> > > no worth to emit per-device information in destroy_devices.
> > > Let's just emit clean up done like old in zram_exit but
> > > use pr_info instead of pr_debug.
> > 
> > not critical let's keep it as is (it just mirrors the message from init()),
> > and I wouldn't say it's totally useless now. we allocate space for devices,
> > disk, queue, etc. and we destroy it here.
> 
> As I said, it is just minor but at least want to fix pr_debug into
> pr_info.

ah, didn't catch your pr_info() <-> pr_debug() note. need more coffee.
the original message was printed by pr_debug().
will send a one-liner.

	-ss