Re: [PATCH V2 2/6] null_blk: give the file-scope mutex a descriptive name

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

On 7/7/26 15:24, Zizhi Wo wrote:
> 
> 
> 在 2026/7/7 12:16, Damien Le Moal 写道:
>> On 7/7/26 11:55, Zizhi Wo wrote:
>>> From: Zizhi Wo <wozizhi@huawei.com>
>>>
>>> The file-scope lock mutex serializes access to global null_blk state,
>>> including the nullb_list and device creation/removal. Rename it to
>>> "nullb_global_lock" to make its purpose clear. No functional change.
>>>
>>> Suggested-by: Bart Van Assche <bvanassche@acm.org>
>>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>>> ---
>>>   drivers/block/null_blk/main.c | 32 ++++++++++++++++----------------
>>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>> index eba204b27785..98f6935bb502 100644
>>> --- a/drivers/block/null_blk/main.c
>>> +++ b/drivers/block/null_blk/main.c
>>> @@ -66,7 +66,7 @@ struct nullb_page {
>>>   #define NULLB_PAGE_FREE (MAP_SZ - 2)
>>>   
>>>   static LIST_HEAD(nullb_list);
>>> -static DEFINE_MUTEX(lock);
>>> +static DEFINE_MUTEX(nullb_global_lock);
>>
>> Since this seem to protect only the device list, why not simply call this
>> nullb_list_lock ?
>>
>>>   static int null_major;
>>>   static DEFINE_IDA(nullb_indexes);
>>>   static struct blk_mq_tag_set tag_set;
>>> @@ -423,9 +423,9 @@ static int nullb_apply_submit_queues(struct nullb_device *dev,
>>>   {
>>>   	int ret;
>>>   
>>> -	mutex_lock(&lock);
>>> +	mutex_lock(&nullb_global_lock);
>>>   	ret = nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues);
>>> -	mutex_unlock(&lock);
>>> +	mutex_unlock(&nullb_global_lock);
>>
>> Nothing in nullb_update_nr_hw_queues() touches the device list. So it is very
>> odd that nullb_global_lock is used for serialization here instead of a
>> nullb_device mutex. If you change this, why not a prep patch to introduce such a
>> mutex ?
>>
> 
> Thanks for pointing this out. The rename really belongs together with
> the locking rework, so I'll drop this patch from v3 and fold the rename
> into a separate series that splits the locking (introducing a per-
> nullb_device mutex for the updates).

Please carefully check if the use of the global lock in these function is not to
serialize with power on/off. The code is a little (uselessly) complicated in
that area and I may not be seeing something. But it seems that on/off control
could also use a device lock, with proper ordering: device lock first, then list
lock.


-- 
Damien Le Moal
Western Digital Research