| Message ID | 55638a1d2a9f79af8b9a19eb444c5d0a41691352.1477336045.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> +static const char *const nvme_rdma_cm_status_strs[] = { > + [NVME_RDMA_CM_INVALID_LEN] = "invalid length", > + [NVME_RDMA_CM_INVALID_RECFMT] = "invalid record format", > + [NVME_RDMA_CM_INVALID_QID] = "invalid queue id", > + [NVME_RDMA_CM_INVALID_HSQSIZE] = "invalid host sq size", > + [NVME_RDMA_CM_INVALID_HRQSIZE] = "invalid host rq size", > + [NVME_RDMA_CM_NO_RSC] = "resource not found", > + [NVME_RDMA_CM_INVALID_IRD] = "invalid ird", > + [NVME_RDMA_CM_INVALID_ORD] = "Invalid ord", > +}; I think it'd be better to move them to include/linux/nvme-rdma.h and use them for logging on the target side too. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +static const char *const nvme_rdma_cm_status_strs[] = { > > + [NVME_RDMA_CM_INVALID_LEN] = "invalid length", > > + [NVME_RDMA_CM_INVALID_RECFMT] = "invalid record format", > > + [NVME_RDMA_CM_INVALID_QID] = "invalid queue id", > > + [NVME_RDMA_CM_INVALID_HSQSIZE] = "invalid host sq size", > > + [NVME_RDMA_CM_INVALID_HRQSIZE] = "invalid host rq size", > > + [NVME_RDMA_CM_NO_RSC] = "resource not found", > > + [NVME_RDMA_CM_INVALID_IRD] = "invalid ird", > > + [NVME_RDMA_CM_INVALID_ORD] = "Invalid ord", > > +}; > > I think it'd be better to move them to include/linux/nvme-rdma.h > and use them for logging on the target side too. > You want to put this in nvme-rdma.h as-is so the memory for this is allocated in each .c file that includes it? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 24, 2016 at 12:07:13PM -0700, Steve Wise wrote: > Also add nvme cm status strings and use them. > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 25, 2016 at 07:36:25PM +0300, Sagi Grimberg wrote: > I think it'd be better to move them to include/linux/nvme-rdma.h > and use them for logging on the target side too. Hmm, that would mean they get compiled into every source file. Also on the target side we can have free-form messages for whatever the exact rejection reason was, no need to limit us to the enum. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Oct 25, 2016 at 07:36:25PM +0300, Sagi Grimberg wrote: >> I think it'd be better to move them to include/linux/nvme-rdma.h >> and use them for logging on the target side too. > > Hmm, that would mean they get compiled into every source file. "every source file" is exactly two... > Also on the target side we can have free-form messages for whatever > the exact rejection reason was, no need to limit us to the enum. Thought it'd be nice to use the same verbosity on both ends, just a suggestion though... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/24/2016 12:09 PM, Steve Wise wrote: > +static const char *const nvme_rdma_cm_status_strs[] = { > + [NVME_RDMA_CM_INVALID_LEN] = "invalid length", > + [NVME_RDMA_CM_INVALID_RECFMT] = "invalid record format", > + [NVME_RDMA_CM_INVALID_QID] = "invalid queue id", > + [NVME_RDMA_CM_INVALID_HSQSIZE] = "invalid host sq size", > + [NVME_RDMA_CM_INVALID_HRQSIZE] = "invalid host rq size", > + [NVME_RDMA_CM_NO_RSC] = "resource not found", > + [NVME_RDMA_CM_INVALID_IRD] = "invalid ird", > + [NVME_RDMA_CM_INVALID_ORD] = "Invalid ord", > +}; Please capitalize abbreviations (SQ, RQ, ID, IRD and ORD). > +static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status) > +{ > + size_t index = status; > + > + if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) || > + !nvme_rdma_cm_status_strs[index]) > + return "unrecognized reason"; > + else > + return nvme_rdma_cm_status_strs[index]; > +}; Please consider rewriting this if-statement such that the recognized reason scenario occurs first. > + rej_msg = rdma_reject_msg(cm_id, status); > + rej_data = (struct nvme_rdma_cm_rej *) > + rdma_consumer_reject_data(cm_id, ev, &rej_data_len); Please reorder the patches in this patch series such that it doesn't break a bisect. rdma_consumer_reject_data() should be introduced before it is used. Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On 10/24/2016 12:09 PM, Steve Wise wrote: > > +static const char *const nvme_rdma_cm_status_strs[] = { > > + [NVME_RDMA_CM_INVALID_LEN] = "invalid length", > > + [NVME_RDMA_CM_INVALID_RECFMT] = "invalid record format", > > + [NVME_RDMA_CM_INVALID_QID] = "invalid queue id", > > + [NVME_RDMA_CM_INVALID_HSQSIZE] = "invalid host sq size", > > + [NVME_RDMA_CM_INVALID_HRQSIZE] = "invalid host rq size", > > + [NVME_RDMA_CM_NO_RSC] = "resource not found", > > + [NVME_RDMA_CM_INVALID_IRD] = "invalid ird", > > + [NVME_RDMA_CM_INVALID_ORD] = "Invalid ord", > > +}; > > Please capitalize abbreviations (SQ, RQ, ID, IRD and ORD). > will do. > > +static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status) > > +{ > > + size_t index = status; > > + > > + if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) || > > + !nvme_rdma_cm_status_strs[index]) > > + return "unrecognized reason"; > > + else > > + return nvme_rdma_cm_status_strs[index]; > > +}; > > Please consider rewriting this if-statement such that the recognized > reason scenario occurs first. > sure. > > + rej_msg = rdma_reject_msg(cm_id, status); > > + rej_data = (struct nvme_rdma_cm_rej *) > > + rdma_consumer_reject_data(cm_id, ev, &rej_data_len); > > Please reorder the patches in this patch series such that it doesn't > break a bisect. rdma_consumer_reject_data() should be introduced before > it is used. > rdma_consumer_reject_data() is introduced in patch 3. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gVHVlLCAyMDE2LTEwLTI1IGF0IDEzOjIwIC0wNTAwLCBTdGV2ZSBXaXNlIHdyb3RlOg0KPiA+ IA0KPiA+IE9uIDEwLzI0LzIwMTYgMTI6MDkgUE0sIFN0ZXZlIFdpc2Ugd3JvdGU6DQo+ID4gPiAN Cj4gPiA+ICsJcmVqX21zZyA9IHJkbWFfcmVqZWN0X21zZyhjbV9pZCwgc3RhdHVzKTsNCj4gPiA+ ICsJcmVqX2RhdGEgPSAoc3RydWN0IG52bWVfcmRtYV9jbV9yZWogKikNCj4gPiA+ICsJCcKgwqDC oHJkbWFfY29uc3VtZXJfcmVqZWN0X2RhdGEoY21faWQsIGV2LA0KPiA+ID4gJnJlal9kYXRhX2xl bik7DQo+ID4gDQo+ID4gUGxlYXNlIHJlb3JkZXIgdGhlIHBhdGNoZXMgaW4gdGhpcyBwYXRjaCBz ZXJpZXMgc3VjaCB0aGF0IGl0DQo+ID4gZG9lc24ndA0KPiA+IGJyZWFrIGEgYmlzZWN0LiByZG1h X2NvbnN1bWVyX3JlamVjdF9kYXRhKCkgc2hvdWxkIGJlIGludHJvZHVjZWQNCj4gPiBiZWZvcmUN Cj4gPiBpdCBpcyB1c2VkLg0KPiANCj4gcmRtYV9jb25zdW1lcl9yZWplY3RfZGF0YSgpIGlzIGlu dHJvZHVjZWQgaW4gcGF0Y2ggMy4NCg0KSGVsbG8gU3RldmUsDQoNClRoYW5rcyBmb3IgdGhlIGZl ZWRiYWNrLiBBcHBhcmVudGx5IG15IGUtbWFpbCBjbGllbnQgZGlzcGxheWVkIHRoZQ0KcGF0Y2hl cyBpbiB0aGUgd3Jvbmcgb3JkZXIgLi4uDQoNCkJhcnQu -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index fbdb226..b6a6b50 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -43,6 +43,28 @@ #define NVME_RDMA_MAX_INLINE_SEGMENTS 1 +static const char *const nvme_rdma_cm_status_strs[] = { + [NVME_RDMA_CM_INVALID_LEN] = "invalid length", + [NVME_RDMA_CM_INVALID_RECFMT] = "invalid record format", + [NVME_RDMA_CM_INVALID_QID] = "invalid queue id", + [NVME_RDMA_CM_INVALID_HSQSIZE] = "invalid host sq size", + [NVME_RDMA_CM_INVALID_HRQSIZE] = "invalid host rq size", + [NVME_RDMA_CM_NO_RSC] = "resource not found", + [NVME_RDMA_CM_INVALID_IRD] = "invalid ird", + [NVME_RDMA_CM_INVALID_ORD] = "Invalid ord", +}; + +static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status) +{ + size_t index = status; + + if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) || + !nvme_rdma_cm_status_strs[index]) + return "unrecognized reason"; + else + return nvme_rdma_cm_status_strs[index]; +}; + /* * We handle AEN commands ourselves and don't even let the * block layer know about them. @@ -1222,17 +1244,25 @@ out_destroy_queue_ib: static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue, struct rdma_cm_event *ev) { - if (ev->param.conn.private_data_len) { - struct nvme_rdma_cm_rej *rej = - (struct nvme_rdma_cm_rej *)ev->param.conn.private_data; + struct rdma_cm_id *cm_id = queue->cm_id; + int status = ev->status; + const char *rej_msg; + struct nvme_rdma_cm_rej *rej_data; + u8 rej_data_len; + + rej_msg = rdma_reject_msg(cm_id, status); + rej_data = (struct nvme_rdma_cm_rej *) + rdma_consumer_reject_data(cm_id, ev, &rej_data_len); + + if (rej_data && rej_data_len >= sizeof(u16)) { + u16 sts = le16_to_cpu(rej_data->sts); dev_err(queue->ctrl->ctrl.device, - "Connect rejected, status %d.", le16_to_cpu(rej->sts)); - /* XXX: Think of something clever to do here... */ - } else { + "Connect rejected: status %d (%s) nvme status %d (%s).\n", + status, rej_msg, sts, nvme_rdma_cm_msg(sts)); + } else dev_err(queue->ctrl->ctrl.device, - "Connect rejected, no private data.\n"); - } + "Connect rejected: status %d (%s).\n", status, rej_msg); return -ECONNRESET; }
Also add nvme cm status strings and use them. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/nvme/host/rdma.c | 46 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-)