Re: [PATCH for-next 01/13] RDMA/rtrs-srv: Make rtrs_srv_open fail if its a second call

[Leon Romanovsky <leon@kernel.org>]

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@kernel.org> 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.

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@ionos.com>
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > Signed-off-by: Grzegorz Prajsner <grzegorz.prajsner@ionos.com>
> > > ---
> > >  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
> > >