open-iscsi / tcmu-runner

A daemon that handles the userspace side of the LIO TCM-User backstore.
Apache License 2.0
189 stars 149 forks source link

Feature request and offer to PR: provide a "no netlink" mode for libtcmu #678

Open geordieintheshellcode opened 2 years ago

geordieintheshellcode commented 2 years ago

Some folks might be using libtcmu in their own applications which don't hook into TCMU runner, much like the consumer.c example. In those cases, it can be useful to avoid using netlink altogether. As I'm sure you are aware, it's almost impossible to have more than one user of TCMU when netlink is involved. This is because the kernel broadcasts device events to all TCMU listeners over netlink and simply accepts the first response it receives. This can lead to the following scenario:

  1. Application A creates a TCMU backstore with handler=app_a
  2. The kernel (target_core_user) broadcasts the netlink message to all instances of libtcmu
  3. Application B receives the message first. Application B doesn't have a handler for app_a, so it replies with an error code to the kernel.
  4. The kernel receives the error and aborts backstore creation.
  5. Application A wonders why its backstore creation failed.

Users can avoid using netlink altogether by setting nl_reply_supported=-1 at backing store creation time. This is great as multiple applications using TCMU can now coexist on the same machine. The problem is we don't have a nice way to utilise libtcmu in this model. We need a way to notify libtcmu that a device has been created, reconfigured, removed so the relevant handlers can be called. Currently, this is done via netlink.

Would you accept a PR that allows users to utilise all the great features of libtcmu, but without using netlink? The PR would be roughly as follows:

This change benefits all users of libtcmu who are managing their own devices and who would like to be able to run multiple instances of their app on one machine and/or coexist with other users of libtcmu/tcmu-runner.

Great work with the library and let me know if this feature sounds interesting!

lxbsz commented 2 years ago

@geordieintheshellcode

This sounds reasonable to me.

BTW, AFAIK currently the target_core_user.c only supports the netlink method, if you want to support this, you need to code the target_core_user.c first and possibly you could do just as the kernel nbd.c driver, which also supports ioctl, does.

geordieintheshellcode commented 2 years ago

This patch allows us to disable netlink on "per-device" basis. It was merged into the kernel mainline on Sep 13 14:01:22 2017 +0900:

commit b849b4567549d5a54ab34ffacfd48fca05e8b34e
Author: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
Date:   Wed Sep 13 14:01:22 2017 +0900

    target: Add netlink command reply supported option for each device

    Currently netlink command reply support option
    (TCMU_ATTR_SUPP_KERN_CMD_REPLY) can be enabled only on module
    scope. Because of that, once an application enables the netlink
    command reply support, all applications using target_core_user.ko
    would be expected to support the netlink reply. To make matters worse,
    users will not be able to add a device via configfs manually.

    To fix these issues, this patch adds an option to make netlink command
    reply disabled on each device through configfs. Original
    TCMU_ATTR_SUPP_KERN_CMD_REPLY is still enabled on module scope to keep
    backward-compatibility and used by default, however once users set
    nl_reply_supported=<NAGATIVE_VALUE> via configfs for a particular
    device, the device disables the netlink command reply support.

    Signed-off-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
    Reviewed-by: Mike Christie <mchristi@redhat.com>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

When users create the backstore they can set nl_reply_supported=-1 via configfs and this disables netlink for that backstore. So looks like we are all good! I've PoC'd this on my machine and it works great. I'll raise a PR some time in the next week hopefully.

lxbsz commented 2 years ago

Sounds nice.