This patch causes boot failures when using REQ_FLUSH requests. Also the
following statement in the commit log:
For non mq calls, the block layer will free the bios when
blk_finish_request is called.
For mq calls, the blk mq code wants the caller to do this.
Seems incorrect as far as I can follow the code as blk_finish_request only
calls __blk_put_request which then completes the bios if req->end_io is
not set. This matches the blk-mq behaviour before this patch, so reverting
it makes the code more similar to the legacy case in addition to fixing the
boot failure.
Signed-off-by: Christoph Hellwig <[email protected]>
---
block/blk-flush.c | 2 +-
block/blk-mq.c | 14 ++++++++++----
block/blk-mq.h | 1 +
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/block/blk-flush.c b/block/blk-flush.c
index 3e4cc9c..c56c37d 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -231,7 +231,7 @@ static void flush_end_io(struct request *flush_rq, int error)
unsigned long flags = 0;
if (q->mq_ops) {
- blk_mq_free_request(flush_rq);
+ blk_mq_finish_request(flush_rq, error);
spin_lock_irqsave(&q->mq_flush_lock, flags);
}
running = &q->flush_queue[q->flush_running_idx];
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 93563e0..423089d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -270,7 +270,7 @@ void blk_mq_free_request(struct request *rq)
}
EXPORT_SYMBOL(blk_mq_free_request);
-static void blk_mq_finish_request(struct request *rq, int error)
+void blk_mq_finish_request(struct request *rq, int error)
{
struct bio *bio = rq->bio;
unsigned int bytes = 0;
@@ -286,17 +286,22 @@ static void blk_mq_finish_request(struct request *rq, int error)
blk_account_io_completion(rq, bytes);
blk_account_io_done(rq);
+ blk_mq_free_request(rq);
}
void blk_mq_complete_request(struct request *rq, int error)
{
trace_block_rq_complete(rq->q, rq);
- blk_mq_finish_request(rq, error);
+ /*
+ * If ->end_io is set, it's responsible for doing the rest of the
+ * completion.
+ */
if (rq->end_io)
rq->end_io(rq, error);
else
- blk_mq_free_request(rq);
+ blk_mq_finish_request(rq, error);
+
}
void __blk_mq_end_io(struct request *rq, int error)
@@ -984,7 +989,8 @@ int blk_mq_execute_rq(struct request_queue *q, struct request *rq)
if (rq->errors)
err = -EIO;
- blk_mq_free_request(rq);
+ blk_mq_finish_request(rq, rq->errors);
+
return err;
}
EXPORT_SYMBOL(blk_mq_execute_rq);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 52bf1f9..42d0110 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -27,6 +27,7 @@ void blk_mq_complete_request(struct request *rq, int error);
void blk_mq_run_request(struct request *rq, bool run_queue, bool async);
void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
void blk_mq_init_flush(struct request_queue *q);
+void blk_mq_finish_request(struct request *rq, int error);
/*
* CPU hotplug helpers
--
1.7.10.4
On 10/4/13 8:49 AM, Christoph Hellwig wrote:
> This patch causes boot failures when using REQ_FLUSH requests. Also the
> following statement in the commit log:
>
> For non mq calls, the block layer will free the bios when
> blk_finish_request is called.
Sorry, messed up function name. I meant blk_end_request*.
For blk_execute_rq_nowait/blk_execute_rq and normal request use, the
lower levels free the bios as they are completed by one of the
blk_finish_request* calls. The caller of of
blk_execute_rq_nowait/blk_execute_rq does not have to worry about
freeing bios. It just frees the request when it is done with it.
>
> For mq calls, the blk mq code wants the caller to do this.
The problem was that Nick's scsi-mq code added a async
blk_mq_execute_rq support and it did not complete bios in the rq->end_io
callback because he most likely expected it to work like the non-mq case
above.
The goal of my patch was to make the underlying behavior and the
function naming similar.
If we can get the the core block code to finish the bios as they are
completed like is done in the non mq case like I was trying to do, then
with your other patches in this set we do not need to have the rq end_io
callbacks have this:
> if (q->mq_ops) {
> - blk_mq_free_request(flush_rq);
> + blk_mq_finish_request(flush_rq, error);
> spin_lock_irqsave(&q->mq_flush_lock, flags);
The block code would free the bios no matter who sent it and mq type,
and the callers of blk_execute* would just call blk_put_request.
On Fri, Oct 04, 2013 at 12:39:33PM -0500, Mike Christie wrote:
> Sorry, messed up function name. I meant blk_end_request*.
>
> For blk_execute_rq_nowait/blk_execute_rq and normal request use, the
> lower levels free the bios as they are completed by one of the
> blk_finish_request* calls. The caller of of
> blk_execute_rq_nowait/blk_execute_rq does not have to worry about
> freeing bios. It just frees the request when it is done with it.
Are you talking about bios or requests? All these functions deal with
requests, so the talk of bios really confuses me.
That beeing said the old ones all require the caller to free the
request, and complicate that with the useless refcounting that my patch
3 removes. Take a look at the other patches how all the calling
conventions can be nicely unified.
On 10/05/2013 05:50 AM, Christoph Hellwig wrote:
> On Fri, Oct 04, 2013 at 12:39:33PM -0500, Mike Christie wrote:
>> Sorry, messed up function name. I meant blk_end_request*.
>>
>> For blk_execute_rq_nowait/blk_execute_rq and normal request use, the
>> lower levels free the bios as they are completed by one of the
>> blk_finish_request* calls. The caller of of
>> blk_execute_rq_nowait/blk_execute_rq does not have to worry about
>> freeing bios. It just frees the request when it is done with it.
>
> Are you talking about bios or requests? All these functions deal with
> requests, so the talk of bios really confuses me.
The functions take in requests as the argument, but they end up
operating on bios too. The scsi layer does
scsi_io_completion->scsi_end_request-> blk_end_request ->
blk_end_bidi_request -> blk_update_bidi_request -> blk_update_request.
That function will then complete bios on the request passed in. It does
not matter if the request is a REQ_TYPE_FS or REQ_TYPE_BLOCK_PC.
With my patch I was trying to make the block layer do the same for mq
REQ_TYPE_BLOCK_PC requests inserted into the queue with
blk_execute_rq_nowait (Nick's patch had support for something like that)
by having the block mq layer call blk_mq_finish_request instead of
making the code that calls blk_execute_rq_nowait do it.
>
> That beeing said the old ones all require the caller to free the
> request, and complicate that with the useless refcounting that my patch
> 3 removes. Take a look at the other patches how all the calling
> conventions can be nicely unified.
I agree. I like them. I am saying though it could be better because even
with your patches the rq->end_io functions need to have the mq_ops check
like the flush_end_io does. If my patch worked as intended we would have
your improvements and we would not need the rq->end_io functions to have
that check and call blk_mq_finish_request because the blk mq layer was
doing it for them.
That is all I am saying. I would like to be able to remove that check
and blk_mq_finish_request call from the rq->end_io callouts.
On Sat, Oct 05, 2013 at 03:20:05PM -0500, Mike Christie wrote:
> The functions take in requests as the argument, but they end up
> operating on bios too. The scsi layer does
> scsi_io_completion->scsi_end_request-> blk_end_request ->
> blk_end_bidi_request -> blk_update_bidi_request -> blk_update_request.
> That function will then complete bios on the request passed in. It does
> not matter if the request is a REQ_TYPE_FS or REQ_TYPE_BLOCK_PC.
>
> With my patch I was trying to make the block layer do the same for mq
> REQ_TYPE_BLOCK_PC requests inserted into the queue with
> blk_execute_rq_nowait (Nick's patch had support for something like that)
> by having the block mq layer call blk_mq_finish_request instead of
> making the code that calls blk_execute_rq_nowait do it.
Ok, I get the point now. Didn't see that issue because I've only been
testing the non-bio REQ_TYPE_BLOCK_PC case as exposed by the virtio-blk
serial attribute so far.
> > That beeing said the old ones all require the caller to free the
> > request, and complicate that with the useless refcounting that my patch
> > 3 removes. Take a look at the other patches how all the calling
> > conventions can be nicely unified.
>
> I agree. I like them. I am saying though it could be better because even
> with your patches the rq->end_io functions need to have the mq_ops check
> like the flush_end_io does. If my patch worked as intended we would have
> your improvements and we would not need the rq->end_io functions to have
> that check and call blk_mq_finish_request because the blk mq layer was
> doing it for them.
>
> That is all I am saying. I would like to be able to remove that check
> and blk_mq_finish_request call from the rq->end_io callouts.
I've found the bug that caused the regression with your patch, it's that
blk-mq incorrectly completes bios midway through a flush sequence, while
the old path didn't.
I'll send out a series soon that fixes this and re-reverts your patch,
although I split that re-revert into two patches in addition to my new
fix, given that I think your mixing up of the blk_mq_finish_request /
blk_mq_free_spit plus the confusing description made it really hard to
grasp, but I'll leave it to Jens how he wants to apply it to his tree
and make it look after the next rebase.