The synchronous blk_execute_rq() had not provided a way for its callers to know if its request was successful or not. Return the errno from the completion status. Reported-by: Yuanyuan Zhong <yzhong@purestorage.com> Signed-off-by: Keith Busch <kbusch@kernel.org> --- block/blk-exec.c | 6 ++++-- include/linux/blkdev.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/block/blk-exec.c b/block/blk-exec.c index beae70a0e5e5..3877a2677dd4 100644 --- a/block/blk-exec.c +++ b/block/blk-exec.c @@ -21,7 +21,7 @@ static void blk_end_sync_rq(struct request *rq, blk_status_t error) { struct completion *waiting = rq->end_io_data; - rq->end_io_data = NULL; + rq->end_io_data = ERR_PTR(blk_status_to_errno(error)); /* * complete last, if this is a stack request the process (and thus @@ -72,8 +72,9 @@ EXPORT_SYMBOL_GPL(blk_execute_rq_nowait); * Description: * Insert a fully prepared request at the back of the I/O scheduler queue * for execution and wait for completion. + * Return: The errno value of the blk_status_t provided to blk_mq_end_request(). */ -void blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head) +int blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head) { DECLARE_COMPLETION_ONSTACK(wait); unsigned long hang_check; @@ -87,5 +88,6 @@ void blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head) while (!wait_for_completion_io_timeout(&wait, hang_check * (HZ/2))); else wait_for_completion_io(&wait); + return PTR_ERR_OR_ZERO(rq->end_io_data); } EXPORT_SYMBOL(blk_execute_rq); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index b91ba6207365..15e4ffac33af 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -938,7 +938,7 @@ extern int blk_rq_map_kern(struct request_queue *, struct request *, void *, uns extern int blk_rq_map_user_iov(struct request_queue *, struct request *, struct rq_map_data *, const struct iov_iter *, gfp_t); -extern void blk_execute_rq(struct gendisk *, struct request *, int); +extern int blk_execute_rq(struct gendisk *, struct request *, int); extern void blk_execute_rq_nowait(struct gendisk *, struct request *, int, rq_end_io_fn *); -- 2.25.4
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(-) 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); -- 2.25.4
> 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; -- Regards, Yuanyuan Zhong
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:
> +extern int blk_execute_rq(struct gendisk *, struct request *, int);
I've not checked if it will work or not, but we have been dropping
the extern keywords for the new declarations and I'm not sure if that
applies here or not, so looks good to me.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
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. -- Regards, Yuanyuan Zhong
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
The synchronous blk_execute_rq() had not provided a way for its callers to know if its request was successful or not. Return the errno from the completion status. Reported-by: Yuanyuan Zhong <yzhong@purestorage.com> Signed-off-by: Keith Busch <kbusch@kernel.org> --- block/blk-exec.c | 6 ++++-- include/linux/blkdev.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/block/blk-exec.c b/block/blk-exec.c index beae70a0e5e5..3877a2677dd4 100644 --- a/block/blk-exec.c +++ b/block/blk-exec.c @@ -21,7 +21,7 @@ static void blk_end_sync_rq(struct request *rq, blk_status_t error) { struct completion *waiting = rq->end_io_data; - rq->end_io_data = NULL; + rq->end_io_data = ERR_PTR(blk_status_to_errno(error)); /* * complete last, if this is a stack request the process (and thus @@ -72,8 +72,9 @@ EXPORT_SYMBOL_GPL(blk_execute_rq_nowait); * Description: * Insert a fully prepared request at the back of the I/O scheduler queue * for execution and wait for completion. + * Return: The errno value of the blk_status_t provided to blk_mq_end_request(). */ -void blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head) +int blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head) { DECLARE_COMPLETION_ONSTACK(wait); unsigned long hang_check; @@ -87,5 +88,6 @@ void blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head) while (!wait_for_completion_io_timeout(&wait, hang_check * (HZ/2))); else wait_for_completion_io(&wait); + return PTR_ERR_OR_ZERO(rq->end_io_data); } EXPORT_SYMBOL(blk_execute_rq); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index b91ba6207365..15e4ffac33af 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -938,7 +938,7 @@ extern int blk_rq_map_kern(struct request_queue *, struct request *, void *, uns extern int blk_rq_map_user_iov(struct request_queue *, struct request *, struct rq_map_data *, const struct iov_iter *, gfp_t); -extern void blk_execute_rq(struct gendisk *, struct request *, int); +extern int blk_execute_rq(struct gendisk *, struct request *, int); extern void blk_execute_rq_nowait(struct gendisk *, struct request *, int, rq_end_io_fn *); -- 2.25.4
Sorry, please disregard this series. This came from the directory of the first version of this patch, but I intended to send the v2.
On Fri, Apr 23, 2021 at 02:57:59PM -0700, Keith Busch wrote: > The synchronous blk_execute_rq() had not provided a way for its callers > to know if its request was successful or not. Return the errno from the > completion status. > > Reported-by: Yuanyuan Zhong <yzhong@purestorage.com> > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > block/blk-exec.c | 6 ++++-- > include/linux/blkdev.h | 2 +- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/block/blk-exec.c b/block/blk-exec.c > index beae70a0e5e5..3877a2677dd4 100644 > --- a/block/blk-exec.c > +++ b/block/blk-exec.c > @@ -21,7 +21,7 @@ static void blk_end_sync_rq(struct request *rq, blk_status_t error) > { > struct completion *waiting = rq->end_io_data; > > - rq->end_io_data = NULL; > + rq->end_io_data = ERR_PTR(blk_status_to_errno(error)); I think we should propagate the blk_status_t here as we're entirely inside the block layer. I.e. declare a blk_status_t on-stack in blk_execute_rq and pass a pointer to it in ->end_io_data. > -extern void blk_execute_rq(struct gendisk *, struct request *, int); > +extern int blk_execute_rq(struct gendisk *, struct request *, int); It would be nice to drop the extern here and spell out the argument names.