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 [thread overview]
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
prev parent reply other threads:[~2021-04-15 22:58 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-12 23:45 [PATCH] nvme-core: initialize status to NVME_SC_HOST_PATH_ERROR 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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).