Re: [PATCH 2/2] io_uring: optimise io_fail_links()

[Pavel Begunkov <asml.silence@gmail.com>]

On 15/10/2020 09:53, Nathan Chancellor wrote:
> On Wed, Oct 14, 2020 at 08:44:22PM +0100, Pavel Begunkov wrote:
>> -		io_put_req_deferred(link, 2);
>> +
>> +		/*
>> +		 * It's ok to free under spinlock as they're not linked anymore,
>> +		 * but avoid REQ_F_WORK_INITIALIZED because it may deadlock on
>> +		 * work.fs->lock.
>> +		 */
>> +		if (link->flags | REQ_F_WORK_INITIALIZED)
>> +			io_put_req_deferred(link, 2);
>> +		else
>> +			io_double_put_req(link);
> 
> fs/io_uring.c:1816:19: warning: bitwise or with non-zero value always
> evaluates to true [-Wtautological-bitwise-compare]
>                 if (link->flags | REQ_F_WORK_INITIALIZED)
>                     ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
> 
> According to the comment, was it intended for that to be a bitwise AND
> then negated to check for the absence of it? If so, wouldn't it be
> clearer to flip the condition so that a negation is not necessary like
> below? I can send a formal patch if my analysis is correct but if not,
> feel free to fix it yourself and add

I have no idea what have happened, but yeah, there should be "&",
though without any additional negation. That's because deferred
version is safer. 

Nathan, thanks for letting know!
Jens, could you please fold in the change below.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 66c41d53a9d3..2c83c2688ec5 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1813,7 +1813,7 @@ static void __io_fail_links(struct io_kiocb *req)
 		 * but avoid REQ_F_WORK_INITIALIZED because it may deadlock on
 		 * work.fs->lock.
 		 */
-		if (link->flags | REQ_F_WORK_INITIALIZED)
+		if (link->flags & REQ_F_WORK_INITIALIZED)
 			io_put_req_deferred(link, 2);
 		else
 			io_double_put_req(link);


-- 
Pavel Begunkov