diff mbox series

[v2,05/14] ublk: don't hard code IO_URING_F_UNLOCKED

Message ID a3928d3de14d2569efc2edd7fb654a4795ae7f86.1710720150.git.asml.silence@gmail.com (mailing list archive)
State New
Headers show
Series remove aux CQE caches | expand

Commit Message

Pavel Begunkov March 18, 2024, 12:41 a.m. UTC
uring_cmd implementations should not try to guess issue_flags, just use
a newly added io_uring_cmd_complete(). We're loosing an optimisation in
the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
is that we don't care that much about it.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/ublk_drv.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Ming Lei March 18, 2024, 8:16 a.m. UTC | #1
On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
> uring_cmd implementations should not try to guess issue_flags, just use
> a newly added io_uring_cmd_complete(). We're loosing an optimisation in
> the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
> is that we don't care that much about it.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  drivers/block/ublk_drv.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index bea3d5cf8a83..97dceecadab2 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
>  	return true;
>  }
>  
> -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> -		unsigned int issue_flags)
> +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
>  {
>  	bool done;
>  
> @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>  	spin_unlock(&ubq->cancel_lock);
>  
>  	if (!done)
> -		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
> +		io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
>  }
>  
>  /*
>   * The ublk char device won't be closed when calling cancel fn, so both
>   * ublk device and queue are guaranteed to be live
>   */
> -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> -		unsigned int issue_flags)
> +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
>  {
>  	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>  	struct ublk_queue *ubq = pdu->ubq;
> @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>  
>  	io = &ubq->ios[pdu->tag];
>  	WARN_ON_ONCE(io->cmd != cmd);
> -	ublk_cancel_cmd(ubq, io, issue_flags);
> +	ublk_cancel_cmd(ubq, io);

.cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
be removed, otherwise double task run is caused because .cancel_fn
can be called multiple times if the request stays in ctx->cancelable_uring_cmd.


Thanks,
Ming
Pavel Begunkov March 18, 2024, 12:52 p.m. UTC | #2
On 3/18/24 08:16, Ming Lei wrote:
> On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
>> uring_cmd implementations should not try to guess issue_flags, just use
>> a newly added io_uring_cmd_complete(). We're loosing an optimisation in
>> the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
>> is that we don't care that much about it.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   drivers/block/ublk_drv.c | 18 ++++++++----------
>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index bea3d5cf8a83..97dceecadab2 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
>>   	return true;
>>   }
>>   
>> -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>> -		unsigned int issue_flags)
>> +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
>>   {
>>   	bool done;
>>   
>> @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>   	spin_unlock(&ubq->cancel_lock);
>>   
>>   	if (!done)
>> -		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
>> +		io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
>>   }
>>   
>>   /*
>>    * The ublk char device won't be closed when calling cancel fn, so both
>>    * ublk device and queue are guaranteed to be live
>>    */
>> -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>> -		unsigned int issue_flags)
>> +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
>>   {
>>   	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>>   	struct ublk_queue *ubq = pdu->ubq;
>> @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>   
>>   	io = &ubq->ios[pdu->tag];
>>   	WARN_ON_ONCE(io->cmd != cmd);
>> -	ublk_cancel_cmd(ubq, io, issue_flags);
>> +	ublk_cancel_cmd(ubq, io);
> 
> .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
> be removed, otherwise double task run is caused because .cancel_fn
> can be called multiple times if the request stays in ctx->cancelable_uring_cmd.

I see, that's exactly why I was asking whether it can be deferred
to tw. Let me see if I can get by without that patch, but honestly
it's a horrible abuse of the ring state. Any ideas how that can be
cleaned up?
Pavel Begunkov March 18, 2024, 1:37 p.m. UTC | #3
On 3/18/24 12:52, Pavel Begunkov wrote:
> On 3/18/24 08:16, Ming Lei wrote:
>> On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
>>> uring_cmd implementations should not try to guess issue_flags, just use
>>> a newly added io_uring_cmd_complete(). We're loosing an optimisation in
>>> the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
>>> is that we don't care that much about it.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>   drivers/block/ublk_drv.c | 18 ++++++++----------
>>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>>> index bea3d5cf8a83..97dceecadab2 100644
>>> --- a/drivers/block/ublk_drv.c
>>> +++ b/drivers/block/ublk_drv.c
>>> @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
>>>       return true;
>>>   }
>>> -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>> -        unsigned int issue_flags)
>>> +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
>>>   {
>>>       bool done;
>>> @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>>       spin_unlock(&ubq->cancel_lock);
>>>       if (!done)
>>> -        io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
>>> +        io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
>>>   }
>>>   /*
>>>    * The ublk char device won't be closed when calling cancel fn, so both
>>>    * ublk device and queue are guaranteed to be live
>>>    */
>>> -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>> -        unsigned int issue_flags)
>>> +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
>>>   {
>>>       struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>>>       struct ublk_queue *ubq = pdu->ubq;
>>> @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>>       io = &ubq->ios[pdu->tag];
>>>       WARN_ON_ONCE(io->cmd != cmd);
>>> -    ublk_cancel_cmd(ubq, io, issue_flags);
>>> +    ublk_cancel_cmd(ubq, io);
>>
>> .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
>> be removed, otherwise double task run is caused because .cancel_fn
>> can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
> 
> I see, that's exactly why I was asking whether it can be deferred
> to tw. Let me see if I can get by without that patch, but honestly
> it's a horrible abuse of the ring state. Any ideas how that can be
> cleaned up?

I assume io_uring_try_cancel_uring_cmd() can run in parallel with
completions, so there can be two parallel calls calls to ->uring_cmd
(e.g. io-wq + cancel), which gives me shivers. Also, I'd rather
no cancel in place requests of another task, io_submit_flush_completions()
but it complicates things.

Is there any argument against removing requests from the cancellation
list in io_uring_try_cancel_uring_cmd() before calling ->uring_cmd?

io_uring_try_cancel_uring_cmd() {
	lock();
	for_each_req() {
		remove_req_from_cancel_list(req);
		req->file->uring_cmd();
	}
	unlock();
}
Pavel Begunkov March 18, 2024, 2:32 p.m. UTC | #4
On 3/18/24 13:37, Pavel Begunkov wrote:
> On 3/18/24 12:52, Pavel Begunkov wrote:
>> On 3/18/24 08:16, Ming Lei wrote:
>>> On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
>>>> uring_cmd implementations should not try to guess issue_flags, just use
>>>> a newly added io_uring_cmd_complete(). We're loosing an optimisation in
>>>> the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
>>>> is that we don't care that much about it.
>>>>
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>   drivers/block/ublk_drv.c | 18 ++++++++----------
>>>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>>>> index bea3d5cf8a83..97dceecadab2 100644
>>>> --- a/drivers/block/ublk_drv.c
>>>> +++ b/drivers/block/ublk_drv.c
>>>> @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
>>>>       return true;
>>>>   }
>>>> -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>>> -        unsigned int issue_flags)
>>>> +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
>>>>   {
>>>>       bool done;
>>>> @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>>>       spin_unlock(&ubq->cancel_lock);
>>>>       if (!done)
>>>> -        io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
>>>> +        io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
>>>>   }
>>>>   /*
>>>>    * The ublk char device won't be closed when calling cancel fn, so both
>>>>    * ublk device and queue are guaranteed to be live
>>>>    */
>>>> -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>>> -        unsigned int issue_flags)
>>>> +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
>>>>   {
>>>>       struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>>>>       struct ublk_queue *ubq = pdu->ubq;
>>>> @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>>>       io = &ubq->ios[pdu->tag];
>>>>       WARN_ON_ONCE(io->cmd != cmd);
>>>> -    ublk_cancel_cmd(ubq, io, issue_flags);
>>>> +    ublk_cancel_cmd(ubq, io);
>>>
>>> .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
>>> be removed, otherwise double task run is caused because .cancel_fn
>>> can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
>>
>> I see, that's exactly why I was asking whether it can be deferred
>> to tw. Let me see if I can get by without that patch, but honestly
>> it's a horrible abuse of the ring state. Any ideas how that can be
>> cleaned up?
> 
> I assume io_uring_try_cancel_uring_cmd() can run in parallel with
> completions, so there can be two parallel calls calls to ->uring_cmd
> (e.g. io-wq + cancel), which gives me shivers. Also, I'd rather
> no cancel in place requests of another task, io_submit_flush_completions()
> but it complicates things.

I'm wrong though on flush_completions, the task there cancels only
its own requests

io_uring_try_cancel_uring_cmd() {
	...
	if (!cancel_all && req->task != task)
		continue;
}


> Is there any argument against removing requests from the cancellation
> list in io_uring_try_cancel_uring_cmd() before calling ->uring_cmd?
> 
> io_uring_try_cancel_uring_cmd() {
>      lock();
>      for_each_req() {
>          remove_req_from_cancel_list(req);
>          req->file->uring_cmd();
>      }
>      unlock();
> }
>
Ming Lei March 18, 2024, 2:34 p.m. UTC | #5
On Mon, Mar 18, 2024 at 12:52:33PM +0000, Pavel Begunkov wrote:
> On 3/18/24 08:16, Ming Lei wrote:
> > On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
> > > uring_cmd implementations should not try to guess issue_flags, just use
> > > a newly added io_uring_cmd_complete(). We're loosing an optimisation in
> > > the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
> > > is that we don't care that much about it.
> > > 
> > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
> > > Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > > ---
> > >   drivers/block/ublk_drv.c | 18 ++++++++----------
> > >   1 file changed, 8 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index bea3d5cf8a83..97dceecadab2 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
> > >   	return true;
> > >   }
> > > -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> > > -		unsigned int issue_flags)
> > > +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
> > >   {
> > >   	bool done;
> > > @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> > >   	spin_unlock(&ubq->cancel_lock);
> > >   	if (!done)
> > > -		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
> > > +		io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
> > >   }
> > >   /*
> > >    * The ublk char device won't be closed when calling cancel fn, so both
> > >    * ublk device and queue are guaranteed to be live
> > >    */
> > > -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > > -		unsigned int issue_flags)
> > > +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
> > >   {
> > >   	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > >   	struct ublk_queue *ubq = pdu->ubq;
> > > @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > >   	io = &ubq->ios[pdu->tag];
> > >   	WARN_ON_ONCE(io->cmd != cmd);
> > > -	ublk_cancel_cmd(ubq, io, issue_flags);
> > > +	ublk_cancel_cmd(ubq, io);
> > 
> > .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
> > be removed, otherwise double task run is caused because .cancel_fn
> > can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
> 
> I see, that's exactly why I was asking whether it can be deferred
> to tw. Let me see if I can get by without that patch, but honestly
> it's a horrible abuse of the ring state. Any ideas how that can be
> cleaned up?

Simply deferring io_uring_cmd_done() in ublk_cancel_cmd() to tw still triggers
warning in  __put_task_struct(), so I'd suggest to add the patch until
it is root-cause & fixed.



Thanks,
Ming
Ming Lei March 18, 2024, 2:39 p.m. UTC | #6
On Mon, Mar 18, 2024 at 02:32:16PM +0000, Pavel Begunkov wrote:
> On 3/18/24 13:37, Pavel Begunkov wrote:
> > On 3/18/24 12:52, Pavel Begunkov wrote:
> > > On 3/18/24 08:16, Ming Lei wrote:
> > > > On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
> > > > > uring_cmd implementations should not try to guess issue_flags, just use
> > > > > a newly added io_uring_cmd_complete(). We're loosing an optimisation in
> > > > > the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
> > > > > is that we don't care that much about it.
> > > > > 
> > > > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > > > Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
> > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > > > > ---
> > > > >   drivers/block/ublk_drv.c | 18 ++++++++----------
> > > > >   1 file changed, 8 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > > index bea3d5cf8a83..97dceecadab2 100644
> > > > > --- a/drivers/block/ublk_drv.c
> > > > > +++ b/drivers/block/ublk_drv.c
> > > > > @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
> > > > >       return true;
> > > > >   }
> > > > > -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> > > > > -        unsigned int issue_flags)
> > > > > +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
> > > > >   {
> > > > >       bool done;
> > > > > @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> > > > >       spin_unlock(&ubq->cancel_lock);
> > > > >       if (!done)
> > > > > -        io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
> > > > > +        io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
> > > > >   }
> > > > >   /*
> > > > >    * The ublk char device won't be closed when calling cancel fn, so both
> > > > >    * ublk device and queue are guaranteed to be live
> > > > >    */
> > > > > -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > > > > -        unsigned int issue_flags)
> > > > > +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
> > > > >   {
> > > > >       struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > > > >       struct ublk_queue *ubq = pdu->ubq;
> > > > > @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > > > >       io = &ubq->ios[pdu->tag];
> > > > >       WARN_ON_ONCE(io->cmd != cmd);
> > > > > -    ublk_cancel_cmd(ubq, io, issue_flags);
> > > > > +    ublk_cancel_cmd(ubq, io);
> > > > 
> > > > .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
> > > > be removed, otherwise double task run is caused because .cancel_fn
> > > > can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
> > > 
> > > I see, that's exactly why I was asking whether it can be deferred
> > > to tw. Let me see if I can get by without that patch, but honestly
> > > it's a horrible abuse of the ring state. Any ideas how that can be
> > > cleaned up?
> > 
> > I assume io_uring_try_cancel_uring_cmd() can run in parallel with
> > completions, so there can be two parallel calls calls to ->uring_cmd
> > (e.g. io-wq + cancel), which gives me shivers. Also, I'd rather
> > no cancel in place requests of another task, io_submit_flush_completions()
> > but it complicates things.
> 
> I'm wrong though on flush_completions, the task there cancels only
> its own requests
> 
> io_uring_try_cancel_uring_cmd() {
> 	...
> 	if (!cancel_all && req->task != task)
> 		continue;
> }
> 
> 
> > Is there any argument against removing requests from the cancellation
> > list in io_uring_try_cancel_uring_cmd() before calling ->uring_cmd?
> > 
> > io_uring_try_cancel_uring_cmd() {
> >      lock();
> >      for_each_req() {
> >          remove_req_from_cancel_list(req);
> >          req->file->uring_cmd();
> >      }
> >      unlock();

Also the req may not be ready to cancel in ->uring_cmd(), so it
should be allowed to retry in future if it isn't canceled this time.


Thanks,
Ming
Pavel Begunkov March 18, 2024, 3:08 p.m. UTC | #7
On 3/18/24 14:34, Ming Lei wrote:
> On Mon, Mar 18, 2024 at 12:52:33PM +0000, Pavel Begunkov wrote:
>> On 3/18/24 08:16, Ming Lei wrote:
>>> On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
>>>> uring_cmd implementations should not try to guess issue_flags, just use
>>>> a newly added io_uring_cmd_complete(). We're loosing an optimisation in
>>>> the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
>>>> is that we don't care that much about it.
>>>>
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>    drivers/block/ublk_drv.c | 18 ++++++++----------
>>>>    1 file changed, 8 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>>>> index bea3d5cf8a83..97dceecadab2 100644
>>>> --- a/drivers/block/ublk_drv.c
>>>> +++ b/drivers/block/ublk_drv.c
>>>> @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
>>>>    	return true;
>>>>    }
>>>> -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>>> -		unsigned int issue_flags)
>>>> +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
>>>>    {
>>>>    	bool done;
>>>> @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>>>    	spin_unlock(&ubq->cancel_lock);
>>>>    	if (!done)
>>>> -		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
>>>> +		io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
>>>>    }
>>>>    /*
>>>>     * The ublk char device won't be closed when calling cancel fn, so both
>>>>     * ublk device and queue are guaranteed to be live
>>>>     */
>>>> -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>>> -		unsigned int issue_flags)
>>>> +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
>>>>    {
>>>>    	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>>>>    	struct ublk_queue *ubq = pdu->ubq;
>>>> @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>>>>    	io = &ubq->ios[pdu->tag];
>>>>    	WARN_ON_ONCE(io->cmd != cmd);
>>>> -	ublk_cancel_cmd(ubq, io, issue_flags);
>>>> +	ublk_cancel_cmd(ubq, io);
>>>
>>> .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
>>> be removed, otherwise double task run is caused because .cancel_fn
>>> can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
>>
>> I see, that's exactly why I was asking whether it can be deferred
>> to tw. Let me see if I can get by without that patch, but honestly
>> it's a horrible abuse of the ring state. Any ideas how that can be
>> cleaned up?
> 
> Simply deferring io_uring_cmd_done() in ublk_cancel_cmd() to tw still triggers
> warning in  __put_task_struct(), so I'd suggest to add the patch until
> it is root-cause & fixed.

I mean drop the patch[es] changing how ublk passes issue_flags
around, moving cancellation point and all related, and leave it
to later really hoping we'll figure how to do it better.
Ming Lei March 18, 2024, 3:16 p.m. UTC | #8
On Mon, Mar 18, 2024 at 03:08:19PM +0000, Pavel Begunkov wrote:
> On 3/18/24 14:34, Ming Lei wrote:
> > On Mon, Mar 18, 2024 at 12:52:33PM +0000, Pavel Begunkov wrote:
> > > On 3/18/24 08:16, Ming Lei wrote:
> > > > On Mon, Mar 18, 2024 at 12:41:50AM +0000, Pavel Begunkov wrote:
> > > > > uring_cmd implementations should not try to guess issue_flags, just use
> > > > > a newly added io_uring_cmd_complete(). We're loosing an optimisation in
> > > > > the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption
> > > > > is that we don't care that much about it.
> > > > > 
> > > > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > > > Link: https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com
> > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > > > > ---
> > > > >    drivers/block/ublk_drv.c | 18 ++++++++----------
> > > > >    1 file changed, 8 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > > index bea3d5cf8a83..97dceecadab2 100644
> > > > > --- a/drivers/block/ublk_drv.c
> > > > > +++ b/drivers/block/ublk_drv.c
> > > > > @@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
> > > > >    	return true;
> > > > >    }
> > > > > -static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> > > > > -		unsigned int issue_flags)
> > > > > +static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
> > > > >    {
> > > > >    	bool done;
> > > > > @@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
> > > > >    	spin_unlock(&ubq->cancel_lock);
> > > > >    	if (!done)
> > > > > -		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
> > > > > +		io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
> > > > >    }
> > > > >    /*
> > > > >     * The ublk char device won't be closed when calling cancel fn, so both
> > > > >     * ublk device and queue are guaranteed to be live
> > > > >     */
> > > > > -static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > > > > -		unsigned int issue_flags)
> > > > > +static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
> > > > >    {
> > > > >    	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > > > >    	struct ublk_queue *ubq = pdu->ubq;
> > > > > @@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
> > > > >    	io = &ubq->ios[pdu->tag];
> > > > >    	WARN_ON_ONCE(io->cmd != cmd);
> > > > > -	ublk_cancel_cmd(ubq, io, issue_flags);
> > > > > +	ublk_cancel_cmd(ubq, io);
> > > > 
> > > > .cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't
> > > > be removed, otherwise double task run is caused because .cancel_fn
> > > > can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
> > > 
> > > I see, that's exactly why I was asking whether it can be deferred
> > > to tw. Let me see if I can get by without that patch, but honestly
> > > it's a horrible abuse of the ring state. Any ideas how that can be
> > > cleaned up?
> > 
> > Simply deferring io_uring_cmd_done() in ublk_cancel_cmd() to tw still triggers
> > warning in  __put_task_struct(), so I'd suggest to add the patch until
> > it is root-cause & fixed.
> 
> I mean drop the patch[es] changing how ublk passes issue_flags
> around, moving cancellation point and all related, and leave it
> to later really hoping we'll figure how to do it better.

Looks fine for me.

Thanks,
Ming
diff mbox series

Patch

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index bea3d5cf8a83..97dceecadab2 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1417,8 +1417,7 @@  static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)
 	return true;
 }
 
-static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
-		unsigned int issue_flags)
+static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io)
 {
 	bool done;
 
@@ -1432,15 +1431,14 @@  static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
 	spin_unlock(&ubq->cancel_lock);
 
 	if (!done)
-		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
+		io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0);
 }
 
 /*
  * The ublk char device won't be closed when calling cancel fn, so both
  * ublk device and queue are guaranteed to be live
  */
-static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
-		unsigned int issue_flags)
+static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd)
 {
 	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
 	struct ublk_queue *ubq = pdu->ubq;
@@ -1464,7 +1462,7 @@  static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
 
 	io = &ubq->ios[pdu->tag];
 	WARN_ON_ONCE(io->cmd != cmd);
-	ublk_cancel_cmd(ubq, io, issue_flags);
+	ublk_cancel_cmd(ubq, io);
 
 	if (need_schedule) {
 		if (ublk_can_use_recovery(ub))
@@ -1484,7 +1482,7 @@  static void ublk_cancel_queue(struct ublk_queue *ubq)
 	int i;
 
 	for (i = 0; i < ubq->q_depth; i++)
-		ublk_cancel_cmd(ubq, &ubq->ios[i], IO_URING_F_UNLOCKED);
+		ublk_cancel_cmd(ubq, &ubq->ios[i]);
 }
 
 /* Cancel all pending commands, must be called after del_gendisk() returns */
@@ -1777,7 +1775,7 @@  static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 	return -EIOCBQUEUED;
 
  out:
-	io_uring_cmd_done(cmd, ret, 0, issue_flags);
+	io_uring_cmd_complete(cmd, ret, 0);
 	pr_devel("%s: complete: cmd op %d, tag %d ret %x io_flags %x\n",
 			__func__, cmd_op, tag, ret, io->flags);
 	return -EIOCBQUEUED;
@@ -1842,7 +1840,7 @@  static void ublk_ch_uring_cmd_cb(struct io_uring_cmd *cmd,
 static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
 {
 	if (unlikely(issue_flags & IO_URING_F_CANCEL)) {
-		ublk_uring_cmd_cancel_fn(cmd, issue_flags);
+		ublk_uring_cmd_cancel_fn(cmd);
 		return 0;
 	}
 
@@ -2930,7 +2928,7 @@  static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 	if (ub)
 		ublk_put_device(ub);
  out:
-	io_uring_cmd_done(cmd, ret, 0, issue_flags);
+	io_uring_cmd_complete(cmd, ret, 0);
 	pr_devel("%s: cmd done ret %d cmd_op %x, dev id %d qid %d\n",
 			__func__, ret, cmd->cmd_op, header->dev_id, header->queue_id);
 	return -EIOCBQUEUED;