On 7/23/25 6:11 AM, Yu Kuai wrote:
> Hi,
>
> 在 2025/07/22 18:19, Nilay Shroff 写道:
>>
>>
>> On 7/22/25 2:54 PM, Yu Kuai wrote:
>>>>>>> So why do we need this ? Isn't your patch 1/2 enough to fix the crash you got ?
>>>>>>>
>>>>>> I think you were right — the first patch alone is sufficient to prevent the
>>>>>> crash.
>>>>>> Initially, I thought the second patch might also be necessary, especially for
>>>>>> the
>>>>>> scenario where the user increases the number of submit_queues for a null_blk
>>>>>> device.
>>>>>> While the block layer does create a new hardware queue (hctx) in response to
>>>>>> such
>>>>>> an update, null_blk (null_map_queues()) does not map any software queue (ctx)
>>>>>> to it.
>>>>>> As a result, the newly added hctx remains unused for I/O.
>>>>>>
>>>>>> Given this behavior, I believe we should not allow users to update submit_queues
>>>>>> on a null_blk device if it is using a shared tagset. Currently, the code allows
>>>>>> this update, but it’s misleading since the change has no actual effect.
>>>>>>
>>>>>> Would it make sense to explicitly prevent updating submit_queues in this case?
>>>>>> That would align the interface with the actual behavior and avoid potential
>>>>>> confusion and also saves us allocating memory for hctx which remains unused.
>>>>>> Maybe we should have this check, in nullb_update_nr_hw_queues():
>>>>>>
>>>>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>>>>> index fbae0427263d..aed2a6904db1 100644
>>>>>> --- a/drivers/block/null_blk/main.c
>>>>>> +++ b/drivers/block/null_blk/main.c
>>>>>> @@ -388,6 +388,12 @@ static int nullb_update_nr_hw_queues(struct nullb_device
>>>>>> *dev,
>>>>>> if (!submit_queues)
>>>>>> return -EINVAL;
>>>>>> + /*
>>>>>> + * Cannot update queues with shared tagset.
>>>>>> + */
>>>>>> + if (dev->shared_tags)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> /*
>>>>>> * Make sure that null_init_hctx() does not access nullb->queues[] past
>>>>>> * the end of that array.
>>>>>>
>>>>>> If the above change looks good, maybe I can update second patch with
>>>>>> the above change.
>>>>>
>>>>> I'm good with this change, howerver, with a quick look it seems lots of
>>>>> configfs api are broken for shared_tags. For example:
>>>>>
>>>>> If we switch shared_tags from 0 to 1, and then read submit_queues,
>>>>> looks like it's the old dev->submit_queues instead of g_submit_queues;
>>>>
>>>> g_submit_queues is used as the initial value for dev->submit_queues. See
>>>> nullb_alloc_dev(). So if we prevent changing dev->submit_queues with configfs,
>>>> we'll get whatever g_submit_queues was set on modprobe for the shared tagset.
>>>
>>> I know that, I mean in the case shared_tags is 0 and set submit_queues
>>> different from g_submit_queues, and then *switch shared_tags from 0 to
>>> 1*, user will read wrong submit_queues :)
>>>
>> I think I got what you were referring here. So in addition to the above change
>> we also need to validate the nullb config before we power on nullb device. And
>> we can add that in null_validate_conf():
>>
>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>> index aa163ae9b2aa..1762dc541a17 100644
>> --- a/drivers/block/null_blk/main.c
>> +++ b/drivers/block/null_blk/main.c
>> @@ -1893,6 +1893,10 @@ static int null_validate_conf(struct nullb_device *dev)
>> dev->submit_queues = 1;
>> dev->prev_submit_queues = dev->submit_queues;
>> + if (dev->shared_tags)
>> + if (dev->submit_queues > g_submit_queues)
>> + dev->submit_queues = g_submit_queues;
>
> Perhaps:
>
> if (dev->shared_tags) {
> dev->submit_queues = g_submit_queues;
> dev->poll_queues = g_poll_queues;
> } else if (dev->poll_queues > g_poll_queues) {
> dev->poll_queues = g_poll_queues;
> }
>
Okay this makes sense. So for the shared tagset, we'd
always reset the submit and poll queues value to the
respective values set while loading the module. And for
the non-shared tagset, we'd clamp poll_queues when its
value exceeds the value initialized during module load
time. I will incorporate this in the next patch.
Thanks,
--Nilay
