| Message ID | 20211227164138.2488066-3-kbusch@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2,1/3] block: introduce rq_list_for_each_safe macro | expand |
> + rq_list_move(rqlist, &requeue_list, req, prev, next); > + > + req = prev; > + if (!req) > + continue; Shouldn't this be a break? > + *rqlist = next; > + prev = NULL; > + } else > + prev = req; > + } I wonder if a restart label here would be a little cleaner, something like: diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 992ee314e91ba..29b433fd12bdd 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -999,9 +999,11 @@ static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req) static void nvme_queue_rqs(struct request **rqlist) { - struct request *req, *next, *prev = NULL; + struct request *req, *next, *prev; struct request *requeue_list = NULL; +restart: + prev = NULL; rq_list_for_each_safe(rqlist, req, next) { struct nvme_queue *nvmeq = req->mq_hctx->driver_data; @@ -1009,9 +1011,9 @@ static void nvme_queue_rqs(struct request **rqlist) /* detach 'req' and add to remainder list */ rq_list_move(rqlist, &requeue_list, req, prev, next); + if (!prev) + break; req = prev; - if (!req) - continue; } if (!next || req->mq_hctx != next->mq_hctx) { @@ -1019,9 +1021,9 @@ static void nvme_queue_rqs(struct request **rqlist) req->rq_next = NULL; nvme_submit_cmds(nvmeq, rqlist); *rqlist = next; - prev = NULL; - } else - prev = req; + goto restart; + } + prev = req; } *rqlist = requeue_list;
On Wed, Dec 29, 2021 at 06:46:02PM +0100, Christoph Hellwig wrote: > > + rq_list_move(rqlist, &requeue_list, req, prev, next); > > + > > + req = prev; > > + if (!req) > > + continue; > > Shouldn't this be a break? The condition just means we're at the beginning of the rqlist. There may be more requests to consider, so we have to continue. Or are you saying any failed prep should just abandon the batched sequence? If so, we would need to concat the return list with the rest of rqlist before breaking. > > + *rqlist = next; > > + prev = NULL; > > + } else > > + prev = req; > > + } > > I wonder if a restart label here would be a little cleaner, something > like: I applied your suggestion to give it a look, and I agree. Will use that for the next version.
On Wed, Dec 29, 2021 at 01:04:46PM -0800, Keith Busch wrote: > On Wed, Dec 29, 2021 at 06:46:02PM +0100, Christoph Hellwig wrote: > > > + rq_list_move(rqlist, &requeue_list, req, prev, next); > > > + > > > + req = prev; > > > + if (!req) > > > + continue; > > > > Shouldn't this be a break? > > The condition just means we're at the beginning of the rqlist. There may > be more requests to consider, so we have to continue. > > Or are you saying any failed prep should just abandon the batched > sequence? If so, we would need to concat the return list with the rest > of rqlist before breaking. No, I misunderstood the check,
On Wed, Dec 29, 2021 at 06:46:02PM +0100, Christoph Hellwig wrote: > I wonder if a restart label here would be a little cleaner, something > like: On second thought, this requires another check that the *rqlist is not NULL before 'goto restart' since the safe iterator dereferences it. I'm not sure this is cleaner than the original anymore. > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 992ee314e91ba..29b433fd12bdd 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -999,9 +999,11 @@ static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req) > > static void nvme_queue_rqs(struct request **rqlist) > { > - struct request *req, *next, *prev = NULL; > + struct request *req, *next, *prev; > struct request *requeue_list = NULL; > > +restart: > + prev = NULL; > rq_list_for_each_safe(rqlist, req, next) { > struct nvme_queue *nvmeq = req->mq_hctx->driver_data; > > @@ -1009,9 +1011,9 @@ static void nvme_queue_rqs(struct request **rqlist) > /* detach 'req' and add to remainder list */ > rq_list_move(rqlist, &requeue_list, req, prev, next); > > + if (!prev) > + break; > req = prev; > - if (!req) > - continue; > } > > if (!next || req->mq_hctx != next->mq_hctx) { > @@ -1019,9 +1021,9 @@ static void nvme_queue_rqs(struct request **rqlist) > req->rq_next = NULL; > nvme_submit_cmds(nvmeq, rqlist); > *rqlist = next; > - prev = NULL; > - } else > - prev = req; > + goto restart; > + } > + prev = req; > } > > *rqlist = requeue_list;
On Tue, Jan 04, 2022 at 11:38:07AM -0800, Keith Busch wrote: > On Wed, Dec 29, 2021 at 06:46:02PM +0100, Christoph Hellwig wrote: > > I wonder if a restart label here would be a little cleaner, something > > like: > > On second thought, this requires another check that the *rqlist is not > NULL before 'goto restart' since the safe iterator dereferences it. I'm > not sure this is cleaner than the original anymore. Ok, let's stick to the original then.
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 50deb8b69c40..992ee314e91b 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -999,30 +999,30 @@ static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req) static void nvme_queue_rqs(struct request **rqlist) { - struct request *req = rq_list_peek(rqlist), *prev = NULL; + struct request *req, *next, *prev = NULL; struct request *requeue_list = NULL; - do { + rq_list_for_each_safe(rqlist, req, next) { struct nvme_queue *nvmeq = req->mq_hctx->driver_data; if (!nvme_prep_rq_batch(nvmeq, req)) { /* detach 'req' and add to remainder list */ - if (prev) - prev->rq_next = req->rq_next; - rq_list_add(&requeue_list, req); - } else { - prev = req; + rq_list_move(rqlist, &requeue_list, req, prev, next); + + req = prev; + if (!req) + continue; } - req = rq_list_next(req); - if (!req || (prev && req->mq_hctx != prev->mq_hctx)) { + if (!next || req->mq_hctx != next->mq_hctx) { /* detach rest of list, and submit */ - if (prev) - prev->rq_next = NULL; + req->rq_next = NULL; nvme_submit_cmds(nvmeq, rqlist); - *rqlist = req; - } - } while (req); + *rqlist = next; + prev = NULL; + } else + prev = req; + } *rqlist = requeue_list; }
If command prep fails, current handling will orphan subsequent requests in the list. Consider a simple example: rqlist = [ 1 -> 2 ] When prep for request '1' fails, it will be appended to the 'requeue_list', leaving request '2' disconnected from the original rqlist and no longer tracked. Meanwhile, rqlist is still pointing to the failed request '1' and will attempt to submit an unprepped command. Fix this by updating the rqlist accordingly using the request list helper functions. Fixes: d62cbcf62f2f ("nvme: add support for mq_ops->queue_rqs()") Signed-off-by: Keith Busch <kbusch@kernel.org> --- v1->v2: Replaced the backward looking iterator with the forward looking version implemented in PATCH 1/3. This is a little easier to read. Replaced the driver's list manipulation with the helper function provided in PATCH 2/3. drivers/nvme/host/pci.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)