[PATCH 0/2] block: fix flush regression introduced in 3.1-rc3

A user reported a regression due to commit
4853abaae7e4a2af938115ce9071ef8684fb7af4 (block: fix flush
machinery for stacking drivers with differring flush flags).
Part of the problem is that blk_insert_flush required a
single bio be attached to the request. In reality, having
no attached bio is also a valid case, as can be observed with
an empty flush.

[1] http://www.redhat.com/archives/dm-d...00154.html

Reported-by: Christophe Saout
Signed-off-by: Jeff Moyer
Signed-off-by: Jeff Moyer

block/blk-flush.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 0ff29c6..720ad60 100644
a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -320,7 +320,7 @@ void blk_insert_flush(struct request *rq)
return;
}

- BUG_ON(!rq->bio || rq->bio != rq->biotail);
+ BUG_ON(rq->bio != rq->biotail); /*assumes zero or single bio rq */

/*
* If there's data but flush is not necessary, the request can be
1.7.1

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

On Wed, Oct 12, 2011 at 05:22:41PM -0400, Jeff Moyer wrote:

A dm-multipath user reported[1] a problem when trying to boot
a kernel with commit 4853abaae7e4a2af938115ce9071ef8684fb7af4
(block: fix flush machinery for stacking drivers with differring
flush flags) applied. It turns out that an empty flush request
can be sent into blk_insert_flush. When the BUG_ON was fixed
to allow for this, I/O on the underlying device would stall. The
reason is that blk_insert_cloned_request does not kick the queue.
In the aforementioned commit, I had added a special case to
kick the queue if data was sent down but the queue flags did
not require a flush. A better solution is to push the queue
kick up into blk_insert_cloned_request.

This patch, along with a follow-on which fixes the BUG_ON, fixes
the issue reported.

[1] http://www.redhat.com/archives/dm-d...00154.html

Reported-by: Christophe Saout
Signed-off-by: Jeff Moyer

Acked-by: Tejun Heo

Thank you for fixing this, but one curiosity, what happens for !flush
cloned requests? Is someone else responsible for kicking the queue?

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

On Wed, Oct 12, 2011 at 05:22:42PM -0400, Jeff Moyer wrote:

A user reported a regression due to commit
4853abaae7e4a2af938115ce9071ef8684fb7af4 (block: fix flush
machinery for stacking drivers with differring flush flags).
Part of the problem is that blk_insert_flush required a
single bio be attached to the request. In reality, having
no attached bio is also a valid case, as can be observed with
an empty flush.

[1] http://www.redhat.com/archives/dm-d...00154.html

Reported-by: Christophe Saout
Signed-off-by: Jeff Moyer
Signed-off-by: Jeff Moyer

Duplicate SOB's and missing space after '/*' in the comment. Other
than that,

Acked-by: Tejun Heo

Thanks.

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

On Wed, Oct 12, 2011 at 03:17:32PM -0700, Tejun Heo wrote:

On Wed, Oct 12, 2011 at 05:22:41PM -0400, Jeff Moyer wrote:
> A dm-multipath user reported[1] a problem when trying to boot
> a kernel with commit 4853abaae7e4a2af938115ce9071ef8684fb7af4
> (block: fix flush machinery for stacking drivers with differring
> flush flags) applied. It turns out that an empty flush request
> can be sent into blk_insert_flush. When the BUG_ON was fixed
> to allow for this, I/O on the underlying device would stall. The
> reason is that blk_insert_cloned_request does not kick the queue.
> In the aforementioned commit, I had added a special case to
> kick the queue if data was sent down but the queue flags did
> not require a flush. A better solution is to push the queue
> kick up into blk_insert_cloned_request.
>
> This patch, along with a follow-on which fixes the BUG_ON, fixes
> the issue reported.
>
> [1] http://www.redhat.com/archives/dm-d...00154.html
>
> Reported-by: Christophe Saout
> Signed-off-by: Jeff Moyer

Acked-by: Tejun Heo

Thank you for fixing this, but one curiosity, what happens for !flush
cloned requests? Is someone else responsible for kicking the queue?

I guess it is working for non flush requests because
blk_insert_cloned_request() inserts requests at the back (ELEVATOR_INSERT_BACK)
and elevator code is kicking the queue in that case.

case ELEVATOR_INSERT_BACK:
rq->cmd_flags |= REQ_SOFTBARRIER;
elv_drain_elevator(q);
list_add_tail(&rq->queuelist, &q->queue_head);
/*
* We kick the queue here for the following reasons.
* - The elevator might have returned NULL previously
* to delay requests and returned them now. As the
* queue wasn't empty before this request, ll_rw_blk
* won't run the queue on return, resulting in hang.
* - Usually, back inserted requests won't be merged
* with anything. There's no point in delaying queue
* processing.
*/
__blk_run_queue(q);
break;

So it is really not clear who should kick the queue and when. Though extra
kick won't harm, so to me it looks that blk_insert_cloned_reqeust()
should always kick the queue after inserting any request (Either back
insert for flush insert etc.).

According to above comment we kick the queue here as elevaor might have
returned NULL in the past despite have a request. If that's the case then
somebody should have setup a timer to dispatch that request in time. What
happens if next request does not come for next 10 seconds. This request
will be sitting there for a long time.

So to me, blk_insert_cloned_request() should not rely on queue kick being
provided by ELEVATOR_INSERT_BACK. It should always kick the queue after
inserting a request. (as tejun mentioned about blk_insert_request()
kicking the queue).

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