Re: [patch] block: properly handle flush/fua requests inblk_insert_cloned_request

[Tejun Heo]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/