| Message ID | 1464187653-3754-3-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 25, 2016 at 08:47:33AM -0600, Tim Gardner wrote: > From: Keith Busch <keith.busch@intel.com> > > BugLink: http://bugs.launchpad.net/bugs/1581034 > > Go directly to ending a request if it wasn't started. Previously, completing a > request may invoke a driver callback for a request it didn't initialize. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > Reviewed-by: Sagi Grimberg <sagig@mellanox.com> > Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de> > Acked-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jens Axboe <axboe@fb.com> > (back ported from commit a59e0f5795fe52dad42a99c00287e3766153b312) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > block/blk-mq.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 93a4e19..3989b2d 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -599,8 +599,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, > * If a request wasn't started before the queue was > * marked dying, kill it here or it'll go unnoticed. > */ > - if (unlikely(blk_queue_dying(rq->q))) > - blk_mq_complete_request(rq, -EIO); > + if (unlikely(blk_queue_dying(rq->q))) { > + rq->errors = -EIO; > + blk_mq_complete_request(rq, rq->errors); > + Why does this call blk_mq_complete_request instead of blk_mq_end_request per the original commit? --chris } > return; > } > if (rq->cmd_flags & REQ_NO_TIMEOUT) > -- > 1.9.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 05/26/2016 06:58 AM, Christopher Arges wrote: > On Wed, May 25, 2016 at 08:47:33AM -0600, Tim Gardner wrote: >> From: Keith Busch <keith.busch@intel.com> >> >> BugLink: http://bugs.launchpad.net/bugs/1581034 >> >> Go directly to ending a request if it wasn't started. Previously, completing a >> request may invoke a driver callback for a request it didn't initialize. >> >> Signed-off-by: Keith Busch <keith.busch@intel.com> >> Reviewed-by: Sagi Grimberg <sagig@mellanox.com> >> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de> >> Acked-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Jens Axboe <axboe@fb.com> >> (back ported from commit a59e0f5795fe52dad42a99c00287e3766153b312) >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> --- >> block/blk-mq.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 93a4e19..3989b2d 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -599,8 +599,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, >> * If a request wasn't started before the queue was >> * marked dying, kill it here or it'll go unnoticed. >> */ >> - if (unlikely(blk_queue_dying(rq->q))) >> - blk_mq_complete_request(rq, -EIO); >> + if (unlikely(blk_queue_dying(rq->q))) { >> + rq->errors = -EIO; >> + blk_mq_complete_request(rq, rq->errors); >> + > > Why does this call blk_mq_complete_request instead of blk_mq_end_request per the > original commit? > This patch ('blk-mq: End unstarted requests on dying queue') came after commit a59e0f5795fe52dad42a99c00287e3766153b312 ('blk-mq: End unstarted requests on dying queue') which changes the call from blk_mq_complete_request(rq, -EIO) to blk_mq_end_request(rq, rq->errors). If you follow the chain of logic, blk_mq_complete_request() calls blk_mq_end_request() within the semantics of the driver. Calling blk_mq_end_request() might have unintended side effects. I chose to implement the simplest backport possible. rtg
On 05/26/2016 07:50 AM, Tim Gardner wrote: > On 05/26/2016 06:58 AM, Christopher Arges wrote: >> On Wed, May 25, 2016 at 08:47:33AM -0600, Tim Gardner wrote: >>> From: Keith Busch <keith.busch@intel.com> >>> >>> BugLink: http://bugs.launchpad.net/bugs/1581034 >>> >>> Go directly to ending a request if it wasn't started. Previously, completing a >>> request may invoke a driver callback for a request it didn't initialize. >>> >>> Signed-off-by: Keith Busch <keith.busch@intel.com> >>> Reviewed-by: Sagi Grimberg <sagig@mellanox.com> >>> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de> >>> Acked-by: Christoph Hellwig <hch@lst.de> >>> Signed-off-by: Jens Axboe <axboe@fb.com> >>> (back ported from commit a59e0f5795fe52dad42a99c00287e3766153b312) >>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >>> --- >>> block/blk-mq.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c >>> index 93a4e19..3989b2d 100644 >>> --- a/block/blk-mq.c >>> +++ b/block/blk-mq.c >>> @@ -599,8 +599,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, >>> * If a request wasn't started before the queue was >>> * marked dying, kill it here or it'll go unnoticed. >>> */ >>> - if (unlikely(blk_queue_dying(rq->q))) >>> - blk_mq_complete_request(rq, -EIO); >>> + if (unlikely(blk_queue_dying(rq->q))) { >>> + rq->errors = -EIO; >>> + blk_mq_complete_request(rq, rq->errors); >>> + >> >> Why does this call blk_mq_complete_request instead of blk_mq_end_request per the >> original commit? >> > This patch ('blk-mq: End unstarted requests on dying queue') came after > commit a59e0f5795fe52dad42a99c00287e3766153b312 ('blk-mq: End unstarted > requests on dying queue') which changes the call from > blk_mq_complete_request(rq, -EIO) to blk_mq_end_request(rq, rq->errors). > If you follow the chain of logic, blk_mq_complete_request() calls > blk_mq_end_request() within the semantics of the driver. Calling > blk_mq_end_request() might have unintended side effects. I chose to > implement the simplest backport possible. > Ugh, that was totally garbled. I'll resumbit with v2.
I added one scaffold patch ('NVMe: Requeue requests on suspended queues'), but
('NVMe: Move error handling to failed reset handler') still required some minor
backporting. ('blk-mq: End unstarted requests on dying queue') I left in its
pristine cherry-pick condition this time.
rtg
This is an ack for the 4 patches fixing this issue. --chris On Thu, May 26, 2016 at 10:25:47AM -0600, Tim Gardner wrote: > I added one scaffold patch ('NVMe: Requeue requests on suspended queues'), but > ('NVMe: Move error handling to failed reset handler') still required some minor > backporting. ('blk-mq: End unstarted requests on dying queue') I left in its > pristine cherry-pick condition this time. > > rtg > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Thu, May 26, 2016 at 10:25:47AM -0600, Tim Gardner wrote: > I added one scaffold patch ('NVMe: Requeue requests on suspended queues'), but > ('NVMe: Move error handling to failed reset handler') still required some minor > backporting. ('blk-mq: End unstarted requests on dying queue') I left in its > pristine cherry-pick condition this time. > > rtg Applied to X: [PATCH */4 Xenial SRU v2] NVMe: Fix namespace removal deadlock NVMe: Requeue requests on suspended queues NVMe: Move error handling to failed reset handler blk-mq: End unstarted requests on dying queue -Kamal
diff --git a/block/blk-mq.c b/block/blk-mq.c index 93a4e19..3989b2d 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -599,8 +599,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, * If a request wasn't started before the queue was * marked dying, kill it here or it'll go unnoticed. */ - if (unlikely(blk_queue_dying(rq->q))) - blk_mq_complete_request(rq, -EIO); + if (unlikely(blk_queue_dying(rq->q))) { + rq->errors = -EIO; + blk_mq_complete_request(rq, rq->errors); + } return; } if (rq->cmd_flags & REQ_NO_TIMEOUT)