On Mon, Aug 12, 2024 at 2:15 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> 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@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.
>
> 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.
Okay
>
> Thanks
