nvmet: protect sqhd update by a lock
Subsystems (2)
PATCH v2 1/1 nvmet: protect sqhd update by a lock
In testing target io in read write mix, we did indeed get into cases where sqhd didn't update properly and slowly missed enough updates to shutdown the queue.
Protect the read/modify/set update of sqhd under a lock for coherency.
---
v2: move locks so around update and assignment
drivers/nvme/target/core.c | 5 +++++ drivers/nvme/target/nvmet.h | 1 + 2 files changed, 6 insertions(+)
drivers/nvme/target/core.c +5 -0
@@ -387,12 +387,16 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)static void __nvmet_req_complete (struct nvmet_req *req,u16 status) { +unsigned long flags; +if (status)nvmet_set_status (req, status); +spin_lock_irqsave (&req->sq->sqhd_lock, flags);if (req->sq->size) req->sq->sqhd = (req->sq->sqhd +1 ) % req->sq->size; req->rsp->sq_head =cpu_to_le16 (req->sq->sqhd); +spin_unlock_irqrestore (&req->sq->sqhd_lock, flags); req->rsp->sq_id =cpu_to_le16 (req->sq->qid); req->rsp->command_id = req->cmd->common.command_id;
@@ -425,6 +429,7 @@ void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, sq->size = size; ctrl->sqs[qid] = sq; +spin_lock_init (&sq->sqhd_lock); }static void nvmet_confirm_sq (struct percpu_ref *ref)
drivers/nvme/target/nvmet.h +1 -0
@@ -77,6 +77,7 @@ struct nvmet_sq {u16 sqhd;struct completion free_done;struct completion confirm_done; +spinlock_t sqhd_lock; };/**
Signature · 1 line
2.13.1
PATCH v2 1/1 nvmet: protect sqhd update by a lock
Still somewhat awkward to read, maybe we can fold in a comment on why the lock only touches these two lines and isn't used outside of this function anymore.
Anyways,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH v2 1/1 nvmet: protect sqhd update by a lock
In reply to James Smartdrivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 399
Quoted 8 lines
drivers/nvme/target/core.c +2
-0if (status)nvmet_set_status (req, status); +spin_lock_irqsave (&req->sq->sqhd_lock, flags);if (req->sq->size) req->sq->sqhd = (req->sq->sqhd +1 ) % req->sq->size; req->rsp->sq_head =cpu_to_le16 (req->sq->sqhd); +spin_unlock_irqrestore (&req->sq->sqhd_lock, flags);
What performance impact does this have? I'm really reluctant to put
an irq disabling spinlock into a hot path for a feature that theoretically is in the spec but ignored by every host.
I'd much rather play games with cmpxchg or similar here.
PATCH v2 1/1 nvmet: protect sqhd update by a lock
Quoted 12 lines · Christoph Hellwig
drivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 399
Quoted 8 lines
if (status) nvmet_set_status(req, status);drivers/nvme/target/core.c +2
-0+spin_lock_irqsave (&req->sq->sqhd_lock, flags);if (req->sq->size) req->sq->sqhd = (req->sq->sqhd +1 ) % req->sq->size; req->rsp->sq_head =cpu_to_le16 (req->sq->sqhd); +spin_unlock_irqrestore (&req->sq->sqhd_lock, flags);What performance impact does this have? I'm really reluctant to put
an irq disabling spinlock into a hot path for a feature that theoretically is in the spec but ignored by every host.
Quoted 1 line
I'd much rather play games with cmpxchg or similar here.
It's not ignored by every host. Its ignored by the linux host. There are other hosts out there that follow the spec and pay attention to sqhd - and the linux target would like to be used with them.
I'm open to other alternatives, but in the past, with pci adapter interfaces with similar "ring" index updates, the increment followed by modulo has always proven to be an issue for such tricks.
-- james
PATCH v2 1/1 nvmet: protect sqhd update by a lock
Quoted 14 lines · James Smart
Quoted 11 lines · Christoph Hellwig
drivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 399
Quoted 7 lines
if (status) nvmet_set_status(req, status);drivers/nvme/target/core.c +2
-0+spin_lock_irqsave (&req->sq->sqhd_lock, flags);if (req->sq->size) req->sq->sqhd = (req->sq->sqhd +1 ) % req->sq->size; req->rsp->sq_head =cpu_to_le16 (req->sq->sqhd); +spin_unlock_irqrestore (&req->sq->sqhd_lock, flags);What performance impact does this have? I'm really reluctant to put
an irq disabling spinlock into a hot path for a feature that theoretically is in the spec but ignored by every host.Quoted 1 line
I'd much rather play games with cmpxchg or similar here.
Quoted 7 lines
It's not ignored by every host. Its ignored by the linux host. There are other hosts out there that follow the spec and pay attention to sqhd - and the linux target would like to be used with them.
I'm open to other alternatives, but in the past, with pci adapter interfaces with similar "ring" index updates, the increment followed by modulo has always proven to be an issue for such tricks.
Something like:
do {
old_sqhd = req->sq->sqhd;
new_sqhd = (old + 1) % req->sq->size;
} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) != old_sqhd);should do the job.
PATCH v2 1/1 nvmet: protect sqhd update by a lock
Quoted 4 lines · Christoph Hellwig
do {
old_sqhd = req->sq->sqhd;
new_sqhd = (old + 1) % req->sq->size;
} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) != old_sqhd);sweet. does look good.
I'll repost as v3, but with an ammended title
-- james