| Message ID | 20210416165353.3088547-2-kbusch@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] block: return errors from blk_execute_rq() | expand |
> if (poll) > nvme_execute_rq_polled(req->q, NULL, req, at_head); You may need to audit other completion handlers for blk_execute_rq_nowait(). How to get error ret from polled rq? > else > - blk_execute_rq(NULL, req, at_head); > + ret = blk_execute_rq(NULL, req, at_head); > if (result) > *result = nvme_req(req)->result; > if (nvme_req(req)->flags & NVME_REQ_CANCELLED) > ret = -EINTR; > - else > + else if (nvme_req(req)->status) > ret = nvme_req(req)->status;
On Fri, Apr 16, 2021 at 10:12:11AM -0700, Yuanyuan Zhong wrote: > > if (poll) > > nvme_execute_rq_polled(req->q, NULL, req, at_head); > You may need to audit other completion handlers for blk_execute_rq_nowait(). Why? Those callers already provide their own callback that directly get the error. > How to get error ret from polled rq? Please see nvme_end_sync_rq() for that driver's polled handler callback. It already has the error.
On 4/16/21 09:54, Keith Busch wrote: > We don't have an nvme status to report if the driver's .queue_rq() > returns an error without dispatching the requested nvme command. Use the > return value from blk_execute_rq() so the caller may know their command > was not successful. > > Reported-by: Yuanyuan Zhong <yzhong@purestorage.com> > Signed-off-by: Keith Busch <kbusch@kernel.org> Looks good. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On Sat, Apr 17, 2021 at 02:17:35AM +0900, Keith Busch wrote: > On Fri, Apr 16, 2021 at 10:12:11AM -0700, Yuanyuan Zhong wrote: > > > if (poll) > > > nvme_execute_rq_polled(req->q, NULL, req, at_head); > > You may need to audit other completion handlers for blk_execute_rq_nowait(). > > Why? Those callers already provide their own callback that directly get > the error. > > > How to get error ret from polled rq? > > Please see nvme_end_sync_rq() for that driver's polled handler callback. > It already has the error. But it never looks at it..
On Mon, Apr 19, 2021 at 09:16:05AM +0200, Christoph Hellwig wrote: > On Sat, Apr 17, 2021 at 02:17:35AM +0900, Keith Busch wrote: > > On Fri, Apr 16, 2021 at 10:12:11AM -0700, Yuanyuan Zhong wrote: > > > > if (poll) > > > > nvme_execute_rq_polled(req->q, NULL, req, at_head); > > > You may need to audit other completion handlers for blk_execute_rq_nowait(). > > > > Why? Those callers already provide their own callback that directly get > > the error. > > > > > How to get error ret from polled rq? > > > > Please see nvme_end_sync_rq() for that driver's polled handler callback. > > It already has the error. > > But it never looks at it.. The question was how to get ret. I didn't mean to imply the example was actually using it. :)
On Mon, Apr 19, 2021 at 8:14 AM Keith Busch <kbusch@kernel.org> wrote: > > On Mon, Apr 19, 2021 at 09:16:05AM +0200, Christoph Hellwig wrote: > > On Sat, Apr 17, 2021 at 02:17:35AM +0900, Keith Busch wrote: > > > On Fri, Apr 16, 2021 at 10:12:11AM -0700, Yuanyuan Zhong wrote: > > > > > if (poll) > > > > > nvme_execute_rq_polled(req->q, NULL, req, at_head); > > > > You may need to audit other completion handlers for blk_execute_rq_nowait(). > > > > > > Why? Those callers already provide their own callback that directly get > > > the error. See below clarification. I was wondering whether the way you were going to propose for nvme_end_sync_rq() would establish new convention for other blk_execute_rq_nowait() completion handlers implementation. > > > > > > > How to get error ret from polled rq? > > > > > > Please see nvme_end_sync_rq() for that driver's polled handler callback. > > > It already has the error. > > > > But it never looks at it.. > > The question was how to get ret. I didn't mean to imply the example was > actually using it. :) The question was how to let nvme_end_sync_rq() propagate the blk_status_t error to the ret for __nvme_submit_sync_cmd(). That's part of the problem here: __nvme_submit_sync_cmd() may return success for a command that failed submission.
On Mon, Apr 19, 2021 at 10:27:42AM -0700, Yuanyuan Zhong wrote: > On Mon, Apr 19, 2021 at 8:14 AM Keith Busch <kbusch@kernel.org> wrote: > > > > On Mon, Apr 19, 2021 at 09:16:05AM +0200, Christoph Hellwig wrote: > > > On Sat, Apr 17, 2021 at 02:17:35AM +0900, Keith Busch wrote: > > > > On Fri, Apr 16, 2021 at 10:12:11AM -0700, Yuanyuan Zhong wrote: > > > > > > if (poll) > > > > > > nvme_execute_rq_polled(req->q, NULL, req, at_head); > > > > > You may need to audit other completion handlers for blk_execute_rq_nowait(). > > > > > > > > Why? Those callers already provide their own callback that directly get > > > > the error. > > See below clarification. I was wondering whether the way you were going to > propose for nvme_end_sync_rq() would establish new convention for other > blk_execute_rq_nowait() completion handlers implementation. I'm not sure it can be turned into a common pattern. The callbacks are implementation specific. > > > > > > > > > How to get error ret from polled rq? > > > > > > > > Please see nvme_end_sync_rq() for that driver's polled handler callback. > > > > It already has the error. > > > > > > But it never looks at it.. > > > > The question was how to get ret. I didn't mean to imply the example was > > actually using it. :) > > The question was how to let nvme_end_sync_rq() propagate the blk_status_t > error to the ret for __nvme_submit_sync_cmd(). That's part of the problem > here: __nvme_submit_sync_cmd() may return success for a command that > failed submission. --- diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index b57157106cac..a0fb9ad132af 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -949,6 +949,9 @@ static void nvme_end_sync_rq(struct request *rq, blk_status_t error) struct completion *waiting = rq->end_io_data; rq->end_io_data = NULL; + if (error && !nvme_req(rq)->status) + nvme_req(rq)->status = blk_status_to_errno(error); complete(waiting); } --
On Mon, Apr 19, 2021 at 10:48 AM Keith Busch <kbusch@kernel.org> wrote: > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index b57157106cac..a0fb9ad132af 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -949,6 +949,9 @@ static void nvme_end_sync_rq(struct request *rq, blk_status_t error) > struct completion *waiting = rq->end_io_data; > > rq->end_io_data = NULL; > + if (error && !nvme_req(rq)->status) Is setting nvme_req(rq)->status for each error return in ->queue_rq() going to gradually roll out, and eventually skipping the fallback here? > + nvme_req(rq)->status = blk_status_to_errno(error); Casting int negative errno to u16 will get 0xfff., meaning NVME_SC_DNR is set. Is that always right? -- Regards, Yuanyuan Zhong
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 85538c38aae9..b57157106cac 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1000,12 +1000,12 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, if (poll) nvme_execute_rq_polled(req->q, NULL, req, at_head); else - blk_execute_rq(NULL, req, at_head); + ret = blk_execute_rq(NULL, req, at_head); if (result) *result = nvme_req(req)->result; if (nvme_req(req)->flags & NVME_REQ_CANCELLED) ret = -EINTR; - else + else if (nvme_req(req)->status) ret = nvme_req(req)->status; out: blk_mq_free_request(req);
We don't have an nvme status to report if the driver's .queue_rq() returns an error without dispatching the requested nvme command. Use the return value from blk_execute_rq() so the caller may know their command was not successful. Reported-by: Yuanyuan Zhong <yzhong@purestorage.com> Signed-off-by: Keith Busch <kbusch@kernel.org> --- drivers/nvme/host/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)