Re: [PATCH 4/5] dm: implement REQ_FLUSH/FUA support for request-based dm

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Aug 30 2010 at  5:58am -0400,
Tejun Heo <tj@kernel.org> wrote:

> This patch converts request-based dm to support the new REQ_FLUSH/FUA.
...
> This patch rips out special flush code path and deals handles
> REQ_FLUSH/FUA requests the same way as other requests.  The only
> special treatment is that REQ_FLUSH requests use the block address 0
> when finding target, which is enough for now.

Looks very comparable to the patch I prepared but I have 2 observations
below (based on my findings from testing my patch).

> @@ -1562,22 +1483,15 @@ static int setup_clone(struct request *clone, struct request *rq,
>  {
>  	int r;
>  
> -	if (dm_rq_is_flush_request(rq)) {
> -		blk_rq_init(NULL, clone);
> -		clone->cmd_type = REQ_TYPE_FS;
> -		clone->cmd_flags |= (REQ_HARDBARRIER | WRITE);
> -	} else {
> -		r = blk_rq_prep_clone(clone, rq, tio->md->bs, GFP_ATOMIC,
> -				      dm_rq_bio_constructor, tio);
> -		if (r)
> -			return r;
> -
> -		clone->cmd = rq->cmd;
> -		clone->cmd_len = rq->cmd_len;
> -		clone->sense = rq->sense;
> -		clone->buffer = rq->buffer;
> -	}
> +	r = blk_rq_prep_clone(clone, rq, tio->md->bs, GFP_ATOMIC,
> +			      dm_rq_bio_constructor, tio);
> +	if (r)
> +		return r;
>  
> +	clone->cmd = rq->cmd;
> +	clone->cmd_len = rq->cmd_len;
> +	clone->sense = rq->sense;
> +	clone->buffer = rq->buffer;
>  	clone->end_io = end_clone_request;
>  	clone->end_io_data = tio;

blk_rq_prep_clone() of a REQ_FLUSH request will result in a
rq_data_dir(clone) of read.

I still had the following:

        if (rq->cmd_flags & REQ_FLUSH) {
                blk_rq_init(NULL, clone);
                clone->cmd_type = REQ_TYPE_FS;
                /* without this the clone has a rq_data_dir of 0 */
                clone->cmd_flags |= WRITE_FLUSH;
        } else {
                r = blk_rq_prep_clone(clone, rq, tio->md->bs, GFP_ATOMIC,
                                      dm_rq_bio_constructor, tio);
                ...

Request-based DM's REQ_FLUSH still works without this special casing but
I figured I'd raise this to ask: what is the proper rq_data_dir() is for
a REQ_FLUSH?

> @@ -1709,15 +1621,12 @@ static void dm_request_fn(struct request_queue *q)
>  		if (!rq)
>  			goto plug_and_out;
>  
> -		if (unlikely(dm_rq_is_flush_request(rq))) {
> -			BUG_ON(md->flush_request);
> -			md->flush_request = rq;
> -			blk_start_request(rq);
> -			queue_work(md->wq, &md->barrier_work);
> -			goto out;
> -		}
> +		/* always use block 0 to find the target for flushes for now */
> +		pos = 0;
> +		if (!(rq->cmd_flags & REQ_FLUSH))
> +			pos = blk_rq_pos(rq);
>  
> -		ti = dm_table_find_target(map, blk_rq_pos(rq));
> +		ti = dm_table_find_target(map, pos);

I added the following here: BUG_ON(!dm_target_is_valid(ti));

>  		if (ti->type->busy && ti->type->busy(ti))
>  			goto plug_and_out;

I also needed to avoid the ->busy call for REQ_FLUSH:

                if (!(rq->cmd_flags & REQ_FLUSH)) {
                        ti = dm_table_find_target(map, blk_rq_pos(rq));
                        BUG_ON(!dm_target_is_valid(ti));
                        if (ti->type->busy && ti->type->busy(ti))
                                goto plug_and_out;
                } else {
                        /* rq-based only ever has one target! leverage this for FLUSH */
                        ti = dm_table_get_target(map, 0);
                }

If I allowed ->busy to be called for REQ_FLUSH it would result in a
deadlock.  I haven't identified where/why yet.

Other than these remaining issues this patch looks good.

Thanks,
Mike