diff options
author | Sergey Senozhatsky <sergey.senozhatsky@gmail.com> | 2015-02-12 18:00:39 -0500 |
---|---|---|
committer | Linus Torvalds <torvalds@linux-foundation.org> | 2015-02-12 21:54:11 -0500 |
commit | a096cafc31862c54da0b56c8441dc14023437008 (patch) | |
tree | e79e5326429f2c91b180091e114b11ec8f2eaf39 /drivers/block | |
parent | ba6b17d68c8e3aa8d55d0474299cb931965c5ea5 (diff) |
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>
Diffstat (limited to 'drivers/block')
-rw-r--r-- | drivers/block/zram/zram_drv.c | 75 |
1 files changed, 33 insertions, 42 deletions
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 2607bd9f4955..81ac8fd53340 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c | |||
@@ -732,8 +732,9 @@ static void zram_reset_device(struct zram *zram) | |||
732 | zram->meta = NULL; | 732 | zram->meta = NULL; |
733 | /* Reset stats */ | 733 | /* Reset stats */ |
734 | memset(&zram->stats, 0, sizeof(zram->stats)); | 734 | memset(&zram->stats, 0, sizeof(zram->stats)); |
735 | |||
736 | zram->disksize = 0; | 735 | zram->disksize = 0; |
736 | set_capacity(zram->disk, 0); | ||
737 | |||
737 | up_write(&zram->init_lock); | 738 | up_write(&zram->init_lock); |
738 | } | 739 | } |
739 | 740 | ||
@@ -826,7 +827,6 @@ static ssize_t reset_store(struct device *dev, | |||
826 | /* Make sure all pending I/O is finished */ | 827 | /* Make sure all pending I/O is finished */ |
827 | fsync_bdev(bdev); | 828 | fsync_bdev(bdev); |
828 | zram_reset_device(zram); | 829 | zram_reset_device(zram); |
829 | set_capacity(zram->disk, 0); | ||
830 | 830 | ||
831 | mutex_unlock(&bdev->bd_mutex); | 831 | mutex_unlock(&bdev->bd_mutex); |
832 | revalidate_disk(zram->disk); | 832 | revalidate_disk(zram->disk); |
@@ -1112,15 +1112,31 @@ out: | |||
1112 | return ret; | 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, | 1117 | struct zram *zram; |
1118 | &zram_disk_attr_group); | 1118 | unsigned int i; |
1119 | 1119 | ||
1120 | del_gendisk(zram->disk); | 1120 | for (i = 0; i < nr; i++) { |
1121 | put_disk(zram->disk); | 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 | static int __init zram_init(void) | 1142 | static int __init zram_init(void) |
@@ -1130,64 +1146,39 @@ static int __init zram_init(void) | |||
1130 | if (num_devices > max_num_devices) { | 1146 | if (num_devices > max_num_devices) { |
1131 | pr_warn("Invalid value for num_devices: %u\n", | 1147 | pr_warn("Invalid value for num_devices: %u\n", |
1132 | num_devices); | 1148 | num_devices); |
1133 | ret = -EINVAL; | 1149 | return -EINVAL; |
1134 | goto out; | ||
1135 | } | 1150 | } |
1136 | 1151 | ||
1137 | zram_major = register_blkdev(0, "zram"); | 1152 | zram_major = register_blkdev(0, "zram"); |
1138 | if (zram_major <= 0) { | 1153 | if (zram_major <= 0) { |
1139 | pr_warn("Unable to get major number\n"); | 1154 | pr_warn("Unable to get major number\n"); |
1140 | ret = -EBUSY; | 1155 | return -EBUSY; |
1141 | goto out; | ||
1142 | } | 1156 | } |
1143 | 1157 | ||
1144 | /* Allocate the device array and initialize each one */ | 1158 | /* Allocate the device array and initialize each one */ |
1145 | zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL); | 1159 | zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL); |
1146 | if (!zram_devices) { | 1160 | if (!zram_devices) { |
1147 | ret = -ENOMEM; | 1161 | unregister_blkdev(zram_major, "zram"); |
1148 | goto unregister; | 1162 | return -ENOMEM; |
1149 | } | 1163 | } |
1150 | 1164 | ||
1151 | for (dev_id = 0; dev_id < num_devices; dev_id++) { | 1165 | for (dev_id = 0; dev_id < num_devices; dev_id++) { |
1152 | ret = create_device(&zram_devices[dev_id], dev_id); | 1166 | ret = create_device(&zram_devices[dev_id], dev_id); |
1153 | if (ret) | 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); | 1171 | pr_info("Created %u device(s)\n", num_devices); |
1158 | |||
1159 | return 0; | 1172 | return 0; |
1160 | 1173 | ||
1161 | free_devices: | 1174 | out_error: |
1162 | while (dev_id) | 1175 | destroy_devices(dev_id); |
1163 | destroy_device(&zram_devices[--dev_id]); | ||
1164 | kfree(zram_devices); | ||
1165 | unregister: | ||
1166 | unregister_blkdev(zram_major, "zram"); | ||
1167 | out: | ||
1168 | return ret; | 1176 | return ret; |
1169 | } | 1177 | } |
1170 | 1178 | ||
1171 | static void __exit zram_exit(void) | 1179 | static void __exit zram_exit(void) |
1172 | { | 1180 | { |
1173 | int i; | 1181 | destroy_devices(num_devices); |
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"); | ||
1191 | } | 1182 | } |
1192 | 1183 | ||
1193 | module_init(zram_init); | 1184 | module_init(zram_init); |