On Mon, Nov 14, 2022 at 9:45 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> wrote:
>
>
>
> On 11/14/22 4:24 PM, Jinpu Wang wrote:
> > On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> wrote:
> >> Hi Jinpu,
> >>
> >> On 11/14/22 3:39 PM, Jinpu Wang wrote:
> >>> Hi Guoqing,
> >>>
> >>> Thx for the patch, see comments below.
> >>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> wrote:
> >>>> The ib_dev_count is supposed to track the number of added ib devices
> >>>> which is only used in rtrs_srv_{add,remove}_one.
> >>>>
> >>>> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
> >>>> -> rtrs_srv_open -> ib_register_client -> client->add which should
> >>>> happen only once.
> >>> client->add is call per ib_device, eg:
> >>> jwang@xxxxxxxxxxxx:~$ ls -l /sys/class/infiniband/mlx5_*
> >>> lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_0 ->
> >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0
> >>> lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_1 ->
> >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1
> >>> rtrs will be call twice for mlx5_0 and mlx5_1 devices
> >> Ah, yes.
> >>
> >> But still we can only load/unload module once, I guess it was used to avoid
> >> racy condition (concurrent loading/unloading module?), could you elaborate
> >> why it is needed?
> > The change was introduced due to a bug report, you can follow the
> > discussion here for the history and reason:
> > https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@xxxxxxxxxxxxxxx/
>
> Thanks for the link.
>
> I probably missed something but I don't know how rnbd_server module can be
> initialized before cma module since we have the dependency chain as
> follows.
Hi Guoqing,
One of the ways this was happening was, when one builds the RNBD/RTRS
module into the kernel bzImage and then boots up the kernel. Depending
on the module init sequence, if the RNBD/RTRS modules are picked
before the RDMA one, then this issue would hit.
With the changes, RNBD/RTRS just register itself to ib. If any devices
are present at that moment, for example when the module is modprob'ed
later into the kernel, then .add gets called through
ib_register_client. If no devices are present, for example in case if
the module is built into the bzImage, and is inserted before RDMA
module, then ib_register_client simply registers RTRS, and returns
without calling .add
Now, when RDMA gets initialized, and it detects devices, then .add is
called for each device, which will land into rtrs_srv_add_one
>
> INFINIBAND_RTRS_SERVER depends on INFINIBAND_ADDR_TRANS
> BLK_DEV_RNBD_SERVER depends on INFINIBAND_RTRS_SERVER
>
> But commit 558d52b2976b ("RDMA/rtrs-srv: Incorporate ib_register_client into
> rtrs server init") did mention this.
>
> "and if the rnbd_server module is initialized before RDMA cma module, a
> null ptr
> dereference occurs during the RDMA bind operation."
>
> Thanks,
> Guoqing
