Re: [PATCH 1/1] loop: convert lo_state to atomic_t type to ensure atomic state checks in queue_rq path

[Damien Le Moal <dlemoal@kernel.org>]

On 12/15/25 16:34, Yongpeng Yang wrote:
> 
> On 12/15/25 15:12, Damien Le Moal wrote:
>> 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).
> 
> Yes, I hadn’t considered that before.
> 
>>
>> As Christoph suggested, using "data_race()" may be enough to silence code
>> checkers. Or use READ_ONCE() WRITE_ONCE() for the state.
> 
> Considering the earlier point that the queue_rq check of lo->lo_state is
> just an optimization, using READ_ONCE() seems more appropriate. As noted
> in the comment for data_race(), for accesses without locking, would
> data_race(READ_ONCE(lo->lo_state)) make more sense here?

Yes, I think it is OK.


-- 
Damien Le Moal
Western Digital Research