[PATCH] nvme: ignore starting sector while error logging for passthrough requests

[PATCH] nvme: ignore starting sector while error logging for passthrough requests

[Pankaj Raghav]

Error log will print a garbage value as the starting LBA for passthrough
requests and requests with uninitialized starting sector that return an
error. Print LBA as 0 instead for these requests. This behaviour is
similar what tracing does for these type of requests.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 drivers/nvme/host/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 059737c1a2c1..41e0a257f1c2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -305,6 +305,13 @@ static void nvme_retry_req(struct request *req)
 	blk_mq_delay_kick_requeue_list(req->q, delay);
 }
 
+static inline u64 nvme_log_error_rq_lba(struct nvme_ns *ns, struct request *rq)
+{
+	if (blk_rq_is_passthrough(rq) || blk_rq_pos(rq) == (sector_t)-1)
+		return 0;
+	return nvme_sect_to_lba(ns, blk_rq_pos(rq));
+}
+
 static void nvme_log_error(struct request *req)
 {
 	struct nvme_ns *ns = req->q->queuedata;
@@ -315,7 +322,7 @@ static void nvme_log_error(struct request *req)
 		       ns->disk ? ns->disk->disk_name : "?",
 		       nvme_get_opcode_str(nr->cmd->common.opcode),
 		       nr->cmd->common.opcode,
-		       (unsigned long long)nvme_sect_to_lba(ns, blk_rq_pos(req)),
+		       (unsigned long long)nvme_log_error_rq_lba(ns, req),
 		       (unsigned long long)blk_rq_bytes(req) >> ns->lba_shift,
 		       nvme_get_error_status_str(nr->status),
 		       nr->status >> 8 & 7,	/* Status Code Type */
-- 
2.25.1

Re: [PATCH] nvme: ignore starting sector while error logging for passthrough requests

[Pankaj Raghav]

The final consensus in Daniel's patch[1] seems to be adding an opt in to
log error only for admin commands, and logging error for user IO commands
will remain as is.

In that case, should we reconsider this patch to avoid printing a garbage
value ((sector_t)-1) while logging error for IO passthrough commands?

[1]
https://lore.kernel.org/all/79E937D0-58DA-4AE2-A0B8-A30185CE7FCE@oracle.com/

On 2022-10-06 11:10, Pankaj Raghav wrote:
> Error log will print a garbage value as the starting LBA for passthrough
> requests and requests with uninitialized starting sector that return an
> error. Print LBA as 0 instead for these requests. This behaviour is
> similar what tracing does for these type of requests.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>  drivers/nvme/host/core.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 059737c1a2c1..41e0a257f1c2 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -305,6 +305,13 @@ static void nvme_retry_req(struct request *req)
>  	blk_mq_delay_kick_requeue_list(req->q, delay);
>  }
>  
> +static inline u64 nvme_log_error_rq_lba(struct nvme_ns *ns, struct request *rq)
> +{
> +	if (blk_rq_is_passthrough(rq) || blk_rq_pos(rq) == (sector_t)-1)
> +		return 0;
> +	return nvme_sect_to_lba(ns, blk_rq_pos(rq));
> +}
> +
>  static void nvme_log_error(struct request *req)
>  {
>  	struct nvme_ns *ns = req->q->queuedata;
> @@ -315,7 +322,7 @@ static void nvme_log_error(struct request *req)
>  		       ns->disk ? ns->disk->disk_name : "?",
>  		       nvme_get_opcode_str(nr->cmd->common.opcode),
>  		       nr->cmd->common.opcode,
> -		       (unsigned long long)nvme_sect_to_lba(ns, blk_rq_pos(req)),
> +		       (unsigned long long)nvme_log_error_rq_lba(ns, req),
>  		       (unsigned long long)blk_rq_bytes(req) >> ns->lba_shift,
>  		       nvme_get_error_status_str(nr->status),
>  		       nr->status >> 8 & 7,	/* Status Code Type */

Re: [PATCH] nvme: ignore starting sector while error logging for passthrough requests

[Christoph Hellwig]

On Tue, Oct 18, 2022 at 03:48:57PM +0200, Pankaj Raghav wrote:
> The final consensus in Daniel's patch[1] seems to be adding an opt in to
> log error only for admin commands, and logging error for user IO commands
> will remain as is.

Hmm, is it?  I'd think even for I/O commands the opt in makes some
sense.

> In that case, should we reconsider this patch to avoid printing a garbage
> value ((sector_t)-1) while logging error for IO passthrough commands?

We should probably do this, but it should probably go along with the
opt-in for I/O commands.

Re: [PATCH] nvme: ignore starting sector while error logging for passthrough requests

[Keith Busch]

On Tue, Oct 18, 2022 at 04:45:35PM +0200, Christoph Hellwig wrote:
> On Tue, Oct 18, 2022 at 03:48:57PM +0200, Pankaj Raghav wrote:
> > The final consensus in Daniel's patch[1] seems to be adding an opt in to
> > log error only for admin commands, and logging error for user IO commands
> > will remain as is.
> 
> Hmm, is it?  I'd think even for I/O commands the opt in makes some
> sense.
> 
> > In that case, should we reconsider this patch to avoid printing a garbage
> > value ((sector_t)-1) while logging error for IO passthrough commands?
> 
> We should probably do this, but it should probably go along with the
> opt-in for I/O commands.

What's the preferred opt-in mechanism? If we want to do it per-command,
we have an unsused 'flags' field in the ioctl structures that we can
start defining options for these kinds of things. If we don't want that
level of fine grained control, we could set it at the controller or
module level with a new sysfs file, though I'm not sure this warrants
creating new exports there.

Re: [PATCH] nvme: ignore starting sector while error logging for passthrough requests

[Christoph Hellwig]

On Tue, Oct 18, 2022 at 09:23:37AM -0600, Keith Busch wrote:
> What's the preferred opt-in mechanism? If we want to do it per-command,
> we have an unsused 'flags' field in the ioctl structures that we can
> start defining options for these kinds of things. If we don't want that
> level of fine grained control, we could set it at the controller or
> module level with a new sysfs file, though I'm not sure this warrants
> creating new exports there.

The flags field is what I had in mind.  There's also another thing
on the plate as some folks really want to opt into kernel error
handling for passthrough commands, and that seems to be the only
way to do it.

Re: [PATCH] nvme: ignore starting sector while error logging for passthrough requests

[Pankaj Raghav]

>> In that case, should we reconsider this patch to avoid printing a garbage
>> value ((sector_t)-1) while logging error for IO passthrough commands?
> 
> We should probably do this, but it should probably go along with the
> opt-in for I/O commands.

Hmm, I think this patch will be orthogonal unless we put some of the opt-in
logic in the nvme_log_error() function itself.

Currently, we always log the error with the -1 sector value for passthrough
requests, which should be changed to print 0 so that we remain consistent
with the tracing infra. The same behavior should also be applicable when we
have the opt-in support, and the user decides to opt-in to log error for
passthrough requests.

That said, I am fine with this update going along with the opt-in support.