[PATCH v2 1/1] zloop: ensure atomicity of state checks in zloop_queue_rq

[PATCH v2 1/1] zloop: ensure atomicity of state checks in zloop_queue_rq

[Yongpeng Yang]

From: Yongpeng Yang <yangyongpeng@xiaomi.com>

The state check of zlo->state in zloop_queue_rq() fails to guarantee
atomicity. This patch changes zlo->state to an atomic_t type variable,
avoiding the use of the zloop_ctl_mutex lock for protection.

Signed-off-by: Yongpeng Yang <yangyongpeng@xiaomi.com>
---
v2:
- Change zlo->state to atomic_t type instead of using zloop_ctl_mutex
for protection.
---
 drivers/block/zloop.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
index 77bd6081b244..6eb4bbab374c 100644
--- a/drivers/block/zloop.c
+++ b/drivers/block/zloop.c
@@ -110,7 +110,7 @@ struct zloop_zone {
 
 struct zloop_device {
 	unsigned int		id;
-	unsigned int		state;
+	atomic_t		state;
 
 	struct blk_mq_tag_set	tag_set;
 	struct gendisk		*disk;
@@ -146,6 +146,16 @@ struct zloop_cmd {
 static DEFINE_IDR(zloop_index_idr);
 static DEFINE_MUTEX(zloop_ctl_mutex);
 
+static inline int zloop_get_state(struct zloop_device *zlo)
+{
+	return atomic_read(&zlo->state);
+}
+
+static inline void zloop_set_state(struct zloop_device *zlo, int state)
+{
+	atomic_set(&zlo->state, state);
+}
+
 static unsigned int rq_zone_no(struct request *rq)
 {
 	struct zloop_device *zlo = rq->q->queuedata;
@@ -697,7 +707,7 @@ static blk_status_t zloop_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct zloop_cmd *cmd = blk_mq_rq_to_pdu(rq);
 	struct zloop_device *zlo = rq->q->queuedata;
 
-	if (zlo->state == Zlo_deleting)
+	if (zloop_get_state(zlo) == Zlo_deleting)
 		return BLK_STS_IOERR;
 
 	/*
@@ -726,16 +736,10 @@ static const struct blk_mq_ops zloop_mq_ops = {
 static int zloop_open(struct gendisk *disk, blk_mode_t mode)
 {
 	struct zloop_device *zlo = disk->private_data;
-	int ret;
 
-	ret = mutex_lock_killable(&zloop_ctl_mutex);
-	if (ret)
-		return ret;
-
-	if (zlo->state != Zlo_live)
-		ret = -ENXIO;
-	mutex_unlock(&zloop_ctl_mutex);
-	return ret;
+	if (zloop_get_state(zlo) != Zlo_live)
+		return -ENXIO;
+	return 0;
 }
 
 static int zloop_report_zones(struct gendisk *disk, sector_t sector,
@@ -1002,7 +1006,7 @@ static int zloop_ctl_add(struct zloop_options *opts)
 		ret = -ENOMEM;
 		goto out;
 	}
-	zlo->state = Zlo_creating;
+	zloop_set_state(zlo, Zlo_creating);
 
 	ret = mutex_lock_killable(&zloop_ctl_mutex);
 	if (ret)
@@ -1112,9 +1116,7 @@ static int zloop_ctl_add(struct zloop_options *opts)
 		goto out_cleanup_disk;
 	}
 
-	mutex_lock(&zloop_ctl_mutex);
-	zlo->state = Zlo_live;
-	mutex_unlock(&zloop_ctl_mutex);
+	zloop_set_state(zlo, Zlo_live);
 
 	pr_info("zloop: device %d, %u zones of %llu MiB, %u B block size\n",
 		zlo->id, zlo->nr_zones,
@@ -1171,13 +1173,14 @@ static int zloop_ctl_remove(struct zloop_options *opts)
 		return ret;
 
 	zlo = idr_find(&zloop_index_idr, opts->id);
-	if (!zlo || zlo->state == Zlo_creating) {
+	if (!zlo || zloop_get_state(zlo) == Zlo_creating) {
 		ret = -ENODEV;
-	} else if (zlo->state == Zlo_deleting) {
+	} else if (zloop_get_state(zlo) == Zlo_deleting) {
 		ret = -EINVAL;
 	} else {
 		idr_remove(&zloop_index_idr, zlo->id);
-		zlo->state = Zlo_deleting;
+		zloop_set_state(zlo, Zlo_deleting);
+
 	}
 
 	mutex_unlock(&zloop_ctl_mutex);
-- 
2.43.0

Re: [PATCH v2 1/1] zloop: ensure atomicity of state checks in zloop_queue_rq

[Christoph Hellwig]

On Sat, Dec 13, 2025 at 09:45:46PM +0800, Yongpeng Yang wrote:
> From: Yongpeng Yang <yangyongpeng@xiaomi.com>
> 
> The state check of zlo->state in zloop_queue_rq() fails to guarantee
> atomicity. This patch changes zlo->state to an atomic_t type variable,
> avoiding the use of the zloop_ctl_mutex lock for protection.

Err, no.  atomic_t for states is rarely a good idea.  And unless I'm
missing something this state check is just an optimization, so a
data_race() should be enough.  Either way this should probably first
be done and discussed for the loop code that has a lot more reviewers
and then ported to zloop.

Re: [PATCH v2 1/1] zloop: ensure atomicity of state checks in zloop_queue_rq

[Yongpeng Yang]

On 12/15/25 13:31, Christoph Hellwig wrote:
> On Sat, Dec 13, 2025 at 09:45:46PM +0800, Yongpeng Yang wrote:
>> From: Yongpeng Yang <yangyongpeng@xiaomi.com>
>>
>> The state check of zlo->state in zloop_queue_rq() fails to guarantee
>> atomicity. This patch changes zlo->state to an atomic_t type variable,
>> avoiding the use of the zloop_ctl_mutex lock for protection.
> 
> Err, no.  atomic_t for states is rarely a good idea.  And unless I'm
> missing something this state check is just an optimization, so a
> data_race() should be enough.  Either way this should probably first
> be done and discussed for the loop code that has a lot more reviewers
> and then ported to zloop.
> 

Got it. I’ll send out the patch shortly.

Thanks
Yongpeng,

Re: [PATCH v2 1/1] zloop: ensure atomicity of state checks in zloop_queue_rq

[Damien Le Moal]

On 12/15/25 14:31, Christoph Hellwig wrote:
> On Sat, Dec 13, 2025 at 09:45:46PM +0800, Yongpeng Yang wrote:
>> From: Yongpeng Yang <yangyongpeng@xiaomi.com>
>>
>> The state check of zlo->state in zloop_queue_rq() fails to guarantee
>> atomicity. This patch changes zlo->state to an atomic_t type variable,
>> avoiding the use of the zloop_ctl_mutex lock for protection.
> 
> Err, no.  atomic_t for states is rarely a good idea.  And unless I'm
> missing something this state check is just an optimization, so a
> data_race() should be enough.  Either way this should probably first
> be done and discussed for the loop code that has a lot more reviewers
> and then ported to zloop.

Moving the check to zloop_cmd_workfn() or zloop_handle_cmd() and simply taking
the mutex is another simple solution.

Dropping completely the check is also OK I think since the device will never go
away under us as long as there are in-flight BIOs anyway. And this is only an
optimization to speedup device teardown.


-- 
Damien Le Moal
Western Digital Research