[dm-devel] Re: [PATCH] add endio fn to request structures

[Jens Axboe]

On Mon, May 24 2004, Mike Christie wrote:
> Jens,
> 
> Some storage devices have a mode where they require a special
> command be sent inorder to perform failover. IBM's fastt is one of
> these devices we are trying to add support for using the dm-multipath
> framework.
> 
> The problem is that dm works at the bio level and we need to send down
> something like a block pc or special request. So from dm multipath we could
> have a thread and do a blk_execute_rq, or if we had some sort of end_request
> function similar to the bio's and bh's endio function we could send the
> failover command asynchronously. The attached patch adds an end_request fn
> to the request structure for this purpose.
> 
> Maybe the fact that I have to ask for such a change should indidcate that
> I am doing something brain dead, in which case please be gentle in your 
> review
> of my patch :)

I don't think you are, on and off I've added this feature myself
privately for various reasons. I can think of several places in the
kernel right now that could be made a lot easier with this sort of async
notication.

> @@ -2752,9 +2769,16 @@ void end_that_request_last(struct reques
>  		disk->in_flight--;
>  	}
>  	__blk_put_request(req->q, req);
> -	/* Do this LAST! The structure may be freed immediately afterwards */
> -	if (waiting)
> -		complete(waiting);
> +
> +	/*
> +	 * If you are allocating reqs from the block layer you
> +	 * should get a handle to the req and release it after
> +	 * your end_request_fn is run.
> +	 */
> +	if (end_fn) {
> +		req->end_request = NULL;
> +		end_fn(req);
> +	}

Uh oh, this is bad news. Let __blk_put_request() clear ->end_request()
if you need to, you definitely cannot do it here after putting your
reference to it.

> diff -aurp linux-2.6.6/drivers/ide/ide-cd.c linux-2.6.6-work/drivers/ide/ide-cd.c
> --- linux-2.6.6/drivers/ide/ide-cd.c	2004-05-09 19:32:29.000000000 -0700
> +++ linux-2.6.6-work/drivers/ide/ide-cd.c	2004-05-22 15:17:05.000000000 -0700
> @@ -555,6 +555,8 @@ static void cdrom_queue_request_sense(id
>  
>  	rq->flags = REQ_SENSE;
>  	rq->waiting = wait;
> +	if (wait)
> +		rq->end_request = blk_end_sync_rq;
>  
>  	/* NOTE! Save the failed command in "rq->buffer" */
>  	rq->buffer = (void *) failed_command;
> @@ -719,6 +721,7 @@ static int cdrom_decode_status(ide_drive
>  		if ((stat & ERR_STAT) != 0) {
>  			wait = rq->waiting;
>  			rq->waiting = NULL;
> +			rq->end_request = NULL;
>  			if ((rq->flags & REQ_BLOCK_PC) != 0) {
>  				cdrom_queue_request_sense(drive, wait,
>  							  rq->sense, rq);

I'd rather you don't 'convert' any drivers. blk_execute_rq() change is
fine, but leave these alone. They need seperate checking imo since cases
like the above aren't straight forward.

-- 
Jens Axboe