Before resetting and removing one zram disk, disk->open_mutex is acquired,
and ->claim is set to prevent new open and zram_remove from being done,
then disk->open_mutex is released, and flush dirty pages and reset the zram
disk.
Turns out it needn't to be so complicated, we can simply flush dirty
pages and reset the zram disk with holding disk->open_mutex. Meantime
we can't fail to remove zram in case that it is running from module
exit, otherwise we will leak the zram device.
bdev_disk_changed() already runs sync_blockdev() with holding
disk->open_mutex, so this way is safe. Also it is safe to call
zram_reset_device() with ->open_mutex too, just we need to deal with
lockdep false warning since backing_dev_store() holds init lock before
acquiring backing device's open_mutex.
More importantly, we can avoid to fail removing zram when unloading module,
then zram device won't be leaked and the warning of 'Error: Removing state
63 which has instances left.' can be fixed.
Reported-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
---
drivers/block/zram/zram_drv.c | 53 +++++++++++++----------------------
drivers/block/zram/zram_drv.h | 4 ---
2 files changed, 20 insertions(+), 37 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 6f6f6a3fee0e..d374c0f46681 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -55,6 +55,8 @@ static size_t huge_class_size;
static const struct block_device_operations zram_devops;
static const struct block_device_operations zram_wb_devops;
+static struct lock_class_key zram_open_lock_key;
+
static void zram_free_page(struct zram *zram, size_t index);
static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
u32 index, int offset, struct bio *bio);
@@ -1782,44 +1784,21 @@ static ssize_t reset_store(struct device *dev,
bdev = zram->disk->part0;
mutex_lock(&bdev->bd_disk->open_mutex);
- /* Do not reset an active device or claimed device */
- if (bdev->bd_openers || zram->claim) {
+ /* Do not reset an active device */
+ if (bdev->bd_openers) {
mutex_unlock(&bdev->bd_disk->open_mutex);
return -EBUSY;
}
- /* From now on, anyone can't open /dev/zram[0-9] */
- zram->claim = true;
- mutex_unlock(&bdev->bd_disk->open_mutex);
-
/* Make sure all the pending I/O are finished */
sync_blockdev(bdev);
zram_reset_device(zram);
-
- mutex_lock(&bdev->bd_disk->open_mutex);
- zram->claim = false;
mutex_unlock(&bdev->bd_disk->open_mutex);
return len;
}
-static int zram_open(struct block_device *bdev, fmode_t mode)
-{
- int ret = 0;
- struct zram *zram;
-
- WARN_ON(!mutex_is_locked(&bdev->bd_disk->open_mutex));
-
- zram = bdev->bd_disk->private_data;
- /* zram was claimed to reset so open request fails */
- if (zram->claim)
- ret = -EBUSY;
-
- return ret;
-}
-
static const struct block_device_operations zram_devops = {
- .open = zram_open,
.submit_bio = zram_submit_bio,
.swap_slot_free_notify = zram_slot_free_notify,
.rw_page = zram_rw_page,
@@ -1827,7 +1806,6 @@ static const struct block_device_operations zram_devops = {
};
static const struct block_device_operations zram_wb_devops = {
- .open = zram_open,
.submit_bio = zram_submit_bio,
.swap_slot_free_notify = zram_slot_free_notify,
.owner = THIS_MODULE
@@ -1922,6 +1900,13 @@ static int zram_add(void)
zram->disk->private_data = zram;
snprintf(zram->disk->disk_name, 16, "zram%d", device_id);
+ /*
+ * avoid false warning on disk->open_mutex because we hold
+ * init_lock before acquiring backing disk's open_mutex, see
+ * backing_dev_store()
+ */
+ lockdep_set_class(&zram->disk->open_mutex, &zram_open_lock_key);
+
/* Actual capacity set using syfs (/sys/block/zram<id>/disksize */
set_capacity(zram->disk, 0);
/* zram devices sort of resembles non-rotational disks */
@@ -1973,19 +1958,17 @@ static int zram_remove(struct zram *zram)
struct block_device *bdev = zram->disk->part0;
mutex_lock(&bdev->bd_disk->open_mutex);
- if (bdev->bd_openers || zram->claim) {
+ if (bdev->bd_openers) {
mutex_unlock(&bdev->bd_disk->open_mutex);
return -EBUSY;
}
- zram->claim = true;
- mutex_unlock(&bdev->bd_disk->open_mutex);
-
- zram_debugfs_unregister(zram);
-
/* Make sure all the pending I/O are finished */
sync_blockdev(bdev);
zram_reset_device(zram);
+ mutex_unlock(&bdev->bd_disk->open_mutex);
+
+ zram_debugfs_unregister(zram);
pr_info("Removed device: %s\n", zram->disk->disk_name);
@@ -2066,7 +2049,11 @@ static struct class zram_control_class = {
static int zram_remove_cb(int id, void *ptr, void *data)
{
- zram_remove(ptr);
+ /*
+ * No one should own the device during module_exit, otherwise zram
+ * module refcount has to be handled wrong by block layer.
+ */
+ WARN_ON_ONCE(zram_remove(ptr));
return 0;
}
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 80c3b43b4828..ad8564c8a52a 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -109,10 +109,6 @@ struct zram {
*/
u64 disksize; /* bytes */
char compressor[CRYPTO_MAX_ALG_NAME];
- /*
- * zram is claimed so open request will be failed
- */
- bool claim; /* Protected by disk->open_mutex */
#ifdef CONFIG_ZRAM_WRITEBACK
struct file *backing_dev;
spinlock_t wb_limit_lock;
--
2.31.1
