linux-kernel - Re: [patch] block: properly handle flush/fua requests in blk_insert_cloned

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: <20110809153826.GB23842@htj.dyndns.org>
Date:	Tue, 9 Aug 2011 17:38:26 +0200
From:	Tejun Heo <tj@...nel.org>
To:	Jeff Moyer <jmoyer@...hat.com>
Cc:	linux-kernel@...r.kernel.org, Jens Axboe <jaxboe@...ionio.com>,
	Mike Snitzer <msnitzer@...hat.com>,
	Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [patch] block: properly handle flush/fua requests in
 blk_insert_cloned_request

Hello,

On Tue, Aug 09, 2011 at 11:05:34AM -0400, Jeff Moyer wrote:
> @@ -1708,6 +1710,21 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)
>  	    should_fail_request(&rq->rq_disk->part0, blk_rq_bytes(rq)))
>  		return -EIO;
>  
> +	/*
> +	 * Check the cmd_flags against the flush flags of the underlying
> +	 * request_queue and resolve any differences.
> +	 */
> +	if (rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) {
> +		if (!(q->flush_flags & REQ_FLUSH))
> +			rq->cmd_flags &= ~REQ_FLUSH;
> +		if (!(q->flush_flags & REQ_FUA))
> +			rq->cmd_flags &= ~REQ_FUA;
> +		if (!(rq->cmd_flags & REQ_FLUSH) && !blk_rq_sectors(rq)) {
> +			blk_end_bidi_request(rq, 0, 0, 0);
> +			return 0;
> +		}
> +	}
> +

I'm a bit confused.  We still need ELEVATOR_INSERT_FLUSH fix for
insertion paths, right?  Or is blk_insert_cloned_request() supposed to
used only by request based dm which lives under the elevator?  If so,
it would be great to make that explicit in the comment.  Maybe just
renaming it to blk_insert_dm_cloned_request() would be better as it
wouldn't be safe for other cases anyway.

> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 6395692..4fe753f 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -168,7 +168,7 @@ enum rq_flag_bits {
>  #define REQ_COMMON_MASK \
>  	(REQ_WRITE | REQ_FAILFAST_MASK | REQ_SYNC | REQ_META | REQ_DISCARD | \
>  	 REQ_NOIDLE | REQ_FLUSH | REQ_FUA | REQ_SECURE)
> -#define REQ_CLONE_MASK		REQ_COMMON_MASK
> +#define REQ_CLONE_MASK		(REQ_COMMON_MASK | REQ_FLUSH_SEQ)

Given the weirdness, I think it deserves fat comment on why
REQ_FLUSH_SEQ is necessary.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/