From: Logan Gunthorpe <logang@deltatee.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@fb.com>, Sagi Grimberg <sagi@grimberg.me>,
Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>,
linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
Stephen Bates <sbates@raithlin.com>,
linux-block@vger.kernel.org, Keith Busch <kbusch@kernel.org>,
linux-fsdevel@vger.kernel.org, Max Gurtovoy <maxg@mellanox.com>
Subject: Re: [PATCH v9 05/12]
Date: Fri, 25 Oct 2019 17:09:56 -0600 [thread overview]
Message-ID: <247eca47-c3bc-6452-fb19-f7aa27b05a60@deltatee.com> (raw)
In-Reply-To: <20191010123406.GC28921@lst.de>
Hey,
Ok, I've got much of the work in progress: anything I haven't mentioned
below I should be able to get done for the next version of the patchset.
However, I have some remaining comments on the following feedback:
On 2019-10-10 6:34 a.m., Christoph Hellwig wrote:
>> + /* don't support host memory buffer */
>> + id->hmpre = 0;
>> + id->hmmin = 0;
>
> What about CMB/PMR?
The CMB and PMR are not specified in the identify structure. They are
specified in PCI registers so there's no need to override anything here.
I don't think it makes any sense to try to pass these through fabrics.
>> + /*
>> + * When passsthru controller is setup using nvme-loop transport it will
>> + * export the passthru ctrl subsysnqn (PCIe NVMe ctrl) and will fail in
>> + * the nvme/host/core.c in the nvme_init_subsystem()->nvme_active_ctrl()
>> + * code path with duplicate ctr subsynqn. In order to prevent that we
>> + * mask the passthru-ctrl subsysnqn with the target ctrl subsysnqn.
>> + */
>> + memcpy(id->subnqn, ctrl->subsysnqn, sizeof(id->subnqn));
>
> I don't think this is a good idea. It will break multipathing when you
> export two ports of the original controller. The whole idea of
> overwriting ctrlid and subsysnqn will also lead to huge problems with
> persistent reservations. I think we need to pass through the subnqn
> and cntlid unmodified.
I think trying to clone the cntlid will cause bigger problems with
multipath... I'll inflict some ascii art on you to try and explain.
The fabrics code creates a new controller for every connection, so if
they all had the cntlid of the passed through controller then we'd have
to restrict each passed through controller to only have a single
connection which means we can't have multi-path over the fabrics side
because we'd end up with something like this:
+-----------------+ +-----------------+ +----------------+
|Host A Subsys A | |Target Subsys A | |Host B Subsys A |
| +--------------+| | +------+ | tcp | +------+|
| | PCI Device || -------------+ Ctrl +-----------------+ Ctrl ||
| | +------+|| | | | 0 | | | | 0 ||
| | | Ctrl +----+ | +------+ | | +------+|
| | | 0 ||| | | +------+ | rdma | +------+|
| | +------+|| +------------+ Ctrl +-----------------+ Ctrl ||
| | +------+|| | | | 0 | | | | 0 ||
| | | Ctrl ||| | | +------+ | | +------+|
| | | 1 ||| | | +------+ | loop | |
| | +------+|| +------------+ Ctrl +----+ | |
| +--------------+| | | 0 | | | | |
| | | +------+ | | | |
| | +-----------------+ | +----------------+
| | |
| ----------------------------------------+
| | |
| | +------+ |
| +---+ Ctrl | |
| | 0 | |
| +------+ |
+-----------------+
Multipath doesn't work on Host B because all the controllers have the
same cntlid and it doesn't work on Host A for the loop back interface
because the cntlid conflicts with the cntlid of the original device. If
we allow the target to assign cntlid's from the IDA, per usual, I think
we have a much better situation:
+-----------------+ +-----------------+ +----------------+
|Host A Subsys A | |Target Subsys A | |Host B Subsys A |
| +--------------+| | +------+ | tcp | +------+|
| | PCI Device || -------------+ Ctrl +-----------------+ Ctrl ||
| | +------+|| | | | 0 | | | | 0 ||
| | | Ctrl +----+ | +------+ | | +------+|
| | | 0 ||| | | +------+ | rdma | +------+|
| | +------+|| +------------+ Ctrl +-----------------+ Ctrl ||
| | +------+|| | | | 1 | | | | 1 ||
| | | Ctrl ||| | | +------+ | | +------+|
| | | 1 ||| | | +------+ | loop | |
| | +------+|| +------------+ Ctrl +----+ | |
| +--------------+| | | 2 | | | | |
| | | +------+ | | | |
| | +-----------------+ | +----------------+
| | |
| ----------------------------------------+
| | |
| | +------+ |
| +---+ Ctrl | |
| | 2 | |
| +------+ |
+-----------------+
Now multipath works for host B and will work with the loopback to Host
A, but *only* if the target assigns it a cntlid that doesn't conflict
with one that was in the original device (which is actually quite common
in the simple case of one device and one target). To deal with this
situation I contend it's better to replace the subsysnqn:
+-----------------+ +-----------------+ +----------------+
|Host A Subsys A | |Target Subsys B | | Host B Subsys B|
| +--------------+| | +------+ | tcp | +------+|
| | PCI Device || -------------+ Ctrl +-----------------+ Ctrl ||
| | +------+|| | | | 2 | | | | 2 ||
| | | Ctrl +----+ | +------+ | | +------+|
| | | 0 ||| | | +------+ | rdma | +------+|
| | +------+|| +------------+ Ctrl +-----------------+ Ctrl ||
| | +------+|| | | | 1 | | | | 1 ||
| | | Ctrl ||| | | +------+ | | +------+|
| | | 1 ||| | | +------+ | loop | |
| | +------+|| +------------- Ctrl +----+ | |
| +--------------+| | | 0 | | | | |
+-----------------+ | +------+ | | | |
+-----------------+ +-----------------+ | +----------------+
|Host A Subsys B | |
| ----------------------------------------+
| | |
| | +------+ |
| +---+ Ctrl | |
| | 0 | |
| +------+ |
+-----------------+
This way loopback is always guaranteed to work, and multipath over
fabrics will still work as well because they are never exposed to the
original subsysnqn. Using a loopback device is really only useful for
testing so I don't think using it as a path in a multipath setup will
ever make any sense and thus we don't lose anything valuable by not
having the same subsysnqn for the looped back host.
The first problem with this is if someone wants to passthru two ports
of a multiport PCI device. If the cntlids and the subsysnqns were copied
we could theoretically do something like this:
+-----------------+ +-----------------+ +-----------------+
|Host A Subsys A | |Target Subsys A | |Host B Subsys A |
| +--------------+| | +------+ | | +------+ |
| | PCI Device || ------------+ Ctrl +------------------+ Ctrl | |
| | +------+|| | | | 0 | | | | 0 | |
| | | Ctrl +----+ | +------+ | | +------+ |
| | | 0 ||| +-----------------+ | +------+ |
| | +------+|| +-----------------+ ------------+ Ctrl | |
| | +------+|| | Target Subsys A | | | | 1 | |
| | | Ctrl +----+ | +------+ | | | +------+ |
| | | 1 ||| | | | Ctrl | | | | |
| | +------+|| -----------+| 1 +------+ +-----------------+
| +--------------+| | +------+ |
+-----------------+ +-----------------+
But this is awkward because we now have two subsystems that will require
different nqns in configfs but will expose the same nqn as the original
device in the identify command. If we try to make it less awkward by
allowing a target subsystem to point to multiple ctrls (of the same
subsystem) then we end up having to deal with a bunch of multipath
complexity like implementing multipath for admin commands, etc. Not to
mention the current passthru code is pretty much bypassing all the core
multipath code so this would all have to be reworked significantly.
I would argue that if someone wants to create a target for a multi-port
PCI device and have multipath through both ports, then they should use
the regular block device target and not a passthru target -- then it
will all work using the existing multipath support in the core.
The second problem with substituting cntlids is that some admin commands
like the namespace attach command, take the cntlid as an input, so we'd
have to translate those some how if we want to pass them through. I
think this should be possible, however, I don't have any hardware that
implements this so it would be hard for me to test any solution for this
problem. So, for now, we've chosen just to reject such admin commands.
>> + /* Support multipath connections with fabrics */
>> + id->cmic |= 1 << 1;
>
> I don't think we can just overwrite this, we need to use the original
> controllers values.
If we don't overwrite this, then none of the multi-path over fabric
scenarios, from above (that we do want to support) will work with any
device that doesn't advertise this bit. As long as we set the bit, then
multipath over the fabrics connection will work just fine.
>> + /* 4. By default, blacklist all admin commands */
>> + default:
>> +
>> + status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
>> + req->execute = NULL;
>> + break;
>> + }
>
> That seems odd. There is plenty of other useful admin commands.
>
> Yes, we need to ignore the PCIe specific ones:
>
> - Create I/O Completion Queue
> - Create I/O Submission Queue
> - Delete I/O Completion Queue
> - Delete I/O Submission Queue
> - Doorbell Buffer Configuration
> - Virtualization Management
>
> but all others seem perfectly valid to pass through.
This was based on Sagi's feedback[1]. He contends that the format NVM
command is not safe in an environment where there might be multiple
hosts. For similar reasons, firmware commands and others might be
dangerous too. We also have to ignore NS attach commands for reasons
outlined above. So it certainly seems like there's more admin commands
than not that we need to at least be careful of. Starting with a black
list then adding the commands that are interesting to pass through (and
that we can properly reason won't break things) seems like a prudent
approach. For our use cases, we largely only care about identify
commands and vendor specific commands.
Logan
[1]
https://lore.kernel.org/linux-block/e4430207-7def-8776-0289-0d58689dc0cd@grimberg.me/
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2019-10-25 23:10 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 19:25 [PATCH v9 00/12] nvmet: add target passthru commands support Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 01/12] nvme-core: introduce nvme_ctrl_get_by_path() Logan Gunthorpe
2019-10-09 22:13 ` Keith Busch
2019-10-10 11:37 ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 02/12] nvme-core: export existing ctrl and ns interfaces Logan Gunthorpe
2019-10-09 22:14 ` Keith Busch
2019-10-09 19:25 ` [PATCH v9 03/12] nvmet: add return value to nvmet_add_async_event() Logan Gunthorpe
2019-10-09 22:17 ` Keith Busch
2019-10-09 19:25 ` [PATCH v9 04/12] nvmet: make nvmet_copy_ns_identifier() non-static Logan Gunthorpe
2019-10-09 22:18 ` Keith Busch
2019-10-10 11:50 ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 05/12] Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> [logang@deltatee.com: fixed some of the wording in the help message] Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Max Gurtovoy <maxg@mellanox.com> Logan Gunthorpe
2019-10-10 12:34 ` Christoph Hellwig
2019-10-25 23:09 ` Logan Gunthorpe [this message]
2019-10-09 19:25 ` [PATCH v9 05/12] nvmet-passthru: add passthru code to process commands Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 06/12] nvmet-passthru: add enable/disable helpers Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 07/12] nvmet-core: don't check the data len for pt-ctrl Logan Gunthorpe
2019-10-10 11:04 ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 08/12] nvmet-tcp: don't check data_len in nvmet_tcp_map_data() Logan Gunthorpe
2019-10-10 11:07 ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 09/12] nvmet-configfs: introduce passthru configfs interface Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat() Logan Gunthorpe
2019-10-10 10:05 ` Christoph Hellwig
2019-10-10 17:56 ` Logan Gunthorpe
2019-10-09 19:25 ` [PATCH v9 11/12] block: call blk_account_io_start() in blk_execute_rq_nowait() Logan Gunthorpe
2019-10-10 10:06 ` Christoph Hellwig
2019-10-09 19:25 ` [PATCH v9 12/12] nvmet-passthru: support block accounting Logan Gunthorpe
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=247eca47-c3bc-6452-fb19-f7aa27b05a60@deltatee.com \
--to=logang@deltatee.com \
--cc=Chaitanya.Kulkarni@wdc.com \
--cc=axboe@fb.com \
--cc=hch@lst.de \
--cc=kbusch@kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=maxg@mellanox.com \
--cc=sagi@grimberg.me \
--cc=sbates@raithlin.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).