Re: [PATCH] ublk: check for unprivileged daemon on each I/O fetch

[Jens Axboe <axboe@kernel.dk>]

On 8/8/25 12:03 PM, Caleb Sander Mateos wrote:
> On Fri, Aug 8, 2025 at 2:01?PM Uday Shankar <ushankar@purestorage.com> wrote:
>>
>> On Fri, Aug 08, 2025 at 09:52:15AM -0600, Caleb Sander Mateos wrote:
>>> Commit ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue
>>> daemon") allowed each ublk I/O to have an independent daemon task.
>>> However, nr_privileged_daemon is only computed based on whether the last
>>> I/O fetched in each ublk queue has an unprivileged daemon task.
>>> Fix this by checking whether every fetched I/O's daemon is privileged.
>>> Change nr_privileged_daemon from a count of queues to a boolean
>>> indicating whether any I/Os have an unprivileged daemon.
>>>
>>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
>>> Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
>>
>> Nice catch!
>>
>>> ---
>>>  drivers/block/ublk_drv.c | 16 +++++++---------
>>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>>> index 6561d2a561fa..a035070dd690 100644
>>> --- a/drivers/block/ublk_drv.c
>>> +++ b/drivers/block/ublk_drv.c
>>> @@ -233,11 +233,11 @@ struct ublk_device {
>>>
>>>       struct ublk_params      params;
>>>
>>>       struct completion       completion;
>>>       unsigned int            nr_queues_ready;
>>> -     unsigned int            nr_privileged_daemon;
>>> +     bool                    unprivileged_daemons;
>>>       struct mutex cancel_mutex;
>>>       bool canceling;
>>>       pid_t   ublksrv_tgid;
>>>  };
>>>
>>> @@ -1548,11 +1548,11 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
>>>               ublk_queue_reinit(ub, ublk_get_queue(ub, i));
>>>
>>>       /* set to NULL, otherwise new tasks cannot mmap io_cmd_buf */
>>>       ub->mm = NULL;
>>>       ub->nr_queues_ready = 0;
>>> -     ub->nr_privileged_daemon = 0;
>>> +     ub->unprivileged_daemons = false;
>>>       ub->ublksrv_tgid = -1;
>>>  }
>>>
>>>  static struct gendisk *ublk_get_disk(struct ublk_device *ub)
>>>  {
>>> @@ -1978,16 +1978,14 @@ static void ublk_reset_io_flags(struct ublk_device *ub)
>>>  /* device can only be started after all IOs are ready */
>>>  static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
>>>       __must_hold(&ub->mutex)
>>>  {
>>>       ubq->nr_io_ready++;
>>> -     if (ublk_queue_ready(ubq)) {
>>> +     if (ublk_queue_ready(ubq))
>>>               ub->nr_queues_ready++;
>>> -
>>> -             if (capable(CAP_SYS_ADMIN))
>>> -                     ub->nr_privileged_daemon++;
>>> -     }
>>> +     if (!ub->unprivileged_daemons && !capable(CAP_SYS_ADMIN))
>>> +             ub->unprivileged_daemons = true;
>>
>> Shorter:
>>
>> ub->unprivileged_daemons |= !capable(CAP_SYS_ADMIN);
> 
> I was trying to avoid the capable() call if unprivileged_daemons was
> already set. But maybe that's not a common case and it's not worth
> optimizing?

Definitely worth it, you did the right thing.

-- 
Jens Axboe