linux-kernel - Re: [PATCH 5/7] blk-mq: remove REQ_ATOM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64dfa760-d433-1537-9bc6-b12cda3c9dc6@oracle.com>
Date:   Thu, 21 Dec 2017 11:56:49 +0800
From:   "jianchao.wang" <jianchao.w.wang@...cle.com>
To:     Tejun Heo <tj@...nel.org>, jbacik@...com
Cc:     jack@...e.cz, axboe@...nel.dk, clm@...com, kernel-team@...com,
        linux-kernel@...r.kernel.org, linux-btrfs@...r.kernel.org,
        peterz@...radead.org, Bart.VanAssche@....com
Subject: Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq

Hi tejun

On 12/16/2017 08:07 PM, Tejun Heo wrote:
> After the recent updates to use generation number and state based
> synchronization, blk-mq no longer depends on REQ_ATOM_COMPLETE except
> to avoid firing the same timeout multiple times.
> 
> Remove all REQ_ATOM_COMPLETE usages and use a new rq_flags flag
> RQF_MQ_TIMEOUT_EXPIRED to avoid firing the same timeout multiple
> times.  This removes atomic bitops from hot paths too.
> 
> v2: Removed blk_clear_rq_complete() from blk_mq_rq_timed_out().
> 
> v3: Added RQF_MQ_TIMEOUT_EXPIRED flag.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>
> Cc: "jianchao.wang" <jianchao.w.wang@...cle.com>
> ---
>  block/blk-mq.c         | 18 ++++++++----------
>  block/blk-timeout.c    |  1 +
>  include/linux/blkdev.h |  2 ++
>  3 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 88baa82..47e722b 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -607,14 +607,12 @@ void blk_mq_complete_request(struct request *rq)
>  	 */
>  	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
>  		rcu_read_lock();
> -		if (blk_mq_rq_aborted_gstate(rq) != rq->gstate &&
> -		    !blk_mark_rq_complete(rq))
> +		if (blk_mq_rq_aborted_gstate(rq) != rq->gstate)
>  			__blk_mq_complete_request(rq);
>  		rcu_read_unlock();
>  	} else {
>  		srcu_idx = srcu_read_lock(hctx->queue_rq_srcu);
> -		if (blk_mq_rq_aborted_gstate(rq) != rq->gstate &&
> -		    !blk_mark_rq_complete(rq))
> +		if (blk_mq_rq_aborted_gstate(rq) != rq->gstate)
>  			__blk_mq_complete_request(rq);
>  		srcu_read_unlock(hctx->queue_rq_srcu, srcu_idx);
>  	}
It's worrying that even though the blk_mark_rq_complete() here is intended to synchronize with
timeout path, but it indeed give the blk_mq_complete_request() the capability to exclude with 
itself. Maybe this capability should be reserved.

Thanks
Jianchao