On Mon, Aug 12, 2024 at 12:59 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> On Mon, Aug 12, 2024 at 12:39:06PM +0200, Haris Iqbal wrote:
> > On Mon, Aug 12, 2024 at 12:34 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Aug 12, 2024 at 12:16:19PM +0200, Haris Iqbal wrote:
> > > > On Sun, Aug 11, 2024 at 10:38 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Aug 09, 2024 at 03:15:26PM +0200, Md Haris Iqbal wrote:
> > > > > > Do not allow opening RTRS server if it is already in use and print
> > > > > > proper error message.
> > > > >
> > > > > 1. How is it even possible? I see only one call to rtrs_srv_open() and
> > > > > it is happening when the driver is loaded.
> > > >
> > > > rtrs_srv_open() is NOT called during RTRS driver load. It is called
> > > > during RNBD driver load, which is a client which uses RTRS.
> > > > RTRS server currently works with only a single client. Hence if, while
> > > > in use by RNBD, another driver wants to use RTRS and calls
> > > > rtrs_srv_open(), it should fail.
> > >
> > > ➜ kernel git:(rdma-next) ✗ git grep rtrs_srv_open
> > > drivers/block/rnbd/rnbd-srv.c: rtrs_ctx = rtrs_srv_open(&rtrs_ops, port_nr); <---- SINGLE CALL
> > > drivers/block/rnbd/rnbd-srv.c: pr_err("rtrs_srv_open(), err: %pe\n", rtrs_ctx);
> > > drivers/infiniband/ulp/rtrs/rtrs-srv.c: * rtrs_srv_open() - open RTRS server context
> > > drivers/infiniband/ulp/rtrs/rtrs-srv.c:struct rtrs_srv_ctx *rtrs_srv_open(struct rtrs_srv_ops *ops, u16 port)
> > > drivers/infiniband/ulp/rtrs/rtrs-srv.c:EXPORT_SYMBOL(rtrs_srv_open);
> > > drivers/infiniband/ulp/rtrs/rtrs.h:struct rtrs_srv_ctx *rtrs_srv_open(struct rtrs_srv_ops *ops, u16 port);
> > >
> > > 807 static int __init rnbd_srv_init_module(void)
> > > 808 {
> > > 809 int err = 0;
> > > 810
> > > 811 BUILD_BUG_ON(sizeof(struct rnbd_msg_hdr) != 4);
> > > 812 BUILD_BUG_ON(sizeof(struct rnbd_msg_sess_info) != 36);
> > > 813 BUILD_BUG_ON(sizeof(struct rnbd_msg_sess_info_rsp) != 36);
> > > 814 BUILD_BUG_ON(sizeof(struct rnbd_msg_open) != 264);
> > > 815 BUILD_BUG_ON(sizeof(struct rnbd_msg_close) != 8);
> > > 816 BUILD_BUG_ON(sizeof(struct rnbd_msg_open_rsp) != 56);
> > > 817 rtrs_ops = (struct rtrs_srv_ops) {
> > > 818 .rdma_ev = rnbd_srv_rdma_ev,
> > > 819 .link_ev = rnbd_srv_link_ev,
> > > 820 };
> > > 821 rtrs_ctx = rtrs_srv_open(&rtrs_ops, port_nr);
> > > 822 if (IS_ERR(rtrs_ctx)) {
> > > 823 pr_err("rtrs_srv_open(), err: %pe\n", rtrs_ctx); <---- ALREADY PRINTED ERROR
> > > 824 return PTR_ERR(rtrs_ctx);
> > > 825 }
> > >
> > > ...
> > >
> > > 843 module_init(rnbd_srv_init_module); <---- SINGLE CALL
> > >
> > > Upstream code has only on RNBD and one RTRS.
> >
> > Yes. But they are different drivers. RTRS as a stand-alone ULP does
> > not know about RNBD or for that matter any other client driver, which
> > may use it, either out of tree or in the future. If RTRS can serve
> > only a single client, then it should should have protection for
> > multiple calls to *_open().
>
> For now, there is only one upstream client and server.
In my understanding, its the general rule of abstraction that this
type of limitation is handled where it exists.
>
> I want to remind you that during initial submission of RTR code, the
> feedback was that this ULP shouldn't exist in first place and right
> thing to do it is to use NVMe over fabrics.
>
> So chances that we will have real out-of-tree client are very low.
One reason for us to write this patch is that we are working on
another client which uses RTRS. We could have kept this change
out-of-tree, but frankly, it felt right to add this protection.
>
> Thanks
>
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > > 2. You already print an error message, why do you need to add another
> > > > > one?
> > > >
> > > > This patch adds only a single error print, in function
> > > > rtrs_srv_open(). And it has not other print message. Am I missing
> > > > something?
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@xxxxxxxxx>
> > > > > > Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxx>
> > > > > > Signed-off-by: Grzegorz Prajsner <grzegorz.prajsner@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 27 +++++++++++++++++++++++---
> > > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 +
> > > > > > 2 files changed, 25 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > > index 1d33efb8fb03..fb67b58a7f62 100644
> > > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > > @@ -2174,9 +2174,18 @@ struct rtrs_srv_ctx *rtrs_srv_open(struct rtrs_srv_ops *ops, u16 port)
> > > > > > struct rtrs_srv_ctx *ctx;
> > > > > > int err;
> > > > > >
> > > > > > + mutex_lock(&ib_ctx.rtrs_srv_ib_mutex);
> > > > > > + if (ib_ctx.srv_ctx) {
> > > > > > + pr_err("%s: Already in use.\n", __func__);
> > > > > > + ctx = ERR_PTR(-EEXIST);
> > > > > > + goto out;
> > > > > > + }
> > > > > > +
> > > > > > ctx = alloc_srv_ctx(ops);
> > > > > > - if (!ctx)
> > > > > > - return ERR_PTR(-ENOMEM);
> > > > > > + if (!ctx) {
> > > > > > + ctx = ERR_PTR(-ENOMEM);
> > > > > > + goto out;
> > > > > > + }
> > > > > >
> > > > > > mutex_init(&ib_ctx.ib_dev_mutex);
> > > > > > ib_ctx.srv_ctx = ctx;
> > > > > > @@ -2185,9 +2194,11 @@ struct rtrs_srv_ctx *rtrs_srv_open(struct rtrs_srv_ops *ops, u16 port)
> > > > > > err = ib_register_client(&rtrs_srv_client);
> > > > > > if (err) {
> > > > > > free_srv_ctx(ctx);
> > > > > > - return ERR_PTR(err);
> > > > > > + ctx = ERR_PTR(err);
> > > > > > }
> > > > > >
> > > > > > +out:
> > > > > > + mutex_unlock(&ib_ctx.rtrs_srv_ib_mutex);
> > > > > > return ctx;
> > > > > > }
> > > > > > EXPORT_SYMBOL(rtrs_srv_open);
> > > > > > @@ -2221,10 +2232,16 @@ static void close_ctx(struct rtrs_srv_ctx *ctx)
> > > > > > */
> > > > > > void rtrs_srv_close(struct rtrs_srv_ctx *ctx)
> > > > > > {
> > > > > > + mutex_lock(&ib_ctx.rtrs_srv_ib_mutex);
> > > > > > + WARN_ON(ib_ctx.srv_ctx != ctx);
> > > > > > +
> > > > > > ib_unregister_client(&rtrs_srv_client);
> > > > > > mutex_destroy(&ib_ctx.ib_dev_mutex);
> > > > > > close_ctx(ctx);
> > > > > > free_srv_ctx(ctx);
> > > > > > +
> > > > > > + ib_ctx.srv_ctx = NULL;
> > > > > > + mutex_unlock(&ib_ctx.rtrs_srv_ib_mutex);
> > > > > > }
> > > > > > EXPORT_SYMBOL(rtrs_srv_close);
> > > > > >
> > > > > > @@ -2282,6 +2299,9 @@ static int __init rtrs_server_init(void)
> > > > > > goto out_dev_class;
> > > > > > }
> > > > > >
> > > > > > + mutex_init(&ib_ctx.rtrs_srv_ib_mutex);
> > > > > > + ib_ctx.srv_ctx = NULL;
> > > > > > +
> > > > > > return 0;
> > > > > >
> > > > > > out_dev_class:
> > > > > > @@ -2292,6 +2312,7 @@ static int __init rtrs_server_init(void)
> > > > > >
> > > > > > static void __exit rtrs_server_exit(void)
> > > > > > {
> > > > > > + mutex_destroy(&ib_ctx.rtrs_srv_ib_mutex);
> > > > > > destroy_workqueue(rtrs_wq);
> > > > > > class_unregister(&rtrs_dev_class);
> > > > > > rtrs_rdma_dev_pd_deinit(&dev_pd);
> > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> > > > > > index 5e325b82ff33..4924dde0a708 100644
> > > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> > > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> > > > > > @@ -127,6 +127,7 @@ struct rtrs_srv_ib_ctx {
> > > > > > u16 port;
> > > > > > struct mutex ib_dev_mutex;
> > > > > > int ib_dev_count;
> > > > > > + struct mutex rtrs_srv_ib_mutex;
> > > > > > };
> > > > > >
> > > > > > extern const struct class rtrs_dev_class;
> > > > > > --
> > > > > > 2.25.1
> > > > > >
