On Tue, Apr 15, 2025 at 07:17:09PM -0600, Jens Axboe wrote:
> On 4/15/25 7:13 PM, Ming Lei wrote:
> > On Mon, Apr 14, 2025 at 02:39:33PM -0600, Jens Axboe wrote:
> >> On 4/14/25 1:58 PM, Uday Shankar wrote:
> >>> +static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_device *ub,
> >>> + struct ublk_queue *ubq, struct ublk_io *io,
> >>> + const struct ublksrv_io_cmd *ub_cmd,
> >>> + unsigned int issue_flags)
> >>> +{
> >>> + int ret = 0;
> >>> +
> >>> + if (issue_flags & IO_URING_F_NONBLOCK)
> >>> + return -EAGAIN;
> >>> +
> >>> + mutex_lock(&ub->mutex);
> >>
> >> This looks like overkill, if we can trylock the mutex that should surely
> >> be fine? And I would imagine succeed most of the time, hence making the
> >> inline/fastpath fine with F_NONBLOCK?
> >
> > The mutex is the innermost lock and it won't block for handling FETCH
> > command, which is just called during queue setting up stage, so I think
> > trylock isn't necessary, but also brings complexity.
>
> Then the NONBLOCK check can go away, and a comment added instead on why
> it's fine. Or maybe even a WARN_ON_ONCE() if trylock fails or something.
> Otherwise it's going to look like a code bug.
Yes, the NONBLOCK check isn't needed.
ublk uring cmd is always handled with !(issue_flags & IO_URING_F_UNLOCKED), please
see ublk_ch_uring_cmd() and ublk_ch_uring_cmd_local().
thanks,
Ming
