Re: [PATCH 1/4] io_uring: allow unbreakable links

[Jens Axboe <axboe@kernel.dk>]

On 12/10/19 3:13 AM, Pavel Begunkov wrote:
> On 12/10/2019 2:18 AM, Jens Axboe wrote:
>> Some commands will invariably end in a failure in the sense that the
>> completion result will be less than zero. One such example is timeouts
>> that don't have a completion count set, they will always complete with
>> -ETIME unless cancelled.
>>
>> For linked commands, we sever links and fail the rest of the chain if
>> the result is less than zero. Since we have commands where we know that
>> will happen, add IOSQE_IO_HARDLINK as a stronger link that doesn't sever
>> regardless of the completion result. Note that the link will still sever
>> if we fail submitting the parent request, hard links are only resilient
>> in the presence of completion results for requests that did submit
>> correctly.
>>
>> Cc: stable@vger.kernel.org # v5.4
>> Reported-by: 李通洲 <carter.li@eoitek.com>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  fs/io_uring.c                 | 54 +++++++++++++++++++++--------------
>>  include/uapi/linux/io_uring.h |  1 +
>>  2 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 405be10da73d..662404854571 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -377,6 +377,7 @@ struct io_kiocb {
>>  #define REQ_F_TIMEOUT_NOSEQ	8192	/* no timeout sequence */
>>  #define REQ_F_INFLIGHT		16384	/* on inflight list */
>>  #define REQ_F_COMP_LOCKED	32768	/* completion under lock */
>> +#define REQ_F_HARDLINK		65536	/* doesn't sever on completion < 0 */
>>  	u64			user_data;
>>  	u32			result;
>>  	u32			sequence;
>> @@ -941,7 +942,7 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
>>  
>>  		list_del_init(&req->link_list);
>>  		if (!list_empty(&nxt->link_list))
>> -			nxt->flags |= REQ_F_LINK;
>> +			nxt->flags |= req->flags & (REQ_F_LINK|REQ_F_HARDLINK);
> 
> I'm not sure we want to unconditionally propagate REQ_F_HARDLINK further.
> 
> E.g. timeout|REQ_F_HARDLINK -> read -> write
> REQ_F_HARDLINK will be set to the following read and its fail won't
> cancel the write, that seems strange. If users want such behaviour, they
> can set REQ_F_HARDLINK when needed by hand.

Yeah you're right, how about if we set REQ_F_HARDLINK in io_submit_sqe()
if IOSQE_IO_HARDLINK is set, and just keep the above as it is. We need
to ensure we're correctly keeping hard link if it's set in the sqe, not
carry over like the above if it wasn't set.

>>  		*nxtptr = nxt;
>>  		break;
>>  	}
>> @@ -1292,6 +1293,11 @@ static void kiocb_end_write(struct io_kiocb *req)
>>  	file_end_write(req->file);
>>  }
>>  
>> +static inline bool req_fail_links(struct io_kiocb *req)
>> +{
>> +	return (req->flags & (REQ_F_LINK | REQ_F_HARDLINK)) == REQ_F_LINK;
>> +}
>> +
> 
> req_fail_links() sounds like it not only do checking, but actually fails
> links. How about as follows?
> 
> +static inline void req_set_fail_links(struct io_kiocb *req)
> +{
> +	if (req->flags & (REQ_F_LINK | REQ_F_HARDLINK)) == REQ_F_LINK)
> +		req->flags |= REQ_F_FAIL_LINK;
> +}
> 
> And it would be less code below

That's cleaner, I'll make the change.

>> @@ -3518,7 +3528,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
>>  		 * If previous wasn't linked and we have a linked command,
>>  		 * that's the end of the chain. Submit the previous link.
>>  		 */
>> -		if (!(sqe_flags & IOSQE_IO_LINK) && link) {
>> +		if (!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) && link) {
> 
> IMHO, requests shouldn't have IOSQE_IO_HARDLINK without IOSQE_IO_LINK,
> the same is in the code. Then, it's sufficient check only IOSQE_IO_LINK.

IOSQE_IO_HARDLINK implies link behavior, it's really just a modifier. So
yes, I agree, and that change should then also be removable.

I'll post a v2, thanks for your review.

-- 
Jens Axboe