On 3/12/25 7:42 PM, Christoph Hellwig wrote:
> On Wed, Mar 12, 2025 at 03:58:38PM +0530, Nilay Shroff wrote:
>> Additionally, debugfs attribute "busy" is currently unprotected. This
>> attribute iterates over all started requests in a tagset and prints them.
>> However, the tags can be updated simultaneously via the sysfs attribute
>> "nr_requests" or "scheduler" (elevator switch), leading to potential race
>> conditions. Since the sysfs attributes "nr_requests" and "scheduler" are
>> already protected using q->elevator_lock, extend this protection to the
>> debugfs "busy" attribute as well.
>
> I'd split that into a separate patch for bisectability.
Ok I will split it.
>
>> struct show_busy_params params = { .m = m, .hctx = hctx };
>> + int res;
>>
>> + res = mutex_lock_interruptible(&hctx->queue->elevator_lock);
>> + if (res)
>> + goto out;
>
> Is mutex_lock_interruptible really the right primitive here? We don't
> really want this read system call interrupted by random signals, do
> we? I guess this should be mutex_lock_killable.
>
> (and the same for the existing methods this is copy and pasted from).
>
I thought we wanted to interrupt using SIGINT (CTRL+C) in case user opens
this file using cat. Maybe that's more convenient than sending SIGKILL.
And yes I used mutex_lock_interruptible because for other attributes we are
already using it. But if mutex_lock_killable is preferred then I'd change it
for all methods.
>> blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy_rq,
>> ¶ms);
>> -
>> - return 0;
>> + mutex_unlock(&hctx->queue->elevator_lock);
>> +out:
>> + return res;
>
> And as Damien already said, no need for the labels here, including for
> the existing code. That should probably be alsot changed in an extra
> patch for the existing code while you're at it.
>
Okay will do in next patch.
Thanks,
--Nilay
