* Re: [PATCH 1/2] nvme: support fused nvme requests
2021-01-05 22:49 ` [PATCH 1/2] nvme: support fused nvme requests klayph
@ 2021-01-05 23:52 ` Keith Busch
2021-01-06 14:55 ` Clay Mayers
2021-01-06 0:35 ` James Smart
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Keith Busch @ 2021-01-05 23:52 UTC (permalink / raw)
To: klayph; +Cc: Jens Axboe, Christoph Hellwig, linux-nvme, Sagi Grimberg
On Tue, Jan 05, 2021 at 02:49:38PM -0800, klayph@gmail.com wrote:
> @@ -157,6 +157,8 @@ struct nvme_request {
> u8 flags;
> u16 status;
> struct nvme_ctrl *ctrl;
> + struct nvme_request *nrq2; /* Points to second fused request */
> + struct nvme_command cmnd; /* Saved fused first cmnd */
This struct is getting a bit large, and we allocate 1000's of them. An
extra 72B (3x its current size) for something that likely never gets
used (and can't be used for fabrics) seems a bit much.
> @@ -918,7 +942,13 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
> }
>
> blk_mq_start_request(req);
> - nvme_submit_cmd(nvmeq, &cmnd, bd->last);
> +
> + if (cmnd.common.flags & NVME_CMD_FUSE_FIRST)
> + memcpy(&nvme_req(req)->nrq2->cmnd, &cmnd, sizeof(cmnd));
> + else if (cmnd.common.flags & NVME_CMD_FUSE_SECOND)
> + nvme_submit_cmd2(nvmeq, &nvme_req(req)->cmnd, &cmnd, bd->last);
> + else
> + nvme_submit_cmd(nvmeq, &cmnd, bd->last);
There's an unlikely scenario where this breaks down, but it'd look
something like this: the FUSE_FIRST is submitted to the driver, then
before the FUSE_SECOND reaches the driver, the controller is reset.
Because you're only using this through the passthrough interface where
the FAILFAST flag is set, the FUSE_FIRST command is ended in failure,
which releases the command ID for reuse. Once the controller is
restarted, the FUSE_SECOND is dispatched to the driver using an invalid
command ID stored in its 'cmnd' field.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/2] nvme: support fused nvme requests
2021-01-05 23:52 ` Keith Busch
@ 2021-01-06 14:55 ` Clay Mayers
0 siblings, 0 replies; 22+ messages in thread
From: Clay Mayers @ 2021-01-06 14:55 UTC (permalink / raw)
To: Keith Busch; +Cc: Jens Axboe, Christoph Hellwig, linux-nvme, Sagi Grimberg
Thanks for the very fast feedback. I will certainly reduce the memory
hit for non-fused cmds and look into dealing with a controller reset.
On Tue, Jan 5, 2021 at 3:52 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Tue, Jan 05, 2021 at 02:49:38PM -0800, klayph@gmail.com wrote:
> > @@ -157,6 +157,8 @@ struct nvme_request {
> > u8 flags;
> > u16 status;
> > struct nvme_ctrl *ctrl;
> > + struct nvme_request *nrq2; /* Points to second fused request */
> > + struct nvme_command cmnd; /* Saved fused first cmnd */
>
> This struct is getting a bit large, and we allocate 1000's of them. An
> extra 72B (3x its current size) for something that likely never gets
> used (and can't be used for fabrics) seems a bit much.
>
> > @@ -918,7 +942,13 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
> > }
> >
> > blk_mq_start_request(req);
> > - nvme_submit_cmd(nvmeq, &cmnd, bd->last);
> > +
> > + if (cmnd.common.flags & NVME_CMD_FUSE_FIRST)
> > + memcpy(&nvme_req(req)->nrq2->cmnd, &cmnd, sizeof(cmnd));
> > + else if (cmnd.common.flags & NVME_CMD_FUSE_SECOND)
> > + nvme_submit_cmd2(nvmeq, &nvme_req(req)->cmnd, &cmnd, bd->last);
> > + else
> > + nvme_submit_cmd(nvmeq, &cmnd, bd->last);
>
> There's an unlikely scenario where this breaks down, but it'd look
> something like this: the FUSE_FIRST is submitted to the driver, then
> before the FUSE_SECOND reaches the driver, the controller is reset.
> Because you're only using this through the passthrough interface where
> the FAILFAST flag is set, the FUSE_FIRST command is ended in failure,
> which releases the command ID for reuse. Once the controller is
> restarted, the FUSE_SECOND is dispatched to the driver using an invalid
> command ID stored in its 'cmnd' field.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvme: support fused nvme requests
2021-01-05 22:49 ` [PATCH 1/2] nvme: support fused nvme requests klayph
2021-01-05 23:52 ` Keith Busch
@ 2021-01-06 0:35 ` James Smart
2021-01-06 15:01 ` Clay Mayers
2021-01-06 7:59 ` Christoph Hellwig
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: James Smart @ 2021-01-06 0:35 UTC (permalink / raw)
To: klayph, linux-nvme
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
[-- Attachment #1.1: Type: text/plain, Size: 3248 bytes --]
On 1/5/2021 2:49 PM, klayph@gmail.com wrote:
> From
>
> /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 3be352403839..c24729e100bc 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -491,6 +491,30 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
> nvmeq->last_sq_tail = nvmeq->sq_tail;
> }
>
> +/**
> + * nvme_submit_cmd2() - Copy fused commands into a queue and ring the doorbell
> + * @nvmeq: The queue to use
> + * @cmd: The first command to send
> + * @cmd2: the second command to send
> + * @write_sq: whether to write to the SQ doorbell
> + */
> +static void nvme_submit_cmd2(struct nvme_queue *nvmeq, struct nvme_command *cmd,
> + struct nvme_command *cmd2, bool write_sq)
> +{
> + spin_lock(&nvmeq->sq_lock);
> + memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> + cmd, sizeof(*cmd));
> + if (++nvmeq->sq_tail == nvmeq->q_depth)
> + nvmeq->sq_tail = 0;
> + memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> + cmd2, sizeof(*cmd2));
> + if (++nvmeq->sq_tail == nvmeq->q_depth)
> + nvmeq->sq_tail = 0;
> + nvme_write_sq_db(nvmeq, write_sq);
> + spin_unlock(&nvmeq->sq_lock);
> +}
> +
> +
> /**
> * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
> * @nvmeq: The queue to use
> @@ -918,7 +942,13 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
> }
>
> blk_mq_start_request(req);
> - nvme_submit_cmd(nvmeq, &cmnd, bd->last);
> +
> + if (cmnd.common.flags & NVME_CMD_FUSE_FIRST)
> + memcpy(&nvme_req(req)->nrq2->cmnd, &cmnd, sizeof(cmnd));
> + else if (cmnd.common.flags & NVME_CMD_FUSE_SECOND)
> + nvme_submit_cmd2(nvmeq, &nvme_req(req)->cmnd, &cmnd, bd->last);
> + else
> + nvme_submit_cmd(nvmeq, &cmnd, bd->last);
> return BLK_STS_OK;
> out_unmap_data:
> nvme_unmap_data(dev, req);
I think Keith caught the most concerning issue. I've been thinking of
others but they mostly seem to work. I assume recopies of the first
fused cmd to the data structure don't hurt. I would expect there to be
a couple of other odd issues if the two requests start working
independently.
This is the pci transport only. You should either put something in to
not allow the ioctl on a non-pci transport, or add the support (or
rejection) to the fabric transports as well.
-- james
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4163 bytes --]
[-- Attachment #2: Type: text/plain, Size: 158 bytes --]
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/2] nvme: support fused nvme requests
2021-01-06 0:35 ` James Smart
@ 2021-01-06 15:01 ` Clay Mayers
0 siblings, 0 replies; 22+ messages in thread
From: Clay Mayers @ 2021-01-06 15:01 UTC (permalink / raw)
To: James Smart
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, linux-nvme, Sagi Grimberg
Thanks for the very fast feedback. I will ensure this is pci only for now.
On Tue, Jan 5, 2021 at 4:35 PM James Smart <james.smart@broadcom.com> wrote:
>
>
>
> On 1/5/2021 2:49 PM, klayph@gmail.com wrote:
> > From
>
> >
> > /*
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 3be352403839..c24729e100bc 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -491,6 +491,30 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
> > nvmeq->last_sq_tail = nvmeq->sq_tail;
> > }
> >
> > +/**
> > + * nvme_submit_cmd2() - Copy fused commands into a queue and ring the doorbell
> > + * @nvmeq: The queue to use
> > + * @cmd: The first command to send
> > + * @cmd2: the second command to send
> > + * @write_sq: whether to write to the SQ doorbell
> > + */
> > +static void nvme_submit_cmd2(struct nvme_queue *nvmeq, struct nvme_command *cmd,
> > + struct nvme_command *cmd2, bool write_sq)
> > +{
> > + spin_lock(&nvmeq->sq_lock);
> > + memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> > + cmd, sizeof(*cmd));
> > + if (++nvmeq->sq_tail == nvmeq->q_depth)
> > + nvmeq->sq_tail = 0;
> > + memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> > + cmd2, sizeof(*cmd2));
> > + if (++nvmeq->sq_tail == nvmeq->q_depth)
> > + nvmeq->sq_tail = 0;
> > + nvme_write_sq_db(nvmeq, write_sq);
> > + spin_unlock(&nvmeq->sq_lock);
> > +}
> > +
> > +
> > /**
> > * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
> > * @nvmeq: The queue to use
> > @@ -918,7 +942,13 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
> > }
> >
> > blk_mq_start_request(req);
> > - nvme_submit_cmd(nvmeq, &cmnd, bd->last);
> > +
> > + if (cmnd.common.flags & NVME_CMD_FUSE_FIRST)
> > + memcpy(&nvme_req(req)->nrq2->cmnd, &cmnd, sizeof(cmnd));
> > + else if (cmnd.common.flags & NVME_CMD_FUSE_SECOND)
> > + nvme_submit_cmd2(nvmeq, &nvme_req(req)->cmnd, &cmnd, bd->last);
> > + else
> > + nvme_submit_cmd(nvmeq, &cmnd, bd->last);
> > return BLK_STS_OK;
> > out_unmap_data:
> > nvme_unmap_data(dev, req);
>
> I think Keith caught the most concerning issue. I've been thinking of
> others but they mostly seem to work. I assume recopies of the first
> fused cmd to the data structure don't hurt. I would expect there to be
> a couple of other odd issues if the two requests start working
> independently.
>
> This is the pci transport only. You should either put something in to
> not allow the ioctl on a non-pci transport, or add the support (or
> rejection) to the fabric transports as well.
>
> -- james
>
>
>
>
> --
> This electronic communication and the information and any files transmitted
> with it, or attached to it, are confidential and are intended solely for
> the use of the individual or entity to whom it is addressed and may contain
> information that is confidential, legally privileged, protected by privacy
> laws, or otherwise restricted from disclosure to anyone else. If you are
> not the intended recipient or the person responsible for delivering the
> e-mail to the intended recipient, you are hereby notified that any use,
> copying, distributing, dissemination, forwarding, printing, or copying of
> this e-mail is strictly prohibited. If you received this e-mail in error,
> please return the e-mail to the sender, delete it from your computer, and
> destroy any printed copy of it.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] nvme: support fused nvme requests
2021-01-05 22:49 ` [PATCH 1/2] nvme: support fused nvme requests klayph
2021-01-05 23:52 ` Keith Busch
2021-01-06 0:35 ` James Smart
@ 2021-01-06 7:59 ` Christoph Hellwig
2021-01-25 19:58 ` [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO clay.mayers
` (2 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2021-01-06 7:59 UTC (permalink / raw)
To: klayph
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, linux-nvme, Sagi Grimberg
I really don't like this. There is no good use case and it really
affects the I/O submission fast path.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-01-05 22:49 ` [PATCH 1/2] nvme: support fused nvme requests klayph
` (2 preceding siblings ...)
2021-01-06 7:59 ` Christoph Hellwig
@ 2021-01-25 19:58 ` clay.mayers
2021-01-26 1:43 ` Chaitanya Kulkarni
2021-01-25 19:58 ` [PATCH V2 1/2] nvme: support fused pci nvme requests clay.mayers
2021-01-25 19:58 ` [PATCH V2 2/2] nvme: support fused NVME_IOCTL_SUBMIT_IO clay.mayers
5 siblings, 1 reply; 22+ messages in thread
From: clay.mayers @ 2021-01-25 19:58 UTC (permalink / raw)
To: linux-nvme; +Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
From: Clay Mayers <clay.mayers@kioxia.com>
Enables fused compare/write to be issued from user mode.
The first patch allows a pair of pass through requests
to be linked and submitted as a fused pair to the nvme
pci device queue. The second patch exposes it through
an nvme ioctl so a fused pair can be issued from user
mode.
There was a question regarding Linux support for this
functionality in nvme-cli. With these patches, that
would be possible.
https://github.com/linux-nvme/nvme-cli/issues/318
Local pci device fused support is also necessary for
NVMeOF targets to support fused operation.
Clay Mayers (2):
nvme: support fused pci nvme requests
nvme: support fused NVME_IOCTL_SUBMIT_IO
drivers/nvme/host/core.c | 267 ++++++++++++++++++++++++++++++---------
drivers/nvme/host/nvme.h | 12 ++
drivers/nvme/host/pci.c | 85 ++++++++++++-
3 files changed, 302 insertions(+), 62 deletions(-)
--
2.27.0
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-01-25 19:58 ` [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO clay.mayers
@ 2021-01-26 1:43 ` Chaitanya Kulkarni
2021-01-26 18:17 ` Clay Mayers
0 siblings, 1 reply; 22+ messages in thread
From: Chaitanya Kulkarni @ 2021-01-26 1:43 UTC (permalink / raw)
To: clay.mayers, linux-nvme
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
On 1/25/21 12:03, clay.mayers@kioxia.com wrote:
> Local pci device fused support is also necessary for
> NVMeOF targets to support fused operation.
Please explain the use case and the application of the NVMeOF
fuse command feature.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-01-26 1:43 ` Chaitanya Kulkarni
@ 2021-01-26 18:17 ` Clay Mayers
2021-01-26 19:00 ` Chaitanya Kulkarni
0 siblings, 1 reply; 22+ messages in thread
From: Clay Mayers @ 2021-01-26 18:17 UTC (permalink / raw)
To: Chaitanya Kulkarni, linux-nvme
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
> From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
> Sent: Monday, January 25, 2021 5:43 PM
> To: Clay Mayers <Clay.Mayers@kioxia.com>; linux-nvme@lists.infradead.org
> Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>;
> Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>
> Subject: Re: [PATCH V2 0/2] nvme: Support for fused
> NVME_IOCTL_SUBMIT_IO
>
> On 1/25/21 12:03, clay.mayers@kioxia.com wrote:
> > Local pci device fused support is also necessary for NVMeOF targets to
> > support fused operation.
> Please explain the use case and the application of the NVMeOF fuse
> command feature.
NVMeOF devices are used to create disaggregated storage systems
where compute and storage are connected over a fabric. Fused
compare/write can be used to arbitrate shared access to NVMeOF devices
w/o a central authority.
A specific example of how fused compare/write is used is the clustered
file system VMFS. It uses the SCSI version of compare/write to manage
meta data on shared SAN systems. File system meta data is updated
using locks stored on the storage media. Those locks are grabbed using
fused compare/write operations as an atomic test & set. VMFS originally
used device reserve, which is a courser gained locking mechanism but it
doesn't scale as well as an atomic test & set.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-01-26 18:17 ` Clay Mayers
@ 2021-01-26 19:00 ` Chaitanya Kulkarni
2021-01-26 21:14 ` Clay Mayers
2021-02-09 0:53 ` Clay Mayers
0 siblings, 2 replies; 22+ messages in thread
From: Chaitanya Kulkarni @ 2021-01-26 19:00 UTC (permalink / raw)
To: Clay Mayers, linux-nvme
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
On 1/26/21 10:17 AM, Clay Mayers wrote:
>> From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
>> Sent: Monday, January 25, 2021 5:43 PM
>> To: Clay Mayers <Clay.Mayers@kioxia.com>; linux-nvme@lists.infradead.org
>> Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>;
>> Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>
>> Subject: Re: [PATCH V2 0/2] nvme: Support for fused
>> NVME_IOCTL_SUBMIT_IO
>>
>> On 1/25/21 12:03, clay.mayers@kioxia.com wrote:
>>> Local pci device fused support is also necessary for NVMeOF targets to
>>> support fused operation.
>> Please explain the use case and the application of the NVMeOF fuse
>> command feature.
> NVMeOF devices are used to create disaggregated storage systems
> where compute and storage are connected over a fabric. Fused
> compare/write can be used to arbitrate shared access to NVMeOF devices
> w/o a central authority.
>
> A specific example of how fused compare/write is used is the clustered
> file system VMFS. It uses the SCSI version of compare/write to manage
> meta data on shared SAN systems. File system meta data is updated
> using locks stored on the storage media. Those locks are grabbed using
> fused compare/write operations as an atomic test & set. VMFS originally
> used device reserve, which is a courser gained locking mechanism but it
> doesn't scale as well as an atomic test & set.
If I understand correctly VMFS is out of tree filesystem is it ?
Can you please explain the setup in detail ? what kind of interface
file-system is using to issue the command ?
Based on your description it looks like target is connected to the
vmware based system and host is vmware based host and not the linux
host which is present in this series.
Also what are other applications or is this the only one application?
>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-01-26 19:00 ` Chaitanya Kulkarni
@ 2021-01-26 21:14 ` Clay Mayers
2021-02-09 0:53 ` Clay Mayers
1 sibling, 0 replies; 22+ messages in thread
From: Clay Mayers @ 2021-01-26 21:14 UTC (permalink / raw)
To: Chaitanya Kulkarni, linux-nvme
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
> From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
> Sent: Tuesday, January 26, 2021 11:01 AM
>
> On 1/26/21 10:17 AM, Clay Mayers wrote:
> >>
> >> On 1/25/21 12:03, clay.mayers@kioxia.com wrote:
> >>> Local pci device fused support is also necessary for NVMeOF targets
> >>> to support fused operation.
> >> Please explain the use case and the application of the NVMeOF fuse
> >> command feature.
> > NVMeOF devices are used to create disaggregated storage systems where
> > compute and storage are connected over a fabric. Fused compare/write
> > can be used to arbitrate shared access to NVMeOF devices w/o a central
> > authority.
> >
> > A specific example of how fused compare/write is used is the clustered
> > file system VMFS. It uses the SCSI version of compare/write to manage
> > meta data on shared SAN systems. File system meta data is updated
> > using locks stored on the storage media. Those locks are grabbed
> > using fused compare/write operations as an atomic test & set. VMFS
> > originally used device reserve, which is a courser gained locking
> > mechanism but it doesn't scale as well as an atomic test & set.
> If I understand correctly VMFS is out of tree filesystem is it ?
I seem to have misunderstood your request for a use-case. As a patch series,
this is not about NVMeOF. This is about pci support for the fused command.
NVMeOF is the use case for pci fused support.
But how strong of a use case is NVMeOF? I offered clustered file systems
and the public example of VMWare's VMFS to illustrate the usefulness.
Here VMWare is the target and Linux is the host serving up storage over
NVMeOF. That requires fused support the target/host and pci. With a
past company I worked for, we use the SPDK to get this functionality for
disaggregated storage. That's right for some solutions but not all.
Our actual goal is to have something like direct device access without
something like the SPDK. We think io uring is the correct solution.
Jens, just before his winter PTO, tweeted about adding ioctl support to
io uring. We hope to extend that to support fused operations as well.
Exposing it through IOCTL makes the pci patch useful now. The one
Example I have is for nvme-cli as requested on github.
https://github.com/linux-nvme/nvme-cli/issues/318
I thought this was better than folding an nvme change in with an io uring
patch series. I'm trying to find the balance between a small isolated unit of
change and something compelling.
> Can you please explain the setup in detail ? what kind of interface file-system
> is using to issue the command ?
> Based on your description it looks like target is connected to the vmware
> based system and host is vmware based host and not the linux host which is
> present in this series.
No - the idea is to be standards based and use NVMeOF for target and host
data exchange. In one example, the target would be running vSphere. The
host, as a Linux machine, would expose its attached devices with NVMeOF.
VSphere would expect fused command support from the Linux machine.
> Also what are other applications or is this the only one application?
The application is disaggregated storage on NVMeOF, both consuming it
and publishing it. I don't have any specific set of applications to offer.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-01-26 19:00 ` Chaitanya Kulkarni
2021-01-26 21:14 ` Clay Mayers
@ 2021-02-09 0:53 ` Clay Mayers
2021-02-09 3:12 ` Keith Busch
2021-02-09 7:54 ` Christoph Hellwig
1 sibling, 2 replies; 22+ messages in thread
From: Clay Mayers @ 2021-02-09 0:53 UTC (permalink / raw)
To: linux-nvme
Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Chaitanya Kulkarni,
Sagi Grimberg
> From: Clay Mayers
> Sent: Tuesday, January 26, 2021 1:14 PM
> To: linux-nvme@lists.infradead.org
> Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>;
> 'Chaitanya Kulkarni' <Chaitanya.Kulkarni@wdc.com>;
> Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>
> Subject: RE: [PATCH V2 0/2] nvme: Support for fused
> NVME_IOCTL_SUBMIT_IO
>
Is there any other feedback on V2?
My main concern I have about my implementation is how fused requests
are tunneled through the mq request layer. The 1st request is marked as
started but it won't be in the device until the 2nd command is queued. As
Keith pointed out, a device reset can split the two so care must be taken to
correctly handle this case. Despite this, I thought this was a better approach
than modifying mq requests to be fused. Especially given Christoph's
concern of cost vs value. This is the lightest touch I could come up with.
Further consideration of this patch may need a more compelling use case.
I've worked on a proprietary storage systems that relied on fused NVMeOF
support so it seems compelling to me. There's a comment in target/core.c
that there is "no support for fused commands yet" implying it's been
considered. Is pci only support for fused too soon or too little? What would
make it more compelling?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-02-09 0:53 ` Clay Mayers
@ 2021-02-09 3:12 ` Keith Busch
2021-02-09 15:24 ` Bart Van Assche
2021-02-09 15:38 ` Clay Mayers
2021-02-09 7:54 ` Christoph Hellwig
1 sibling, 2 replies; 22+ messages in thread
From: Keith Busch @ 2021-02-09 3:12 UTC (permalink / raw)
To: Clay Mayers
Cc: Jens Axboe, Sagi Grimberg, Chaitanya Kulkarni, linux-nvme,
Christoph Hellwig
On Tue, Feb 09, 2021 at 12:53:17AM +0000, Clay Mayers wrote:
> Is there any other feedback on V2?
>
> My main concern I have about my implementation is how fused requests
> are tunneled through the mq request layer. The 1st request is marked as
> started but it won't be in the device until the 2nd command is queued. As
> Keith pointed out, a device reset can split the two so care must be taken to
> correctly handle this case. Despite this, I thought this was a better approach
> than modifying mq requests to be fused. Especially given Christoph's
> concern of cost vs value. This is the lightest touch I could come up with.
>
> Further consideration of this patch may need a more compelling use case.
> I've worked on a proprietary storage systems that relied on fused NVMeOF
> support so it seems compelling to me. There's a comment in target/core.c
> that there is "no support for fused commands yet" implying it's been
> considered. Is pci only support for fused too soon or too little? What would
> make it more compelling?
The complications it introduces to the IO path and error handling for an
archaic feature has me on the "Nak" side. NVMeOF was introduced well after the
spec define Reservations, and the kernel has supported that capability for many
years. I'm not aware of any other use case for fused commands, so it appears to
be dead weight in the spec.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-02-09 3:12 ` Keith Busch
@ 2021-02-09 15:24 ` Bart Van Assche
2021-02-09 15:38 ` Clay Mayers
1 sibling, 0 replies; 22+ messages in thread
From: Bart Van Assche @ 2021-02-09 15:24 UTC (permalink / raw)
To: Keith Busch, Clay Mayers
Cc: Jens Axboe, linux-nvme, Sagi Grimberg, Chaitanya Kulkarni,
Christoph Hellwig
On 2/8/21 7:12 PM, Keith Busch wrote:
> On Tue, Feb 09, 2021 at 12:53:17AM +0000, Clay Mayers wrote:
>> Is there any other feedback on V2?
>>
>> My main concern I have about my implementation is how fused requests
>> are tunneled through the mq request layer. The 1st request is marked as
>> started but it won't be in the device until the 2nd command is queued. As
>> Keith pointed out, a device reset can split the two so care must be taken to
>> correctly handle this case. Despite this, I thought this was a better approach
>> than modifying mq requests to be fused. Especially given Christoph's
>> concern of cost vs value. This is the lightest touch I could come up with.
>>
>> Further consideration of this patch may need a more compelling use case.
>> I've worked on a proprietary storage systems that relied on fused NVMeOF
>> support so it seems compelling to me. There's a comment in target/core.c
>> that there is "no support for fused commands yet" implying it's been
>> considered. Is pci only support for fused too soon or too little? What would
>> make it more compelling?
>
> The complications it introduces to the IO path and error handling for an
> archaic feature has me on the "Nak" side. NVMeOF was introduced well after the
> spec define Reservations, and the kernel has supported that capability for many
> years. I'm not aware of any other use case for fused commands, so it appears to
> be dead weight in the spec.
Hi Keith,
Do you agree that NVMe persistent reservation commands apply to an NVMe
namespace in its entirety while fused compare-and-write commands allow
to implement locking for subsets of the LBA range of a namespace? In
other words, I think there is a valid use case for fused commands.
Thanks,
Bart.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-02-09 3:12 ` Keith Busch
2021-02-09 15:24 ` Bart Van Assche
@ 2021-02-09 15:38 ` Clay Mayers
1 sibling, 0 replies; 22+ messages in thread
From: Clay Mayers @ 2021-02-09 15:38 UTC (permalink / raw)
To: Keith Busch
Cc: Jens Axboe, Sagi Grimberg, Chaitanya Kulkarni, linux-nvme,
Christoph Hellwig
> From: Keith Busch <kbusch@kernel.org>
> Sent: Monday, February 8, 2021 7:13 PM
> To: Clay Mayers <Clay.Mayers@kioxia.com>
> Cc: linux-nvme@lists.infradead.org; Jens Axboe <axboe@fb.com>; Chaitanya
> Kulkarni <Chaitanya.Kulkarni@wdc.com>; Christoph Hellwig <hch@lst.de>;
> Sagi Grimberg <sagi@grimberg.me>
> Subject: Re: [PATCH V2 0/2] nvme: Support for fused
> NVME_IOCTL_SUBMIT_IO
>
> On Tue, Feb 09, 2021 at 12:53:17AM +0000, Clay Mayers wrote:
> > Is there any other feedback on V2?
> >
> > My main concern I have about my implementation is how fused requests
> > are tunneled through the mq request layer. The 1st request is marked
> > as started but it won't be in the device until the 2nd command is
> > queued. As Keith pointed out, a device reset can split the two so
> > care must be taken to correctly handle this case. Despite this, I
> > thought this was a better approach than modifying mq requests to be
> > fused. Especially given Christoph's concern of cost vs value. This is the
> lightest touch I could come up with.
> >
> > Further consideration of this patch may need a more compelling use case.
> > I've worked on a proprietary storage systems that relied on fused
> > NVMeOF support so it seems compelling to me. There's a comment in
> > target/core.c that there is "no support for fused commands yet"
> > implying it's been considered. Is pci only support for fused too soon
> > or too little? What would make it more compelling?
>
> The complications it introduces to the IO path and error handling for an
> archaic feature has me on the "Nak" side. NVMeOF was introduced well after
> the spec define Reservations, and the kernel has supported that capability
> for many years. I'm not aware of any other use case for fused commands, so
> it appears to be dead weight in the spec.
Thanks for sharing your perspective. I can see the point that reservations are
good enough for many. For VmWare, moving from reservations to cmp/write
on SCSI led to an increase in performance (couldn't find an actual # but heard
30% anecdotally). That's a specialized use case (storage for VMs) on a much
slower medium that may not translate to more general storage systems or
much faster storage.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-02-09 0:53 ` Clay Mayers
2021-02-09 3:12 ` Keith Busch
@ 2021-02-09 7:54 ` Christoph Hellwig
2021-02-09 15:53 ` Clay Mayers
1 sibling, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2021-02-09 7:54 UTC (permalink / raw)
To: Clay Mayers
Cc: Sagi Grimberg, Chaitanya Kulkarni, linux-nvme, Jens Axboe,
Keith Busch, Christoph Hellwig
On Tue, Feb 09, 2021 at 12:53:17AM +0000, Clay Mayers wrote:
> > From: Clay Mayers
> > Sent: Tuesday, January 26, 2021 1:14 PM
> > To: linux-nvme@lists.infradead.org
> > Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>;
> > 'Chaitanya Kulkarni' <Chaitanya.Kulkarni@wdc.com>;
> > Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>
> > Subject: RE: [PATCH V2 0/2] nvme: Support for fused
> > NVME_IOCTL_SUBMIT_IO
> >
> Is there any other feedback on V2?
My feedback is that we should not add code to the kernel driver and
our fast path for fused command unless we have a mainstream in-kernel
users, that is not at all for the foreseeable future.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO
2021-02-09 7:54 ` Christoph Hellwig
@ 2021-02-09 15:53 ` Clay Mayers
0 siblings, 0 replies; 22+ messages in thread
From: Clay Mayers @ 2021-02-09 15:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Jens Axboe, Chaitanya Kulkarni, linux-nvme, Sagi Grimberg
> From: Christoph Hellwig <hch@lst.de>
> Sent: Monday, February 8, 2021 11:55 PM
> To: Clay Mayers <Clay.Mayers@kioxia.com>
> Cc: linux-nvme@lists.infradead.org; Keith Busch <kbusch@kernel.org>; Jens
> Axboe <axboe@fb.com>; Chaitanya Kulkarni
> <Chaitanya.Kulkarni@wdc.com>; Christoph Hellwig <hch@lst.de>; Sagi
> Grimberg <sagi@grimberg.me>
> Subject: Re: [PATCH V2 0/2] nvme: Support for fused
> NVME_IOCTL_SUBMIT_IO
>
> On Tue, Feb 09, 2021 at 12:53:17AM +0000, Clay Mayers wrote:
> > > From: Clay Mayers
> > > Sent: Tuesday, January 26, 2021 1:14 PM
> > > To: linux-nvme@lists.infradead.org
> > > Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>;
> > > 'Chaitanya Kulkarni' <Chaitanya.Kulkarni@wdc.com>; Christoph Hellwig
> > > <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>
> > > Subject: RE: [PATCH V2 0/2] nvme: Support for fused
> > > NVME_IOCTL_SUBMIT_IO
> > >
> > Is there any other feedback on V2?
>
> My feedback is that we should not add code to the kernel driver and our fast
> path for fused command unless we have a mainstream in-kernel users, that
> is not at all for the foreseeable future.
Thanks - that makes sense. I know the patch isn't enough to be very useful on
its own. I shared it at this stage because of my concern over the implementation.
Thanks to the feedback from the list, it is better than it was.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH V2 1/2] nvme: support fused pci nvme requests
2021-01-05 22:49 ` [PATCH 1/2] nvme: support fused nvme requests klayph
` (3 preceding siblings ...)
2021-01-25 19:58 ` [PATCH V2 0/2] nvme: Support for fused NVME_IOCTL_SUBMIT_IO clay.mayers
@ 2021-01-25 19:58 ` clay.mayers
2021-01-25 19:58 ` [PATCH V2 2/2] nvme: support fused NVME_IOCTL_SUBMIT_IO clay.mayers
5 siblings, 0 replies; 22+ messages in thread
From: clay.mayers @ 2021-01-25 19:58 UTC (permalink / raw)
To: linux-nvme; +Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
From: Clay Mayers <clay.mayers@kioxia.com>
Adds support for fused nvme commands to be tunneled through a blk_mq
queue and submitted atomically to a pci nvme device queue.
In nvme_queue_rq(), when an nvme cmnd has the first fused flag set,
the nvme cmnd is saved in nvme_request.fctx and the command is not
queued to the device. Once the nvme cmnd with the second fused flag
set is queued, nvme_request.nrq is used to get back to the first
fused request so both cmnds can be queued to the device atomically.
v2:
Reduced size of nvme_request by pointing to saved cmnd instead so
only fused commands require the extra 64 bytes. The saved cmnd is
now with the first request to ease clean up. Flipped second req
to point to the first so a union with the saved cmnd pointer can
be used to limit the fused impact to 8 bytes.
Fixed issue with aborted first fused cmnd being submitted by
unaborted second fused cmnd.
Signed-off-by: Clay Mayers <clay.mayers@kioxia.com>
---
drivers/nvme/host/nvme.h | 12 ++++++
drivers/nvme/host/pci.c | 85 ++++++++++++++++++++++++++++++++++++++--
2 files changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 567f7ad18a91..b41ce7cd4f49 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -157,6 +157,18 @@ struct nvme_request {
u8 flags;
u16 status;
struct nvme_ctrl *ctrl;
+ union {
+ struct nvme_request *nrq; /* other fused request */
+ /*
+ * Between the time the first fused is queued with nvme_queue_rq
+ * and the second command is queued, the first fused command
+ * uses fctx instead of nrq.
+ */
+ struct nvme_fused_ctx {
+ struct nvme_request *nrq2; /* copy of nrq */
+ struct nvme_command cmnd; /* copy of 1st fused */
+ } *fctx;
+ };
};
/*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3be352403839..ba4798685811 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -491,6 +491,30 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
nvmeq->last_sq_tail = nvmeq->sq_tail;
}
+/**
+ * nvme_submit_cmd2() - Copy fused commands into a queue and ring the doorbell
+ * @nvmeq: The queue to use
+ * @cmd: The first command to send
+ * @cmd2: the second command to send
+ * @write_sq: whether to write to the SQ doorbell
+ */
+static void nvme_submit_cmd2(struct nvme_queue *nvmeq, struct nvme_command *cmd,
+ struct nvme_command *cmd2, bool write_sq)
+{
+ spin_lock(&nvmeq->sq_lock);
+ memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
+ cmd, sizeof(*cmd));
+ if (++nvmeq->sq_tail == nvmeq->q_depth)
+ nvmeq->sq_tail = 0;
+ memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
+ cmd2, sizeof(*cmd2));
+ if (++nvmeq->sq_tail == nvmeq->q_depth)
+ nvmeq->sq_tail = 0;
+ nvme_write_sq_db(nvmeq, write_sq);
+ spin_unlock(&nvmeq->sq_lock);
+}
+
+
/**
* nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
* @nvmeq: The queue to use
@@ -876,6 +900,37 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
return BLK_STS_OK;
}
+static blk_status_t nvme_queue_frq(struct nvme_queue *nvmeq,
+ struct request *req, struct nvme_command *cmnd,
+ bool write_sq)
+{
+ struct nvme_fused_ctx *fctx;
+
+ if (cmnd->common.flags & NVME_CMD_FUSE_FIRST) {
+ /* Save cmnd to submit with 2nd fused */
+ fctx = kmalloc(sizeof(*fctx), GFP_KERNEL);
+ if (!fctx)
+ return BLK_STS_RESOURCE;
+ fctx->nrq2 = nvme_req(req)->nrq;
+ memcpy(&fctx->cmnd, cmnd, sizeof(*cmnd));
+ nvme_req(req)->fctx = fctx;
+ blk_mq_start_request(req);
+ return BLK_STS_OK;
+ }
+ /* handle NVME_CMD_FUSED_SECOND */
+ if (!nvme_req(req)->nrq) {
+ nvme_req(req)->status = NVME_SC_FUSED_FAIL;
+ return BLK_STS_IOERR; /* First i/o has been canceled */
+ }
+
+ fctx = nvme_req(req)->nrq->fctx;
+ nvme_req(req)->nrq->fctx = NULL;
+ blk_mq_start_request(req);
+ nvme_submit_cmd2(nvmeq, &fctx->cmnd, cmnd, write_sq);
+ kfree(fctx);
+ return BLK_STS_OK;
+}
+
/*
* NOTE: ns is NULL when called on the admin queue.
*/
@@ -889,6 +944,7 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
struct nvme_command cmnd;
blk_status_t ret;
+ int fused;
iod->aborted = 0;
iod->npages = -1;
@@ -917,8 +973,15 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
goto out_unmap_data;
}
- blk_mq_start_request(req);
- nvme_submit_cmd(nvmeq, &cmnd, bd->last);
+ fused = cmnd.common.flags & (NVME_CMD_FUSE_FIRST|NVME_CMD_FUSE_SECOND);
+ if (likely(!fused)) {
+ blk_mq_start_request(req);
+ nvme_submit_cmd(nvmeq, &cmnd, bd->last);
+ } else {
+ ret = nvme_queue_frq(nvmeq, req, &cmnd, bd->last);
+ if (ret)
+ goto out_unmap_data;
+ }
return BLK_STS_OK;
out_unmap_data:
nvme_unmap_data(dev, req);
@@ -2423,6 +2486,22 @@ static void nvme_pci_disable(struct nvme_dev *dev)
}
}
+static bool nvme_pci_cancel_rq(struct request *req, void *data, bool reserved)
+{
+ if (unlikely(nvme_req(req)->cmd->common.flags & NVME_CMD_FUSE_FIRST)) {
+ struct nvme_fused_ctx *fctx = nvme_req(req)->fctx;
+
+ /* this will only be set if 2nd fused isn't queued yet */
+ if (unlikely(fctx)) {
+ fctx->nrq2->nrq = NULL; /* break link of 2nd fused */
+ nvme_req(req)->fctx = NULL;
+ kfree(fctx);
+ }
+ }
+ return nvme_cancel_request(req, data, reserved);
+}
+
+
static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
{
bool dead = true, freeze = false;
@@ -2459,7 +2538,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
nvme_pci_disable(dev);
nvme_reap_pending_cqes(dev);
- blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
+ blk_mq_tagset_busy_iter(&dev->tagset, nvme_pci_cancel_rq, &dev->ctrl);
blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
blk_mq_tagset_wait_completed_request(&dev->tagset);
blk_mq_tagset_wait_completed_request(&dev->admin_tagset);
--
2.27.0
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH V2 2/2] nvme: support fused NVME_IOCTL_SUBMIT_IO
2021-01-05 22:49 ` [PATCH 1/2] nvme: support fused nvme requests klayph
` (4 preceding siblings ...)
2021-01-25 19:58 ` [PATCH V2 1/2] nvme: support fused pci nvme requests clay.mayers
@ 2021-01-25 19:58 ` clay.mayers
5 siblings, 0 replies; 22+ messages in thread
From: clay.mayers @ 2021-01-25 19:58 UTC (permalink / raw)
To: linux-nvme; +Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
From: Clay Mayers <clay.mayers@kioxia.com>
Extends the functionality of the NVME_IOCTL_SUBMIT_IO ioctl to support
a pair of fused nvme_user_io requests.
When submitting a fused pair, an array of two nvme_user_io structs are
supplied when invoking NVME_IOCTL_SUBMIT_IO ioctl. Rather than
introduce a new ioctl code, the presence of a fused pair is indicated
by the nvme_user_io.flags having the value of NVME_CMD_FUSED_FIRST.
This then indicates a second nvme_user_io struct follows the first with
an nvme_user_io.flags set to NVME_CMD_FUSED_SECOND.
A fused pair may fail to submit with -EWOULDBLOCK. This indicates the
device queue selected for the first command didn't have a tag available
when the request for the second command was created. The caller should
retry the request.
v2:
Restricted fused support to just the pci transport
Restored the #ifdef CONFIG_COMPAT around nvme_user_io32 defines.
Signed-off-by: Clay Mayers <clay.mayers@kioxia.com>
---
drivers/nvme/host/core.c | 267 ++++++++++++++++++++++++++++++---------
1 file changed, 208 insertions(+), 59 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9a270e49df17..29e9129f6bf3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1467,16 +1467,40 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
return (void __user *)ptrval;
}
-static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
+struct nvme_user_io_req {
+ struct nvme_command cmd;
+ struct request *rq;
+ struct bio *bio; /* bio in rq at the time of allocation */
+ void *meta;
+ void __user *udata;
+ void __user *umeta;
+ unsigned int len;
+ unsigned int mlen;
+ u32 mseed;
+};
+
+static void nvme_free_io(struct nvme_user_io_req *nrq)
+{
+ if (!nrq)
+ return;
+ kfree(nrq->meta);
+ if (nrq->bio)
+ blk_rq_unmap_user(nrq->bio);
+ if (nrq->rq)
+ blk_mq_free_request(nrq->rq);
+ nrq->meta = NULL;
+ nrq->bio = NULL;
+ nrq->rq = NULL;
+}
+
+static int nvme_prep_io(struct nvme_ns *ns, struct nvme_user_io_req *nrq,
+ struct nvme_user_io __user *uio, int size)
{
struct nvme_user_io io;
- struct nvme_command c;
- unsigned length, meta_len;
- void __user *metadata;
- if (copy_from_user(&io, uio, sizeof(io)))
+ if (unlikely(copy_from_user(&io, uio, size)))
return -EFAULT;
- if (io.flags)
+ if (unlikely(io.flags & ~(NVME_CMD_FUSE_FIRST|NVME_CMD_FUSE_SECOND)))
return -EINVAL;
switch (io.opcode) {
@@ -1488,33 +1512,165 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
return -EINVAL;
}
- length = (io.nblocks + 1) << ns->lba_shift;
- meta_len = (io.nblocks + 1) * ns->ms;
- metadata = nvme_to_user_ptr(io.metadata);
+ nrq->udata = nvme_to_user_ptr(io.addr);
+ nrq->len = (io.nblocks + 1) << ns->lba_shift;
+ nrq->umeta = nvme_to_user_ptr(io.metadata);
+ nrq->mlen = (io.nblocks + 1) * ns->ms;
+ nrq->mseed = lower_32_bits(io.slba);
+ nrq->bio = nrq->meta = NULL;
if (ns->features & NVME_NS_EXT_LBAS) {
- length += meta_len;
- meta_len = 0;
- } else if (meta_len) {
+ nrq->len += nrq->mlen;
+ nrq->mlen = 0;
+ } else if (nrq->mlen) {
if ((io.metadata & 3) || !io.metadata)
return -EINVAL;
}
- memset(&c, 0, sizeof(c));
- c.rw.opcode = io.opcode;
- c.rw.flags = io.flags;
- c.rw.nsid = cpu_to_le32(ns->head->ns_id);
- c.rw.slba = cpu_to_le64(io.slba);
- c.rw.length = cpu_to_le16(io.nblocks);
- c.rw.control = cpu_to_le16(io.control);
- c.rw.dsmgmt = cpu_to_le32(io.dsmgmt);
- c.rw.reftag = cpu_to_le32(io.reftag);
- c.rw.apptag = cpu_to_le16(io.apptag);
- c.rw.appmask = cpu_to_le16(io.appmask);
-
- return nvme_submit_user_cmd(ns->queue, &c,
- nvme_to_user_ptr(io.addr), length,
- metadata, meta_len, lower_32_bits(io.slba), NULL, 0);
+ memset(&nrq->cmd, 0, sizeof(nrq->cmd));
+ nrq->cmd.rw.opcode = io.opcode;
+ nrq->cmd.rw.flags = io.flags;
+ nrq->cmd.rw.nsid = cpu_to_le32(ns->head->ns_id);
+ nrq->cmd.rw.slba = cpu_to_le64(io.slba);
+ nrq->cmd.rw.length = cpu_to_le16(io.nblocks);
+ nrq->cmd.rw.control = cpu_to_le16(io.control);
+ nrq->cmd.rw.dsmgmt = cpu_to_le32(io.dsmgmt);
+ nrq->cmd.rw.reftag = cpu_to_le32(io.reftag);
+ nrq->cmd.rw.apptag = cpu_to_le16(io.apptag);
+ nrq->cmd.rw.appmask = cpu_to_le16(io.appmask);
+
+ return 0;
+}
+
+static struct request *nvme_mk_req_io(struct nvme_ns *ns,
+ struct nvme_user_io_req *nrq,
+ blk_mq_req_flags_t flags, int qid,
+ int timeout)
+{
+ bool write = nvme_is_write(&nrq->cmd);
+ struct request_queue *q = ns->queue;
+ struct gendisk *disk = ns->disk;
+ struct request *rq;
+ struct bio *bio = NULL;
+ void *meta = NULL;
+ int ret;
+
+ rq = nvme_alloc_request(q, &nrq->cmd, flags, qid);
+ if (IS_ERR(rq))
+ return rq;
+
+ rq->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+ nvme_req(rq)->flags |= NVME_REQ_USERCMD;
+
+ if (nrq->udata && nrq->len) {
+ ret = blk_rq_map_user(q, rq, NULL, nrq->udata, nrq->len,
+ GFP_KERNEL);
+ if (ret)
+ goto out;
+ bio = rq->bio;
+ bio->bi_disk = disk;
+ if (disk && nrq->umeta && nrq->mlen) {
+ meta = nvme_add_user_metadata(bio, nrq->umeta, nrq->mlen,
+ nrq->mseed, write);
+ if (IS_ERR(meta)) {
+ ret = PTR_ERR(meta);
+ goto out_unmap;
+ }
+ nrq->meta = meta;
+ }
+ }
+ nrq->bio = bio;
+ return rq;
+out_unmap:
+ if (bio)
+ blk_rq_unmap_user(bio);
+ out:
+ blk_mq_free_request(rq);
+ return ERR_PTR(ret);
+}
+
+static int nvme_unprep_io(struct nvme_user_io_req *nrq,
+ u64 *result)
+{
+ struct request *rq = nrq->rq;
+ int write = nvme_is_write(&nrq->cmd);
+ int ret;
+
+ if (unlikely(nvme_req(rq)->flags & NVME_REQ_CANCELLED))
+ ret = -EINTR;
+ else
+ ret = nvme_req(rq)->status;
+ if (result)
+ *result = le64_to_cpu(nvme_req(rq)->result.u64);
+ if (nrq->meta && !ret && !write) {
+ if (copy_to_user(nrq->umeta, nrq->meta, nrq->mlen))
+ ret = -EFAULT;
+ }
+ nvme_free_io(nrq);
+ return ret;
+}
+
+/* support both NVME_IOCTL_SUBMIT_IO and NVME_IOCTL_SUBMIT_IO32 */
+static int nvme_submit_io(struct nvme_ns *ns, void __user *uio,
+ int size)
+{
+ struct nvme_ctrl *ctrl = ns->ctrl;
+ struct nvme_user_io_req nrq, nrq2;
+ struct request *rq, *rq2;
+ int ret, fused;
+
+ ret = nvme_prep_io(ns, &nrq, uio, size);
+ if (unlikely(ret))
+ return ret;
+ fused = (nrq.cmd.common.flags == NVME_CMD_FUSE_FIRST);
+ if (fused) {
+ if (!(ctrl->ops->flags & NVME_F_PCI_P2PDMA))
+ return -EINVAL;
+ ret = nvme_prep_io(ns, &nrq2, uio+size, size);
+ if (unlikely(ret))
+ return ret;
+ if (unlikely(nrq2.cmd.common.flags != NVME_CMD_FUSE_SECOND))
+ return -EINVAL;
+ } else if (unlikely(nrq.cmd.common.flags)) {
+ return -EINVAL;
+ }
+ rq = nvme_mk_req_io(ns, &nrq, 0, NVME_QID_ANY, 0);
+ if (IS_ERR(rq))
+ return PTR_ERR(rq);
+ nrq.rq = rq;
+ if (fused) {
+ DECLARE_COMPLETION_ONSTACK(wait);
+
+ rq2 = nvme_mk_req_io(ns, &nrq2, BLK_MQ_REQ_NOWAIT,
+ nvme_req_qid(rq), 0);
+ if (IS_ERR(rq2)) {
+ nvme_free_io(&nrq);
+ return PTR_ERR(rq2);
+ }
+ /* point requests at each other */
+ nvme_req(rq)->nrq = nvme_req(rq2);
+ nvme_req(rq2)->nrq = nvme_req(rq);
+ nrq2.rq = rq2;
+
+ rq->cmd_flags |= REQ_NOMERGE;
+ rq2->cmd_flags |= REQ_NOMERGE;
+ rq->end_io_data = &wait;
+ blk_execute_rq_nowait(rq->q, ns->disk, rq, false, nvme_end_sync_rq);
+ nvme_execute_passthru_rq(rq2);
+
+ /*
+ * both will be complete at this point, but nvme spec doesn't
+ * specify cqe ordering for fused operations so wait for the
+ * first to complete as well
+ */
+ wait_for_completion_io(&wait);
+ nvme_unprep_io(&nrq, NULL);
+ ret = nvme_unprep_io(&nrq2, NULL);
+ } else {
+ nvme_execute_passthru_rq(rq);
+ ret = nvme_unprep_io(&nrq, NULL);
+ }
+ return ret;
}
static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
@@ -1671,6 +1827,25 @@ static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
return ret;
}
+#ifdef CONFIG_COMPAT
+struct nvme_user_io32 {
+ __u8 opcode;
+ __u8 flags;
+ __u16 control;
+ __u16 nblocks;
+ __u16 rsvd;
+ __u64 metadata;
+ __u64 addr;
+ __u64 slba;
+ __u32 dsmgmt;
+ __u32 reftag;
+ __u16 apptag;
+ __u16 appmask;
+} __attribute__((__packed__));
+
+#define NVME_IOCTL_SUBMIT_IO32 _IOW('N', 0x42, struct nvme_user_io32)
+#endif
+
static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
@@ -1699,8 +1874,12 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
case NVME_IOCTL_IO_CMD:
ret = nvme_user_cmd(ns->ctrl, ns, argp);
break;
+#ifdef CONFIG_COMPAT
+ case NVME_IOCTL_SUBMIT_IO32:
+ fallthrough; /* structures are identical except size */
+#endif
case NVME_IOCTL_SUBMIT_IO:
- ret = nvme_submit_io(ns, argp);
+ ret = nvme_submit_io(ns, argp, _IOC_SIZE(cmd));
break;
case NVME_IOCTL_IO64_CMD:
ret = nvme_user_cmd64(ns->ctrl, ns, argp);
@@ -1716,41 +1895,11 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return ret;
}
-#ifdef CONFIG_COMPAT
-struct nvme_user_io32 {
- __u8 opcode;
- __u8 flags;
- __u16 control;
- __u16 nblocks;
- __u16 rsvd;
- __u64 metadata;
- __u64 addr;
- __u64 slba;
- __u32 dsmgmt;
- __u32 reftag;
- __u16 apptag;
- __u16 appmask;
-} __attribute__((__packed__));
-
-#define NVME_IOCTL_SUBMIT_IO32 _IOW('N', 0x42, struct nvme_user_io32)
+#ifdef CONFIG_COMPAT
static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
- /*
- * Corresponds to the difference of NVME_IOCTL_SUBMIT_IO
- * between 32 bit programs and 64 bit kernel.
- * The cause is that the results of sizeof(struct nvme_user_io),
- * which is used to define NVME_IOCTL_SUBMIT_IO,
- * are not same between 32 bit compiler and 64 bit compiler.
- * NVME_IOCTL_SUBMIT_IO32 is for 64 bit kernel handling
- * NVME_IOCTL_SUBMIT_IO issued from 32 bit programs.
- * Other IOCTL numbers are same between 32 bit and 64 bit.
- * So there is nothing to do regarding to other IOCTL numbers.
- */
- if (cmd == NVME_IOCTL_SUBMIT_IO32)
- return nvme_ioctl(bdev, mode, NVME_IOCTL_SUBMIT_IO, arg);
-
return nvme_ioctl(bdev, mode, cmd, arg);
}
#else
--
2.27.0
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 22+ messages in thread