block completion races

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.

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.

I think the simplest approach to this one is to make the error code a part of the completion structure, so:

struct blkdev_completion {
	struct completion completion;
	int errcode;
};

If you agree, I'll do the patch.

drivers/block/ll_rw_blk.c +4 -4

@@ -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)
-

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

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.

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.

drivers/block/ll_rw_blk.c +1 -1

@@ -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)
 {

We have a more interesting problem!
This does not show up in 2.5.1 which is patch base for 2.5.3-pre1. It does show up in 2.5.2-pre10 but I have not walked the patch through 2.5.2preX series.

We have a very bad queue race that is PIO specific but really the whole darn driver before the patch was applied. ACB only tighten the driver's alignment to the NCITS standards. One should note the direct access via the ioctl does not lock the driver. Only coming down from BLOCK will this event occur.

Repeatable test "hdparm -d0 -t /dev/hdx"

If you apply the acb-io patch to 2.5.1 this does not happen.

In the introduction of BIO, there were no "q->queue_lock" applied to protecting the queue.

        /*
         * Is meant to protect the queue in the future instead of
         * io_request_lock
         */
        spinlock_t              queue_lock;

Well we pulled "io_request_lock" but did we forgot to insert or add q->queue_lock spinlocks?

It is going to be a LONG LONG NIGHT :-(