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 01:17:11PM +0200, Haris Iqbal wrote:
> On Mon, Aug 12, 2024 at 12:59 PM Leon Romanovsky <leon@kernel.org> 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@kernel.org> 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@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.
> > >
> > > 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.

We have such protection and it is called "monolithic kernel".

> 
> >
> > 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.

So once you will have this client upstream, we can discuss this change.

Thanks