From: Damien Le Moal <dlemoal@kernel.org>
To: Yongpeng Yang <yangyongpeng.storage@gmail.com>,
Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
Ming Lei <ming.lei@redhat.com>
Cc: linux-block@vger.kernel.org,
Yongpeng Yang <yangyongpeng@xiaomi.com>,
Yongpeng Yang <yangyongpeng.storage@outlook.com>
Subject: Re: [PATCH 1/1] loop: convert lo_state to atomic_t type to ensure atomic state checks in queue_rq path
Date: Mon, 15 Dec 2025 16:12:54 +0900 [thread overview]
Message-ID: <66ae3b25-7d82-445c-b125-bc017d299c85@kernel.org> (raw)
In-Reply-To: <20251215065458.1452317-2-yangyongpeng.storage@gmail.com>
On 12/15/25 15:54, Yongpeng Yang wrote:
> From: Yongpeng Yang <yangyongpeng@xiaomi.com>
>
> lo_state is currently defined as an int, which does not guarantee
> atomicity for state checks. In the queue_rq path, ensuring correct state
> checks requires holding lo->lo_mutex, which may increase I/O submission
> latency. This patch converts lo_state to atomic_t type. The main changes
> are:
> 1. Updates to lo_state still require holding lo->lo_mutex, since the
> state must be validated before modification, and the lock ensures that
> no concurrent operation can change the state.
> 2. Read-only accesses to lo_state no longer require holding lo->lo_mutex.
>
> This allows atomic state checks in the queue_rq fast path while avoiding
> unnecessary locking overhead.
Code like:
if (loop_device_get_state(lo) != Lo_bound)
is absolutely *not* atomic, since the state can change in between the atomic
read and the comparison instruction. So this is not about atomicity, it is about
not reading garbage from the state field if there is a load and a store
concurrently executed on different CPUs (that happening depends on the CPU
architecture though).
As Christoph suggested, using "data_race()" may be enough to silence code
checkers. Or use READ_ONCE() WRITE_ONCE() for the state.
>
> Signed-off-by: Yongpeng Yang <yangyongpeng@xiaomi.com>
> ---
> drivers/block/loop.c | 67 ++++++++++++++++++++++++--------------------
> 1 file changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 272bc608e528..bc661ecb449a 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -59,7 +59,7 @@ struct loop_device {
> gfp_t old_gfp_mask;
>
> spinlock_t lo_lock;
> - int lo_state;
> + atomic_t lo_state;
> spinlock_t lo_work_lock;
> struct workqueue_struct *workqueue;
> struct work_struct rootcg_work;
> @@ -94,6 +94,16 @@ static DEFINE_IDR(loop_index_idr);
> static DEFINE_MUTEX(loop_ctl_mutex);
> static DEFINE_MUTEX(loop_validate_mutex);
>
> +static inline int loop_device_get_state(struct loop_device *lo)
> +{
> + return atomic_read(&lo->lo_state);
> +}
> +
> +static inline void loop_device_set_state(struct loop_device *lo, int state)
> +{
> + atomic_set(&lo->lo_state, state);
> +}
> +
> /**
> * loop_global_lock_killable() - take locks for safe loop_validate_file() test
> *
> @@ -200,7 +210,7 @@ static bool lo_can_use_dio(struct loop_device *lo)
> static inline void loop_update_dio(struct loop_device *lo)
> {
> lockdep_assert_held(&lo->lo_mutex);
> - WARN_ON_ONCE(lo->lo_state == Lo_bound &&
> + WARN_ON_ONCE(loop_device_get_state(lo) == Lo_bound &&
> lo->lo_queue->mq_freeze_depth == 0);
>
> if ((lo->lo_flags & LO_FLAGS_DIRECT_IO) && !lo_can_use_dio(lo))
> @@ -495,7 +505,7 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
> return -EBADF;
>
> l = I_BDEV(f->f_mapping->host)->bd_disk->private_data;
> - if (l->lo_state != Lo_bound)
> + if (loop_device_get_state(l) != Lo_bound)
> return -EINVAL;
> /* Order wrt setting lo->lo_backing_file in loop_configure(). */
> rmb();
> @@ -563,7 +573,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> if (error)
> goto out_putf;
> error = -ENXIO;
> - if (lo->lo_state != Lo_bound)
> + if (loop_device_get_state(lo) != Lo_bound)
> goto out_err;
>
> /* the loop device has to be read-only */
> @@ -1019,7 +1029,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> goto out_bdev;
>
> error = -EBUSY;
> - if (lo->lo_state != Lo_unbound)
> + if (loop_device_get_state(lo) != Lo_unbound)
> goto out_unlock;
>
> error = loop_validate_file(file, bdev);
> @@ -1082,7 +1092,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> /* Order wrt reading lo_state in loop_validate_file(). */
> wmb();
>
> - lo->lo_state = Lo_bound;
> + loop_device_set_state(lo, Lo_bound);
> if (part_shift)
> lo->lo_flags |= LO_FLAGS_PARTSCAN;
> partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> @@ -1179,7 +1189,7 @@ static void __loop_clr_fd(struct loop_device *lo)
> if (!part_shift)
> set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
> mutex_lock(&lo->lo_mutex);
> - lo->lo_state = Lo_unbound;
> + loop_device_set_state(lo, Lo_unbound);
> mutex_unlock(&lo->lo_mutex);
>
> /*
> @@ -1206,7 +1216,7 @@ static int loop_clr_fd(struct loop_device *lo)
> err = loop_global_lock_killable(lo, true);
> if (err)
> return err;
> - if (lo->lo_state != Lo_bound) {
> + if (loop_device_get_state(lo) != Lo_bound) {
> loop_global_unlock(lo, true);
> return -ENXIO;
> }
> @@ -1218,7 +1228,7 @@ static int loop_clr_fd(struct loop_device *lo)
>
> lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
> if (disk_openers(lo->lo_disk) == 1)
> - lo->lo_state = Lo_rundown;
> + loop_device_set_state(lo, Lo_rundown);
> loop_global_unlock(lo, true);
>
> return 0;
> @@ -1235,7 +1245,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
> err = mutex_lock_killable(&lo->lo_mutex);
> if (err)
> return err;
> - if (lo->lo_state != Lo_bound) {
> + if (loop_device_get_state(lo) != Lo_bound) {
> err = -ENXIO;
> goto out_unlock;
> }
> @@ -1289,7 +1299,7 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info)
> ret = mutex_lock_killable(&lo->lo_mutex);
> if (ret)
> return ret;
> - if (lo->lo_state != Lo_bound) {
> + if (loop_device_get_state(lo) != Lo_bound) {
> mutex_unlock(&lo->lo_mutex);
> return -ENXIO;
> }
> @@ -1408,7 +1418,7 @@ static int loop_set_capacity(struct loop_device *lo)
> {
> loff_t size;
>
> - if (unlikely(lo->lo_state != Lo_bound))
> + if (unlikely(loop_device_get_state(lo) != Lo_bound))
> return -ENXIO;
>
> size = lo_calculate_size(lo, lo->lo_backing_file);
> @@ -1422,7 +1432,7 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
> bool use_dio = !!arg;
> unsigned int memflags;
>
> - if (lo->lo_state != Lo_bound)
> + if (loop_device_get_state(lo) != Lo_bound)
> return -ENXIO;
> if (use_dio == !!(lo->lo_flags & LO_FLAGS_DIRECT_IO))
> return 0;
> @@ -1464,7 +1474,7 @@ static int loop_set_block_size(struct loop_device *lo, blk_mode_t mode,
> if (err)
> goto abort_claim;
>
> - if (lo->lo_state != Lo_bound) {
> + if (loop_device_get_state(lo) != Lo_bound) {
> err = -ENXIO;
> goto unlock;
> }
> @@ -1716,16 +1726,11 @@ static int lo_compat_ioctl(struct block_device *bdev, blk_mode_t mode,
> static int lo_open(struct gendisk *disk, blk_mode_t mode)
> {
> struct loop_device *lo = disk->private_data;
> - int err;
> -
> - err = mutex_lock_killable(&lo->lo_mutex);
> - if (err)
> - return err;
> + int state = loop_device_get_state(lo);
>
> - if (lo->lo_state == Lo_deleting || lo->lo_state == Lo_rundown)
> - err = -ENXIO;
> - mutex_unlock(&lo->lo_mutex);
> - return err;
> + if (state == Lo_deleting || state == Lo_rundown)
> + return -ENXIO;
> + return 0;
> }
>
> static void lo_release(struct gendisk *disk)
> @@ -1742,10 +1747,10 @@ static void lo_release(struct gendisk *disk)
> */
>
> mutex_lock(&lo->lo_mutex);
> - if (lo->lo_state == Lo_bound && (lo->lo_flags & LO_FLAGS_AUTOCLEAR))
> - lo->lo_state = Lo_rundown;
> + if (loop_device_get_state(lo) == Lo_bound && (lo->lo_flags & LO_FLAGS_AUTOCLEAR))
> + loop_device_set_state(lo, Lo_rundown);
>
> - need_clear = (lo->lo_state == Lo_rundown);
> + need_clear = (loop_device_get_state(lo) == Lo_rundown);
> mutex_unlock(&lo->lo_mutex);
>
> if (need_clear)
> @@ -1858,7 +1863,7 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>
> blk_mq_start_request(rq);
>
> - if (lo->lo_state != Lo_bound)
> + if (loop_device_get_state(lo) != Lo_bound)
> return BLK_STS_IOERR;
>
> switch (req_op(rq)) {
> @@ -2016,7 +2021,7 @@ static int loop_add(int i)
> lo->worker_tree = RB_ROOT;
> INIT_LIST_HEAD(&lo->idle_worker_list);
> timer_setup(&lo->timer, loop_free_idle_workers_timer, TIMER_DEFERRABLE);
> - lo->lo_state = Lo_unbound;
> + loop_device_set_state(lo, Lo_unbound);
>
> err = mutex_lock_killable(&loop_ctl_mutex);
> if (err)
> @@ -2168,13 +2173,13 @@ static int loop_control_remove(int idx)
> ret = mutex_lock_killable(&lo->lo_mutex);
> if (ret)
> goto mark_visible;
> - if (lo->lo_state != Lo_unbound || disk_openers(lo->lo_disk) > 0) {
> + if (loop_device_get_state(lo) != Lo_unbound || disk_openers(lo->lo_disk) > 0) {
> mutex_unlock(&lo->lo_mutex);
> ret = -EBUSY;
> goto mark_visible;
> }
> /* Mark this loop device as no more bound, but not quite unbound yet */
> - lo->lo_state = Lo_deleting;
> + loop_device_set_state(lo, Lo_deleting);
> mutex_unlock(&lo->lo_mutex);
>
> loop_remove(lo);
> @@ -2198,7 +2203,7 @@ static int loop_control_get_free(int idx)
> return ret;
> idr_for_each_entry(&loop_index_idr, lo, id) {
> /* Hitting a race results in creating a new loop device which is harmless. */
> - if (lo->idr_visible && data_race(lo->lo_state) == Lo_unbound)
> + if (lo->idr_visible && loop_device_get_state(lo) == Lo_unbound)
> goto found;
> }
> mutex_unlock(&loop_ctl_mutex);
--
Damien Le Moal
Western Digital Research
next prev parent reply other threads:[~2025-12-15 7:12 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-15 6:54 [PATCH 1/1] loop: convert lo_state to atomic_t type to ensure atomic state checks in queue_rq path Yongpeng Yang
2025-12-15 7:12 ` Damien Le Moal [this message]
2025-12-15 7:34 ` Yongpeng Yang
2025-12-15 7:44 ` Damien Le Moal
2025-12-15 8:19 ` Yongpeng Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66ae3b25-7d82-445c-b125-bc017d299c85@kernel.org \
--to=dlemoal@kernel.org \
--cc=axboe@kernel.dk \
--cc=hch@lst.de \
--cc=linux-block@vger.kernel.org \
--cc=ming.lei@redhat.com \
--cc=yangyongpeng.storage@gmail.com \
--cc=yangyongpeng.storage@outlook.com \
--cc=yangyongpeng@xiaomi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox