lkml.org 
[lkml]   [2002]   [Jan]   [16]   [last100]   RSS Feed
Views: [more markup]   [less markup]   [headers]   [forward]  
 
Messages in this thread
/
DateWed, 16 Jan 2002 08:07:05 +0100
FromJens Axboe <>
SubjectRe: block completion races
On Tue, Jan 15 2002, Andrew Morton wrote:
> void end_that_request_last(struct request *req)
> {
>         if (req->waiting != NULL)
>                 complete(req->waiting);
> 
>         blkdev_release_request(req);
> }
> 
> 
> I think a bug.  Sometimes (eg, cdrom_queue_packet_command())
> the request is allocated on a task's kernel stack.  As soon as
> we call complete(), that task can wake and release the request
> while blkdev_release_request() is diddling it on this CPU.
> 
> Do you see any problem with releasing the request before running
> complete()?.  Also I think it's best to uninline blkdev_release_request().
> It's 104 bytes long, and we have four copies of it in ll_rw_blk.c.  A
> patch is here.

Agreed, patch is fine with me.

> Also, there is this code in ide_do_drive_cmd():
> 
>         if (action == ide_wait) {
>                 wait_for_completion(&wait);     /* wait for it to be serviced */
>                 return rq->errors ? -EIO : 0;   /* return -EIO if errors */
>         }
> 
> Is it safe to use `rq' here?  It has just been recycled in
> end_that_request_last() and we don't own it any more.

Not really, I guess it would be easiest to make end_that_request_last
return errors status.

> --- linux-2.4.18-pre4/drivers/block/ll_rw_blk.c	Tue Jan 15 15:08:24 2002
> +++ linux-akpm/drivers/block/ll_rw_blk.c	Tue Jan 15 17:39:22 2002
> @@ -546,7 +546,7 @@ static inline void add_request(request_q
>  /*
>   * Must be called with io_request_lock held and interrupts disabled
>   */
> -inline void blkdev_release_request(struct request *req)
> +void blkdev_release_request(struct request *req)
>  {
>  	request_queue_t *q = req->q;
>  	int rw = req->cmd;
> @@ -1084,10 +1084,11 @@ int end_that_request_first (struct reque
> 
>  void end_that_request_last(struct request *req)
>  {
> -	if (req->waiting != NULL)
> -		complete(req->waiting);
> +	struct completion *waiting = req->waiting;
> 
>  	blkdev_release_request(req);
> +	if (waiting != NULL)
> +		complete(waiting);
>  }
> 
>  #define MB(kb)	((kb) << 10)

I've applied this.

-- 
Jens Axboe

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

\
 
 \ /
  Last update: 2005-03-22 11:23    [W:0.566 / U:0.260 seconds]
©2003-2008 Jasper Spaans