[PATCH 2/5] memstick/ms_block: simplify refcounting
Implement the ->free_disk method to free the msb_data structure only once
the last gendisk reference goes away instead of keeping a local refcount.
Signed-off-by: Christoph Hellwig
---
drivers/memstick/core/ms_block.c | 64
drivers/memstick/core/ms_block.h | 1 -
2 files changed, 15 insertions(+), 50 deletions(-)
diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c
index 0cda6c6baefc3..3993bdd4b519c 100644
--- a/drivers/memstick/core/ms_block.c
+++ b/drivers/memstick/core/ms_block.c
@@ -1943,22 +1943,6 @@ static void msb_io_work(struct work_struct *work)
static DEFINE_IDR(msb_disk_idr); /*set of used disk numbers */
static DEFINE_MUTEX(msb_disk_lock); /* protects against races in open/release
*/
-static int msb_bd_open(struct block_device *bdev, fmode_t mode)
-{
- struct gendisk *disk = bdev->bd_disk;
- struct msb_data *msb = disk->private_data;
-
- dbg_verbose("block device open");
-
- mutex_lock(_disk_lock);
-
- if (msb && msb->card)
- msb->usage_count++;
-
- mutex_unlock(_disk_lock);
- return 0;
-}
-
static void msb_data_clear(struct msb_data *msb)
{
kfree(msb->boot_page);
@@ -1968,33 +1952,6 @@ static void msb_data_clear(struct msb_data *msb)
msb->card = NULL;
}
-static int msb_disk_release(struct gendisk *disk)
-{
- struct msb_data *msb = disk->private_data;
-
- dbg_verbose("block device release");
- mutex_lock(_disk_lock);
-
- if (msb) {
- if (msb->usage_count)
- msb->usage_count--;
-
- if (!msb->usage_count) {
- disk->private_data = NULL;
- idr_remove(_disk_idr, msb->disk_id);
- put_disk(disk);
- kfree(msb);
- }
- }
- mutex_unlock(_disk_lock);
- return 0;
-}
-
-static void msb_bd_release(struct gendisk *disk, fmode_t mode)
-{
- msb_disk_release(disk);
-}
-
static int msb_bd_getgeo(struct block_device *bdev,
struct hd_geometry *geo)
{
@@ -2003,6 +1960,17 @@ static int msb_bd_getgeo(struct block_device *bdev,
return 0;
}
+static void msb_bd_free_disk(struct gendisk *disk)
+{
+ struct msb_data *msb = disk->private_data;
+
+ mutex_lock(_disk_lock);
+ idr_remove(_disk_idr, msb->disk_id);
+ mutex_unlock(_disk_lock);
+
+ kfree(msb);
+}
+
static blk_status_t msb_queue_rq(struct blk_mq_hw_ctx *hctx,
const struct blk_mq_queue_data *bd)
{
@@ -2096,10 +2064,9 @@ static void msb_start(struct memstick_dev *card)
}
static const struct block_device_operations msb_bdops = {
- .open= msb_bd_open,
- .release = msb_bd_release,
- .getgeo = msb_bd_getgeo,
- .owner = THIS_MODULE
+ .owner = THIS_MODULE,
+ .getgeo = msb_bd_getgeo,
+ .free_disk = msb_bd_free_disk,
};
static const struct blk_mq_ops msb_mq_ops = {
@@ -2147,7 +2114,6 @@ static int msb_init_disk(struct memstick_dev *card)
set_capacity(msb->disk, capacity);
dbg("Set total disk size to %lu sectors", capacity);
- msb->usage_count = 1;
msb->io_queue = alloc_ordered_workqueue("ms_block", WQ_MEM_RECLAIM);
INIT_WORK(>io_work, msb_io_work);
sg_init_table(msb->prealloc_sg, MS_BLOCK_MAX_SEGS+1);
@@ -2229,7 +2195,7 @@ static void msb_remove(struct memstick_dev *card)
msb_data_clear(msb);
mutex_unlock(_disk_lock);
- msb_disk_release(msb->disk);
+ put_disk(msb->disk);
memstick_set_drvdata(card, NULL);
}
diff --git a/drivers/memstick/core/ms_block.h b/drivers/memstick/core/ms_block.h
index 122e1a8a8bd5b..7058f9aefeb92 100644
--- a/drivers/memstick/core/ms_block.h
+++ b/drivers/memstick/core/ms_block.h
@@ -143,7 +143,6 @@ struct ms_boot_page {
} __packed;
struct msb_data {
- unsigned intusage_count;
struct memstick_dev *card;
struct gendisk *disk;
struct request_queue*queue;
--
2.30.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/5] memstick/ms_block: simplify refcounting
On Wed, Feb 09, 2022 at 09:21:17AM +0100, Christoph Hellwig wrote: > Implement the ->free_disk method to free the msb_data structure only once > the last gendisk reference goes away instead of keeping a local refcount. > The approach looks good, just the error handling needs to be careful, such as, once driver data is bound to disk->private_data, the previous error handling code shouldn't touch/free the driver data any more. That said assigning disk->private_data implies driver data ownership transfer after this conversion. Such as, in msb_init_disk(), once blk_cleanup_disk() is done, the code branch of out_release_id shouldn't be run; msb_probe() has the similar issue too. Thanks, Ming ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/5] memstick/ms_block: simplify refcounting
Implement the ->free_disk method to free the msb_data structure only once
the last gendisk reference goes away instead of keeping a local refcount.
Signed-off-by: Christoph Hellwig
---
drivers/memstick/core/ms_block.c | 64
drivers/memstick/core/ms_block.h | 1 -
2 files changed, 15 insertions(+), 50 deletions(-)
diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c
index 0cda6c6baefc3..3993bdd4b519c 100644
--- a/drivers/memstick/core/ms_block.c
+++ b/drivers/memstick/core/ms_block.c
@@ -1943,22 +1943,6 @@ static void msb_io_work(struct work_struct *work)
static DEFINE_IDR(msb_disk_idr); /*set of used disk numbers */
static DEFINE_MUTEX(msb_disk_lock); /* protects against races in open/release
*/
-static int msb_bd_open(struct block_device *bdev, fmode_t mode)
-{
- struct gendisk *disk = bdev->bd_disk;
- struct msb_data *msb = disk->private_data;
-
- dbg_verbose("block device open");
-
- mutex_lock(_disk_lock);
-
- if (msb && msb->card)
- msb->usage_count++;
-
- mutex_unlock(_disk_lock);
- return 0;
-}
-
static void msb_data_clear(struct msb_data *msb)
{
kfree(msb->boot_page);
@@ -1968,33 +1952,6 @@ static void msb_data_clear(struct msb_data *msb)
msb->card = NULL;
}
-static int msb_disk_release(struct gendisk *disk)
-{
- struct msb_data *msb = disk->private_data;
-
- dbg_verbose("block device release");
- mutex_lock(_disk_lock);
-
- if (msb) {
- if (msb->usage_count)
- msb->usage_count--;
-
- if (!msb->usage_count) {
- disk->private_data = NULL;
- idr_remove(_disk_idr, msb->disk_id);
- put_disk(disk);
- kfree(msb);
- }
- }
- mutex_unlock(_disk_lock);
- return 0;
-}
-
-static void msb_bd_release(struct gendisk *disk, fmode_t mode)
-{
- msb_disk_release(disk);
-}
-
static int msb_bd_getgeo(struct block_device *bdev,
struct hd_geometry *geo)
{
@@ -2003,6 +1960,17 @@ static int msb_bd_getgeo(struct block_device *bdev,
return 0;
}
+static void msb_bd_free_disk(struct gendisk *disk)
+{
+ struct msb_data *msb = disk->private_data;
+
+ mutex_lock(_disk_lock);
+ idr_remove(_disk_idr, msb->disk_id);
+ mutex_unlock(_disk_lock);
+
+ kfree(msb);
+}
+
static blk_status_t msb_queue_rq(struct blk_mq_hw_ctx *hctx,
const struct blk_mq_queue_data *bd)
{
@@ -2096,10 +2064,9 @@ static void msb_start(struct memstick_dev *card)
}
static const struct block_device_operations msb_bdops = {
- .open= msb_bd_open,
- .release = msb_bd_release,
- .getgeo = msb_bd_getgeo,
- .owner = THIS_MODULE
+ .owner = THIS_MODULE,
+ .getgeo = msb_bd_getgeo,
+ .free_disk = msb_bd_free_disk,
};
static const struct blk_mq_ops msb_mq_ops = {
@@ -2147,7 +2114,6 @@ static int msb_init_disk(struct memstick_dev *card)
set_capacity(msb->disk, capacity);
dbg("Set total disk size to %lu sectors", capacity);
- msb->usage_count = 1;
msb->io_queue = alloc_ordered_workqueue("ms_block", WQ_MEM_RECLAIM);
INIT_WORK(>io_work, msb_io_work);
sg_init_table(msb->prealloc_sg, MS_BLOCK_MAX_SEGS+1);
@@ -2229,7 +2195,7 @@ static void msb_remove(struct memstick_dev *card)
msb_data_clear(msb);
mutex_unlock(_disk_lock);
- msb_disk_release(msb->disk);
+ put_disk(msb->disk);
memstick_set_drvdata(card, NULL);
}
diff --git a/drivers/memstick/core/ms_block.h b/drivers/memstick/core/ms_block.h
index 122e1a8a8bd5b..7058f9aefeb92 100644
--- a/drivers/memstick/core/ms_block.h
+++ b/drivers/memstick/core/ms_block.h
@@ -143,7 +143,6 @@ struct ms_boot_page {
} __packed;
struct msb_data {
- unsigned intusage_count;
struct memstick_dev *card;
struct gendisk *disk;
struct request_queue*queue;
--
2.30.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
