Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
From: Keith Busch <kbusch@kernel.org>
To: Yuanyuan Zhong <yzhong@purestorage.com>
Cc: Chaitanya.Kulkarni@wdc.com, axboe@fb.com, hch@lst.de,
	sagi@grimberg.me, linux-nvme@lists.infradead.org,
	Casey Chen <cachen@purestorage.com>
Subject: Re: [PATCH v2] nvme-core: initialize status to NVME_SC_HOST_PATH_ERROR
Date: Thu, 15 Apr 2021 15:58:20 -0700
Message-ID: <20210415225820.GA2760266@dhcp-10-100-145-180.wdc.com> (raw)
In-Reply-To: <CA+AMecHdnFg73z8LkjTDzT-5y0dcApV9X-vJ9cDRMGBhkNrNtw@mail.gmail.com>

On Thu, Apr 15, 2021 at 03:11:13PM -0700, Yuanyuan Zhong wrote:
> > > It doesn't look like these types of errors are unique to nvme. Could we
> > > not just have blk_execute_rq() return an error instead?
> Looking at history of commit b7819b925918 ("block: remove the blk_execute_rq
> return value") and commit be549d491154 ("scsi: core: set result when the
> command cannot be dispatched"), it seems scsi code no longer have issue.

Those happened when 'struct request' carried an 'errors' field that a
caller could check. The struct doesn't have that anymore.

> > -extern void blk_execute_rq(struct gendisk *, struct request *, int);
> > +extern int blk_execute_rq(struct gendisk *, struct request *, int);
> Changing blk_execute_rq() prototype needs tree-wide callers update.

Callers aren't relying on an error now, so I don't think this suggestion
requires a tree-wide update. It might want an audit to see if existing
callers are susceptible to the same bug you found with NVMe, but
changing the return as suggested shouldn't introduce new issues.

> While it could be a fix, I'd wait for maintainers to chime in.

Yes, this does require concensus, but Linux is usually adaptable to
changing kernal APIs when it makes sense. I *think* my suggestion makes
sense, but I appreciate any feedback.
 
> I don't quite like initializing the status for every nvme command. However for
> such a long standing bug across multiple stable releases, I think it will be an
> easy backport for stable-tree.

Your proposal is easier for stable, however, the mainline fix doesn't
necessarily need to consider that. We can always deal with a back-port
if the mainline approach does not readily apply.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

      reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-12 23:45 [PATCH] " Yuanyuan Zhong
2021-04-13  3:19 ` Chaitanya Kulkarni
2021-04-14 17:08   ` [PATCH v2] " Yuanyuan Zhong
2021-04-15 19:30     ` Keith Busch
2021-04-15 20:17       ` Keith Busch
2021-04-15 22:11         ` Yuanyuan Zhong
2021-04-15 22:58           ` Keith Busch [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210415225820.GA2760266@dhcp-10-100-145-180.wdc.com \
    --to=kbusch@kernel.org \
    --cc=Chaitanya.Kulkarni@wdc.com \
    --cc=axboe@fb.com \
    --cc=cachen@purestorage.com \
    --cc=hch@lst.de \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    --cc=yzhong@purestorage.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git