| Message ID | 1448037342-18384-3-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Jens Axboe |
| Headers | show |
Christoph Hellwig <hch@lst.de> writes: > We only added the request to the request list for the !blk-mq case, > so we should only delete it in that case as well. Sorry, I don't follow. Do you mean the timeout_list? It appears to me that blk_add_timer is called for both the old and mq paths. -Jeff > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-timeout.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/block/blk-timeout.c b/block/blk-timeout.c > index 246dfb1..aa40aa9 100644 > --- a/block/blk-timeout.c > +++ b/block/blk-timeout.c > @@ -158,11 +158,13 @@ void blk_abort_request(struct request *req) > { > if (blk_mark_rq_complete(req)) > return; > - blk_delete_timer(req); > - if (req->q->mq_ops) > + > + if (req->q->mq_ops) { > blk_mq_rq_timed_out(req, false); > - else > + } else { > + blk_delete_timer(req); > blk_rq_timed_out(req); > + } > } > EXPORT_SYMBOL_GPL(blk_abort_request); -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/20/2015 02:43 PM, Jeff Moyer wrote: > Christoph Hellwig <hch@lst.de> writes: > >> We only added the request to the request list for the !blk-mq case, >> so we should only delete it in that case as well. > > Sorry, I don't follow. Do you mean the timeout_list? It appears to me > that blk_add_timer is called for both the old and mq paths. It is called for both, but only !mq adds it to the list. We don't need that tracking on the mq side, as we can look it up in our tags.
Jens Axboe <axboe@fb.com> writes: > On 11/20/2015 02:43 PM, Jeff Moyer wrote: >> Christoph Hellwig <hch@lst.de> writes: >> >>> We only added the request to the request list for the !blk-mq case, >>> so we should only delete it in that case as well. >> >> Sorry, I don't follow. Do you mean the timeout_list? It appears to me >> that blk_add_timer is called for both the old and mq paths. > > It is called for both, but only !mq adds it to the list. We don't need > that tracking on the mq side, as we can look it up in our tags. Yeah, just saw that. Thanks. -Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig <hch@lst.de> writes: > We only added the request to the request list for the !blk-mq case, > so we should only delete it in that case as well. I think I'd rather see the conditional moved into blk_delete_timer. Breaking the balance of add/delete seems ugly to me. I had expected to see a later patch make use of the list, but nothing does. So, this is just a cleanup (as opposed to a preparatory patch), yes ? -Jeff > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-timeout.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/block/blk-timeout.c b/block/blk-timeout.c > index 246dfb1..aa40aa9 100644 > --- a/block/blk-timeout.c > +++ b/block/blk-timeout.c > @@ -158,11 +158,13 @@ void blk_abort_request(struct request *req) > { > if (blk_mark_rq_complete(req)) > return; > - blk_delete_timer(req); > - if (req->q->mq_ops) > + > + if (req->q->mq_ops) { > blk_mq_rq_timed_out(req, false); > - else > + } else { > + blk_delete_timer(req); > blk_rq_timed_out(req); > + } > } > EXPORT_SYMBOL_GPL(blk_abort_request); -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 20, 2015 at 05:20:21PM -0500, Jeff Moyer wrote: > > We only added the request to the request list for the !blk-mq case, > > so we should only delete it in that case as well. > > I think I'd rather see the conditional moved into blk_delete_timer. > Breaking the balance of add/delete seems ugly to me. I had expected to > see a later patch make use of the list, but nothing does. So, this is > just a cleanup (as opposed to a preparatory patch), yes ? It was needed in an earlier version of the series. Note that it's still needed e.g. for libsas. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block/blk-timeout.c b/block/blk-timeout.c index 246dfb1..aa40aa9 100644 --- a/block/blk-timeout.c +++ b/block/blk-timeout.c @@ -158,11 +158,13 @@ void blk_abort_request(struct request *req) { if (blk_mark_rq_complete(req)) return; - blk_delete_timer(req); - if (req->q->mq_ops) + + if (req->q->mq_ops) { blk_mq_rq_timed_out(req, false); - else + } else { + blk_delete_timer(req); blk_rq_timed_out(req); + } } EXPORT_SYMBOL_GPL(blk_abort_request);
We only added the request to the request list for the !blk-mq case, so we should only delete it in that case as well. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-timeout.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)