| Message ID | 20210730131832.118865-10-jinpu.wang@ionos.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Misc update for RTRS | expand |
On Fri, Jul 30, 2021 at 03:18:31PM +0200, Jack Wang wrote: > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > This commit adds support to reject connection on a specific IB port which > can be specified in the added sysfs entry for the rtrs-server module. > > Example, > > $ echo "mlx4_0 1" > /sys/class/rtrs-server/ctl/disable_port > > When a connection request is received on the above IB port, rtrs_srv > rejects the connection and notifies the client to disable reconnection > attempts. A manual reconnect has to be triggerred in such a case. > > A manual reconnect can be triggered by doing the following, > > echo 1 > /sys/class/rtrs-client/blya/paths/<select-path>/reconnect > > Signed-off-by: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/infiniband/ulp/rtrs/rtrs-clt.c | 10 ++++ > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 82 +++++++++++++++++++++++++- > 2 files changed, 90 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > index 5cce727abca0..21001818e607 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > @@ -1898,6 +1898,7 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con, > struct rdma_cm_event *ev) > { > struct rtrs_sess *s = con->c.sess; > + struct rtrs_clt_sess *sess = to_clt_sess(s); > const struct rtrs_msg_conn_rsp *msg; > const char *rej_msg; > int status, errno; > @@ -1916,6 +1917,15 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con, > rtrs_err(s, > "Connect rejected: status %d (%s), rtrs errno %d\n", > status, rej_msg, errno); > + > + if (errno == -ENETDOWN) { > + /* > + * Stop reconnection attempts > + */ > + sess->reconnect_attempts = -1; > + rtrs_err(s, > + "Disabling auto-reconnect. Trigger a manual reconnect after issue is resolved\n"); > + } > } else { > rtrs_err(s, > "Connect rejected but with malformed message: status %d (%s)\n", > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index cc65cffdc65a..90d833041ccf 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -32,7 +32,9 @@ MODULE_LICENSE("GPL"); > static struct rtrs_rdma_dev_pd dev_pd; > static mempool_t *chunk_pool; > struct class *rtrs_dev_class; > +static struct device *rtrs_dev; > static struct rtrs_srv_ib_ctx ib_ctx; > +static char disabled_port[NAME_MAX]; > > static int __read_mostly max_chunk_size = DEFAULT_MAX_CHUNK_SIZE; > static int __read_mostly sess_queue_depth = DEFAULT_SESS_QUEUE_DEPTH; > @@ -1826,6 +1828,20 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > u16 recon_cnt; > int err = -ECONNRESET; > > + if (strnlen(disabled_port, NAME_MAX) > 0) { > + char ib_device[NAME_MAX]; > + > + snprintf(ib_device, NAME_MAX, "%s %u", cm_id->device->name, cm_id->port_num); > + if (strncmp(disabled_port, ib_device, NAME_MAX) == 0) { > + /* > + * Reject connection attempt on disabled port > + */ > + pr_err("Error: Connection request on a disabled port"); > + err = -ENETDOWN; > + goto reject_w_err; > + } > + } > + > if (len < sizeof(*msg)) { > pr_err("Invalid RTRS connection request\n"); > goto reject_w_err; > @@ -2242,6 +2258,56 @@ static int check_module_params(void) > return 0; > } > > +static ssize_t disable_port_show(struct kobject *kobj, > + struct kobj_attribute *attr, > + char *page) > +{ > + return sysfs_emit(page, "%s\n", disabled_port); > +} > + > +static ssize_t disable_port_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + int ret, len; > + > + if (count > 1 && strnlen(disabled_port, NAME_MAX) > 0) { > + pr_err("A disabled port already exists.\n"); > + return -EINVAL; > + } > + > + ret = strscpy(disabled_port, buf, NAME_MAX); > + if (ret == -E2BIG) { > + pr_err("String too big.\n"); > + disabled_port[0] = '\0'; > + return ret; > + } > + > + len = strlen(disabled_port); > + if (len > 0 && disabled_port[len-1] == '\n') All the "\n" checks in rtrs sysfs looks like cargo cult. You don't need them. And maybe Jason thinks differently, but I don't feel comfortable with such new sysfs file at all. Thanks
On Mon, Aug 2, 2021 at 9:30 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Fri, Jul 30, 2021 at 03:18:31PM +0200, Jack Wang wrote: > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > This commit adds support to reject connection on a specific IB port which > > can be specified in the added sysfs entry for the rtrs-server module. > > > > Example, > > > > $ echo "mlx4_0 1" > /sys/class/rtrs-server/ctl/disable_port > > > > When a connection request is received on the above IB port, rtrs_srv > > rejects the connection and notifies the client to disable reconnection > > attempts. A manual reconnect has to be triggerred in such a case. > > > > A manual reconnect can be triggered by doing the following, > > > > echo 1 > /sys/class/rtrs-client/blya/paths/<select-path>/reconnect > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > --- > > drivers/infiniband/ulp/rtrs/rtrs-clt.c | 10 ++++ > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 82 +++++++++++++++++++++++++- > > 2 files changed, 90 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > > index 5cce727abca0..21001818e607 100644 > > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > > @@ -1898,6 +1898,7 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con, > > struct rdma_cm_event *ev) > > { > > struct rtrs_sess *s = con->c.sess; > > + struct rtrs_clt_sess *sess = to_clt_sess(s); > > const struct rtrs_msg_conn_rsp *msg; > > const char *rej_msg; > > int status, errno; > > @@ -1916,6 +1917,15 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con, > > rtrs_err(s, > > "Connect rejected: status %d (%s), rtrs errno %d\n", > > status, rej_msg, errno); > > + > > + if (errno == -ENETDOWN) { > > + /* > > + * Stop reconnection attempts > > + */ > > + sess->reconnect_attempts = -1; > > + rtrs_err(s, > > + "Disabling auto-reconnect. Trigger a manual reconnect after issue is resolved\n"); > > + } > > } else { > > rtrs_err(s, > > "Connect rejected but with malformed message: status %d (%s)\n", > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > index cc65cffdc65a..90d833041ccf 100644 > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > @@ -32,7 +32,9 @@ MODULE_LICENSE("GPL"); > > static struct rtrs_rdma_dev_pd dev_pd; > > static mempool_t *chunk_pool; > > struct class *rtrs_dev_class; > > +static struct device *rtrs_dev; > > static struct rtrs_srv_ib_ctx ib_ctx; > > +static char disabled_port[NAME_MAX]; > > > > static int __read_mostly max_chunk_size = DEFAULT_MAX_CHUNK_SIZE; > > static int __read_mostly sess_queue_depth = DEFAULT_SESS_QUEUE_DEPTH; > > @@ -1826,6 +1828,20 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > u16 recon_cnt; > > int err = -ECONNRESET; > > > > + if (strnlen(disabled_port, NAME_MAX) > 0) { > > + char ib_device[NAME_MAX]; > > + > > + snprintf(ib_device, NAME_MAX, "%s %u", cm_id->device->name, cm_id->port_num); > > + if (strncmp(disabled_port, ib_device, NAME_MAX) == 0) { > > + /* > > + * Reject connection attempt on disabled port > > + */ > > + pr_err("Error: Connection request on a disabled port"); > > + err = -ENETDOWN; > > + goto reject_w_err; > > + } > > + } > > + > > if (len < sizeof(*msg)) { > > pr_err("Invalid RTRS connection request\n"); > > goto reject_w_err; > > @@ -2242,6 +2258,56 @@ static int check_module_params(void) > > return 0; > > } > > > > +static ssize_t disable_port_show(struct kobject *kobj, > > + struct kobj_attribute *attr, > > + char *page) > > +{ > > + return sysfs_emit(page, "%s\n", disabled_port); > > +} > > + > > +static ssize_t disable_port_store(struct kobject *kobj, > > + struct kobj_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int ret, len; > > + > > + if (count > 1 && strnlen(disabled_port, NAME_MAX) > 0) { > > + pr_err("A disabled port already exists.\n"); > > + return -EINVAL; > > + } > > + > > + ret = strscpy(disabled_port, buf, NAME_MAX); > > + if (ret == -E2BIG) { > > + pr_err("String too big.\n"); > > + disabled_port[0] = '\0'; > > + return ret; > > + } > > + > > + len = strlen(disabled_port); > > + if (len > 0 && disabled_port[len-1] == '\n') > > All the "\n" checks in rtrs sysfs looks like cargo cult. You don't need > them. Thanks. Will change and resend. > > And maybe Jason thinks differently, but I don't feel comfortable with > such new sysfs file at all. > > Thanks
On Mon, Aug 02, 2021 at 04:31:01PM +0200, Haris Iqbal wrote: > On Mon, Aug 2, 2021 at 9:30 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Fri, Jul 30, 2021 at 03:18:31PM +0200, Jack Wang wrote: > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > This commit adds support to reject connection on a specific IB port which > > > can be specified in the added sysfs entry for the rtrs-server module. > > > > > > Example, > > > > > > $ echo "mlx4_0 1" > /sys/class/rtrs-server/ctl/disable_port > > > > > > When a connection request is received on the above IB port, rtrs_srv > > > rejects the connection and notifies the client to disable reconnection > > > attempts. A manual reconnect has to be triggerred in such a case. > > > > > > A manual reconnect can be triggered by doing the following, > > > > > > echo 1 > /sys/class/rtrs-client/blya/paths/<select-path>/reconnect <...> > > > > And maybe Jason thinks differently, but I don't feel comfortable with > > such new sysfs file at all. This part is much more important and should be cleared before resending. > > > > Thanks
On Mon, Aug 2, 2021 at 6:36 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Mon, Aug 02, 2021 at 04:31:01PM +0200, Haris Iqbal wrote: > > On Mon, Aug 2, 2021 at 9:30 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Fri, Jul 30, 2021 at 03:18:31PM +0200, Jack Wang wrote: > > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > > > This commit adds support to reject connection on a specific IB port which > > > > can be specified in the added sysfs entry for the rtrs-server module. > > > > > > > > Example, > > > > > > > > $ echo "mlx4_0 1" > /sys/class/rtrs-server/ctl/disable_port > > > > > > > > When a connection request is received on the above IB port, rtrs_srv > > > > rejects the connection and notifies the client to disable reconnection > > > > attempts. A manual reconnect has to be triggerred in such a case. > > > > > > > > A manual reconnect can be triggered by doing the following, > > > > > > > > echo 1 > /sys/class/rtrs-client/blya/paths/<select-path>/reconnect > > <...> > > > > > > > And maybe Jason thinks differently, but I don't feel comfortable with > > > such new sysfs file at all. > > This part is much more important and should be cleared before resending. Agreed. I will wait for Jason to respond. > > > > > > > Thanks
On Mon, Aug 02, 2021 at 07:43:05PM +0200, Haris Iqbal wrote: > On Mon, Aug 2, 2021 at 6:36 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Mon, Aug 02, 2021 at 04:31:01PM +0200, Haris Iqbal wrote: > > > On Mon, Aug 2, 2021 at 9:30 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > On Fri, Jul 30, 2021 at 03:18:31PM +0200, Jack Wang wrote: > > > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > > > > > This commit adds support to reject connection on a specific IB port which > > > > > can be specified in the added sysfs entry for the rtrs-server module. > > > > > > > > > > Example, > > > > > > > > > > $ echo "mlx4_0 1" > /sys/class/rtrs-server/ctl/disable_port > > > > > > > > > > When a connection request is received on the above IB port, rtrs_srv > > > > > rejects the connection and notifies the client to disable reconnection > > > > > attempts. A manual reconnect has to be triggerred in such a case. > > > > > > > > > > A manual reconnect can be triggered by doing the following, > > > > > > > > > > echo 1 > /sys/class/rtrs-client/blya/paths/<select-path>/reconnect > > > > <...> > > > > > > > > > > And maybe Jason thinks differently, but I don't feel comfortable with > > > > such new sysfs file at all. > > > > This part is much more important and should be cleared before resending. > > Agreed. I will wait for Jason to respond. Based on some past conversation with Greg I'm skeptical he would approve of this kind of usage of sysfs.. It is also very strange that this is under a class directory, I'm starting to think it was a mistake to merge the original sysfs stuff :( Can you do this some other way? Jason
On Fri, Aug 6, 2021 at 3:22 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Aug 02, 2021 at 07:43:05PM +0200, Haris Iqbal wrote: > > On Mon, Aug 2, 2021 at 6:36 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Mon, Aug 02, 2021 at 04:31:01PM +0200, Haris Iqbal wrote: > > > > On Mon, Aug 2, 2021 at 9:30 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > > > On Fri, Jul 30, 2021 at 03:18:31PM +0200, Jack Wang wrote: > > > > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > > > > > > > This commit adds support to reject connection on a specific IB port which > > > > > > can be specified in the added sysfs entry for the rtrs-server module. > > > > > > > > > > > > Example, > > > > > > > > > > > > $ echo "mlx4_0 1" > /sys/class/rtrs-server/ctl/disable_port > > > > > > > > > > > > When a connection request is received on the above IB port, rtrs_srv > > > > > > rejects the connection and notifies the client to disable reconnection > > > > > > attempts. A manual reconnect has to be triggerred in such a case. > > > > > > > > > > > > A manual reconnect can be triggered by doing the following, > > > > > > > > > > > > echo 1 > /sys/class/rtrs-client/blya/paths/<select-path>/reconnect > > > > > > <...> > > > > > > > > > > > > > And maybe Jason thinks differently, but I don't feel comfortable with > > > > > such new sysfs file at all. > > > > > > This part is much more important and should be cleared before resending. > > > > Agreed. I will wait for Jason to respond. > > Based on some past conversation with Greg I'm skeptical he would > approve of this kind of usage of sysfs.. > > It is also very strange that this is under a class directory, I'm > starting to think it was a mistake to merge the original sysfs stuff > :( > > Can you do this some other way? I understand the discomfort. I will try to see if there is another way to do this. In the meanwhile, I will resend the other patches which has been reviewed. Thanks. > > Jason
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c index 5cce727abca0..21001818e607 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c @@ -1898,6 +1898,7 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con, struct rdma_cm_event *ev) { struct rtrs_sess *s = con->c.sess; + struct rtrs_clt_sess *sess = to_clt_sess(s); const struct rtrs_msg_conn_rsp *msg; const char *rej_msg; int status, errno; @@ -1916,6 +1917,15 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con, rtrs_err(s, "Connect rejected: status %d (%s), rtrs errno %d\n", status, rej_msg, errno); + + if (errno == -ENETDOWN) { + /* + * Stop reconnection attempts + */ + sess->reconnect_attempts = -1; + rtrs_err(s, + "Disabling auto-reconnect. Trigger a manual reconnect after issue is resolved\n"); + } } else { rtrs_err(s, "Connect rejected but with malformed message: status %d (%s)\n", diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index cc65cffdc65a..90d833041ccf 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -32,7 +32,9 @@ MODULE_LICENSE("GPL"); static struct rtrs_rdma_dev_pd dev_pd; static mempool_t *chunk_pool; struct class *rtrs_dev_class; +static struct device *rtrs_dev; static struct rtrs_srv_ib_ctx ib_ctx; +static char disabled_port[NAME_MAX]; static int __read_mostly max_chunk_size = DEFAULT_MAX_CHUNK_SIZE; static int __read_mostly sess_queue_depth = DEFAULT_SESS_QUEUE_DEPTH; @@ -1826,6 +1828,20 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, u16 recon_cnt; int err = -ECONNRESET; + if (strnlen(disabled_port, NAME_MAX) > 0) { + char ib_device[NAME_MAX]; + + snprintf(ib_device, NAME_MAX, "%s %u", cm_id->device->name, cm_id->port_num); + if (strncmp(disabled_port, ib_device, NAME_MAX) == 0) { + /* + * Reject connection attempt on disabled port + */ + pr_err("Error: Connection request on a disabled port"); + err = -ENETDOWN; + goto reject_w_err; + } + } + if (len < sizeof(*msg)) { pr_err("Invalid RTRS connection request\n"); goto reject_w_err; @@ -2242,6 +2258,56 @@ static int check_module_params(void) return 0; } +static ssize_t disable_port_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *page) +{ + return sysfs_emit(page, "%s\n", disabled_port); +} + +static ssize_t disable_port_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + int ret, len; + + if (count > 1 && strnlen(disabled_port, NAME_MAX) > 0) { + pr_err("A disabled port already exists.\n"); + return -EINVAL; + } + + ret = strscpy(disabled_port, buf, NAME_MAX); + if (ret == -E2BIG) { + pr_err("String too big.\n"); + disabled_port[0] = '\0'; + return ret; + } + + len = strlen(disabled_port); + if (len > 0 && disabled_port[len-1] == '\n') + disabled_port[len-1] = '\0'; + + return ret; +} + +static struct kobj_attribute rtrs_srv_disable_port_device_attr = + __ATTR(disable_port, 0644, + disable_port_show, disable_port_store); + +static struct attribute *default_attrs[] = { + &rtrs_srv_disable_port_device_attr.attr, + NULL, +}; + +static struct attribute_group default_attr_group = { + .attrs = default_attrs, +}; + +static const struct attribute_group *default_attr_groups[] = { + &default_attr_group, + NULL, +}; + static int __init rtrs_server_init(void) { int err; @@ -2268,15 +2334,26 @@ static int __init rtrs_server_init(void) err = PTR_ERR(rtrs_dev_class); goto out_chunk_pool; } + + rtrs_dev = device_create_with_groups(rtrs_dev_class, NULL, + MKDEV(0, 0), NULL, + default_attr_groups, "ctl"); + if (IS_ERR(rtrs_dev)) { + err = PTR_ERR(rtrs_dev); + goto out_class; + } + rtrs_wq = alloc_workqueue("rtrs_server_wq", 0, 0); if (!rtrs_wq) { err = -ENOMEM; - goto out_dev_class; + goto out_dev; } return 0; -out_dev_class: +out_dev: + device_destroy(rtrs_dev_class, MKDEV(0, 0)); +out_class: class_destroy(rtrs_dev_class); out_chunk_pool: mempool_destroy(chunk_pool); @@ -2287,6 +2364,7 @@ static int __init rtrs_server_init(void) static void __exit rtrs_server_exit(void) { destroy_workqueue(rtrs_wq); + device_destroy(rtrs_dev_class, MKDEV(0, 0)); class_destroy(rtrs_dev_class); mempool_destroy(chunk_pool); rtrs_rdma_dev_pd_deinit(&dev_pd);