Commit a096cafc31862c54da0b56c8441dc14023437008

Authored by Sergey Senozhatsky
Committed by Linus Torvalds
1 parent ba6b17d68c

zram: rework reset and destroy path

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.

[akpm@linux-foundation.org: tweak comment]
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Jerome Marchand <jmarchan@redhat.com>
Cc: Nitin Gupta <ngupta@vflare.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Showing 1 changed file with 33 additions and 42 deletions Side-by-side Diff

drivers/block/zram/zram_drv.c
... ... @@ -732,8 +732,9 @@
732 732 zram->meta = NULL;
733 733 /* Reset stats */
734 734 memset(&zram->stats, 0, sizeof(zram->stats));
735   -
736 735 zram->disksize = 0;
  736 + set_capacity(zram->disk, 0);
  737 +
737 738 up_write(&zram->init_lock);
738 739 }
739 740  
... ... @@ -826,7 +827,6 @@
826 827 /* Make sure all pending I/O is finished */
827 828 fsync_bdev(bdev);
828 829 zram_reset_device(zram);
829   - set_capacity(zram->disk, 0);
830 830  
831 831 mutex_unlock(&bdev->bd_mutex);
832 832 revalidate_disk(zram->disk);
833 833  
834 834  
835 835  
... ... @@ -1112,15 +1112,31 @@
1112 1112 return ret;
1113 1113 }
1114 1114  
1115   -static void destroy_device(struct zram *zram)
  1115 +static void destroy_devices(unsigned int nr)
1116 1116 {
1117   - sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
1118   - &zram_disk_attr_group);
  1117 + struct zram *zram;
  1118 + unsigned int i;
1119 1119  
1120   - del_gendisk(zram->disk);
1121   - put_disk(zram->disk);
  1120 + for (i = 0; i < nr; i++) {
  1121 + zram = &zram_devices[i];
  1122 + /*
  1123 + * Remove sysfs first, so no one will perform a disksize
  1124 + * store while we destroy the devices
  1125 + */
  1126 + sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
  1127 + &zram_disk_attr_group);
1122 1128  
1123   - blk_cleanup_queue(zram->queue);
  1129 + zram_reset_device(zram);
  1130 +
  1131 + del_gendisk(zram->disk);
  1132 + put_disk(zram->disk);
  1133 +
  1134 + blk_cleanup_queue(zram->queue);
  1135 + }
  1136 +
  1137 + kfree(zram_devices);
  1138 + unregister_blkdev(zram_major, "zram");
  1139 + pr_info("Destroyed %u device(s)\n", nr);
1124 1140 }
1125 1141  
1126 1142 static int __init zram_init(void)
1127 1143  
1128 1144  
1129 1145  
1130 1146  
1131 1147  
1132 1148  
... ... @@ -1130,64 +1146,39 @@
1130 1146 if (num_devices > max_num_devices) {
1131 1147 pr_warn("Invalid value for num_devices: %u\n",
1132 1148 num_devices);
1133   - ret = -EINVAL;
1134   - goto out;
  1149 + return -EINVAL;
1135 1150 }
1136 1151  
1137 1152 zram_major = register_blkdev(0, "zram");
1138 1153 if (zram_major <= 0) {
1139 1154 pr_warn("Unable to get major number\n");
1140   - ret = -EBUSY;
1141   - goto out;
  1155 + return -EBUSY;
1142 1156 }
1143 1157  
1144 1158 /* Allocate the device array and initialize each one */
1145 1159 zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
1146 1160 if (!zram_devices) {
1147   - ret = -ENOMEM;
1148   - goto unregister;
  1161 + unregister_blkdev(zram_major, "zram");
  1162 + return -ENOMEM;
1149 1163 }
1150 1164  
1151 1165 for (dev_id = 0; dev_id < num_devices; dev_id++) {
1152 1166 ret = create_device(&zram_devices[dev_id], dev_id);
1153 1167 if (ret)
1154   - goto free_devices;
  1168 + goto out_error;
1155 1169 }
1156 1170  
1157   - pr_info("Created %u device(s) ...\n", num_devices);
1158   -
  1171 + pr_info("Created %u device(s)\n", num_devices);
1159 1172 return 0;
1160 1173  
1161   -free_devices:
1162   - while (dev_id)
1163   - destroy_device(&zram_devices[--dev_id]);
1164   - kfree(zram_devices);
1165   -unregister:
1166   - unregister_blkdev(zram_major, "zram");
1167   -out:
  1174 +out_error:
  1175 + destroy_devices(dev_id);
1168 1176 return ret;
1169 1177 }
1170 1178  
1171 1179 static void __exit zram_exit(void)
1172 1180 {
1173   - int i;
1174   - struct zram *zram;
1175   -
1176   - for (i = 0; i < num_devices; i++) {
1177   - zram = &zram_devices[i];
1178   -
1179   - destroy_device(zram);
1180   - /*
1181   - * Shouldn't access zram->disk after destroy_device
1182   - * because destroy_device already released zram->disk.
1183   - */
1184   - zram_reset_device(zram);
1185   - }
1186   -
1187   - unregister_blkdev(zram_major, "zram");
1188   -
1189   - kfree(zram_devices);
1190   - pr_debug("Cleanup done!\n");
  1181 + destroy_devices(num_devices);
1191 1182 }
1192 1183  
1193 1184 module_init(zram_init);