* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
@ 2017-09-13 15:10 ` Bryant G. Ly
2017-09-13 15:58 ` Mike Christie
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Bryant G. Ly @ 2017-09-13 15:10 UTC (permalink / raw)
To: target-devel
On 9/13/17 12:01 AM, Kenjiro Nakayama wrote:
> 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>
> ---
> drivers/target/target_core_user.c | 59 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 942d094269fb..709c27ed4206 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -150,6 +150,8 @@ struct tcmu_dev {
> wait_queue_head_t nl_cmd_wq;
>
> char dev_config[TCMU_CONFIG_LEN];
> +
> + int nl_reply_supported;
> };
>
> #define TCMU_DEV(_se_dev) container_of(_se_dev, struct tcmu_dev, se_dev)
> @@ -1306,6 +1308,10 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd)
>
> if (!tcmu_kern_cmd_reply_supported)
> return;
> +
> + if (udev->nl_reply_supported <= 0)
> + return;
> +
> relock:
> spin_lock(&udev->nl_cmd_lock);
>
> @@ -1332,6 +1338,9 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev)
> if (!tcmu_kern_cmd_reply_supported)
> return 0;
>
> + if (udev->nl_reply_supported <= 0)
> + return 0;
> +
> pr_debug("sleeping for nl reply\n");
> wait_for_completion(&nl_cmd->complete);
>
> @@ -1506,6 +1515,12 @@ static int tcmu_configure_device(struct se_device *dev)
> dev->dev_attrib.emulate_write_cache = 0;
> dev->dev_attrib.hw_queue_depth = 128;
>
> + /* If user didn't explicitly disable netlink reply support, use
> + * module scope setting.
> + */
> + if (udev->nl_reply_supported >= 0)
> + udev->nl_reply_supported = tcmu_kern_cmd_reply_supported;
> +
> /*
> * Get a ref incase userspace does a close on the uio device before
> * LIO has initiated tcmu_free_device.
> @@ -1610,7 +1625,7 @@ static void tcmu_destroy_device(struct se_device *dev)
>
> enum {
> Opt_dev_config, Opt_dev_size, Opt_hw_block_size, Opt_hw_max_sectors,
> - Opt_err,
> + Opt_nl_reply_supported, Opt_err,
> };
>
> static match_table_t tokens = {
> @@ -1618,6 +1633,7 @@ static match_table_t tokens = {
> {Opt_dev_size, "dev_size=%u"},
> {Opt_hw_block_size, "hw_block_size=%u"},
> {Opt_hw_max_sectors, "hw_max_sectors=%u"},
> + {Opt_nl_reply_supported, "nl_reply_supported=%d"},
> {Opt_err, NULL}
> };
>
> @@ -1692,6 +1708,18 @@ static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev,
> ret = tcmu_set_dev_attrib(&args[0],
> &(dev->dev_attrib.hw_max_sectors));
> break;
> + case Opt_nl_reply_supported:
> + arg_p = match_strdup(&args[0]);
> + if (!arg_p) {
> + ret = -ENOMEM;
> + break;
> + }
> + ret = kstrtol(arg_p, 0,
> + (long int *) &udev->nl_reply_supported);
> + kfree(arg_p);
> + if (ret < 0)
> + pr_err("kstrtoul() failed for nl_reply_supported=\n");
> + break;
> default:
> break;
> }
> @@ -1842,6 +1870,34 @@ static ssize_t tcmu_dev_size_store(struct config_item *item, const char *page,
> }
> CONFIGFS_ATTR(tcmu_, dev_size);
>
> +static ssize_t tcmu_nl_reply_supported_show(struct config_item *item,
> + char *page)
> +{
> + struct se_dev_attrib *da = container_of(to_config_group(item),
> + struct se_dev_attrib, da_group);
> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
> +
> + return snprintf(page, PAGE_SIZE, "%d\n", udev->nl_reply_supported);
> +}
> +
> +static ssize_t tcmu_nl_reply_supported_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct se_dev_attrib *da = container_of(to_config_group(item),
> + struct se_dev_attrib, da_group);
> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
> + s8 val;
> + int ret;
> +
> + ret = kstrtos8(page, 0, &val);
> + if (ret < 0)
> + return ret;
> +
> + udev->nl_reply_supported = val;
> + return count;
> +}
> +CONFIGFS_ATTR(tcmu_, nl_reply_supported);
> +
> static ssize_t tcmu_emulate_write_cache_show(struct config_item *item,
> char *page)
> {
> @@ -1884,6 +1940,7 @@ static struct configfs_attribute *tcmu_attrib_attrs[] = {
> &tcmu_attr_dev_config,
> &tcmu_attr_dev_size,
> &tcmu_attr_emulate_write_cache,
> + &tcmu_attr_nl_reply_supported,
> NULL,
> };
>
The problem with this patch is that for tcmu-runner/targetcli-fb users
the code will depend
on the genl_info structure that contains whether or not a netlink reply
is supported. You will
need to fix that path where if you were to update the configfs to
disable netlink reply and if a user
already has stuff setup then things will get messy. Userspace will think
netlink reply is supported,
but kernel doesn't expect one.
-Bryant
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
2017-09-13 15:10 ` Bryant G. Ly
@ 2017-09-13 15:58 ` Mike Christie
2017-09-13 16:04 ` Mike Christie
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2017-09-13 15:58 UTC (permalink / raw)
To: target-devel
On 09/13/2017 12:01 AM, Kenjiro Nakayama wrote:
> 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
I am ok with the idea for the patch, but what type of setup do you have
where there are multiple applications using the interface and some
support it and some do not? Is it because tcmu-runner is starting by
default due to something like a distro systemd unit file and then you
also have your app running too?
Also, if you do not implement sync netlink support, how does your daemon
tell the kernel if the device failed to setup in the daemon? For
deletion how did you work around the uio crashes and leaks?
> would be expected to support the netlink reply. To make matters worse,
> users will not be able to add a device via configfs manually.
>
What is the specific issue with manually setting it up through configfs?
rtslib/tagretcli use the same configfs operations and does not know what
tcmu and daemons like tcmu-runner support.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
2017-09-13 15:10 ` Bryant G. Ly
2017-09-13 15:58 ` Mike Christie
@ 2017-09-13 16:04 ` Mike Christie
2017-09-13 18:35 ` Nakayama Kenjiro
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2017-09-13 16:04 UTC (permalink / raw)
To: target-devel
On 09/13/2017 10:10 AM, Bryant G. Ly wrote:
>
> On 9/13/17 12:01 AM, Kenjiro Nakayama wrote:
>
>> 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>
>> ---
>> drivers/target/target_core_user.c | 59
>> ++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 58 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_user.c
>> b/drivers/target/target_core_user.c
>> index 942d094269fb..709c27ed4206 100644
>> --- a/drivers/target/target_core_user.c
>> +++ b/drivers/target/target_core_user.c
>> @@ -150,6 +150,8 @@ struct tcmu_dev {
>> wait_queue_head_t nl_cmd_wq;
>>
>> char dev_config[TCMU_CONFIG_LEN];
>> +
>> + int nl_reply_supported;
>> };
>>
>> #define TCMU_DEV(_se_dev) container_of(_se_dev, struct tcmu_dev,
>> se_dev)
>> @@ -1306,6 +1308,10 @@ static void tcmu_init_genl_cmd_reply(struct
>> tcmu_dev *udev, int cmd)
>>
>> if (!tcmu_kern_cmd_reply_supported)
>> return;
>> +
>> + if (udev->nl_reply_supported <= 0)
>> + return;
>> +
>> relock:
>> spin_lock(&udev->nl_cmd_lock);
>>
>> @@ -1332,6 +1338,9 @@ static int tcmu_wait_genl_cmd_reply(struct
>> tcmu_dev *udev)
>> if (!tcmu_kern_cmd_reply_supported)
>> return 0;
>>
>> + if (udev->nl_reply_supported <= 0)
>> + return 0;
>> +
>> pr_debug("sleeping for nl reply\n");
>> wait_for_completion(&nl_cmd->complete);
>>
>> @@ -1506,6 +1515,12 @@ static int tcmu_configure_device(struct
>> se_device *dev)
>> dev->dev_attrib.emulate_write_cache = 0;
>> dev->dev_attrib.hw_queue_depth = 128;
>>
>> + /* If user didn't explicitly disable netlink reply support, use
>> + * module scope setting.
>> + */
>> + if (udev->nl_reply_supported >= 0)
>> + udev->nl_reply_supported = tcmu_kern_cmd_reply_supported;
>> +
>> /*
>> * Get a ref incase userspace does a close on the uio device before
>> * LIO has initiated tcmu_free_device.
>> @@ -1610,7 +1625,7 @@ static void tcmu_destroy_device(struct se_device
>> *dev)
>>
>> enum {
>> Opt_dev_config, Opt_dev_size, Opt_hw_block_size,
>> Opt_hw_max_sectors,
>> - Opt_err,
>> + Opt_nl_reply_supported, Opt_err,
>> };
>>
>> static match_table_t tokens = {
>> @@ -1618,6 +1633,7 @@ static match_table_t tokens = {
>> {Opt_dev_size, "dev_size=%u"},
>> {Opt_hw_block_size, "hw_block_size=%u"},
>> {Opt_hw_max_sectors, "hw_max_sectors=%u"},
>> + {Opt_nl_reply_supported, "nl_reply_supported=%d"},
>> {Opt_err, NULL}
>> };
>>
>> @@ -1692,6 +1708,18 @@ static ssize_t
>> tcmu_set_configfs_dev_params(struct se_device *dev,
>> ret = tcmu_set_dev_attrib(&args[0],
>> &(dev->dev_attrib.hw_max_sectors));
>> break;
>> + case Opt_nl_reply_supported:
>> + arg_p = match_strdup(&args[0]);
>> + if (!arg_p) {
>> + ret = -ENOMEM;
>> + break;
>> + }
>> + ret = kstrtol(arg_p, 0,
>> + (long int *) &udev->nl_reply_supported);
>> + kfree(arg_p);
>> + if (ret < 0)
>> + pr_err("kstrtoul() failed for nl_reply_supported=\n");
>> + break;
>> default:
>> break;
>> }
>> @@ -1842,6 +1870,34 @@ static ssize_t tcmu_dev_size_store(struct
>> config_item *item, const char *page,
>> }
>> CONFIGFS_ATTR(tcmu_, dev_size);
>>
>> +static ssize_t tcmu_nl_reply_supported_show(struct config_item *item,
>> + char *page)
>> +{
>> + struct se_dev_attrib *da = container_of(to_config_group(item),
>> + struct se_dev_attrib, da_group);
>> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
>> +
>> + return snprintf(page, PAGE_SIZE, "%d\n", udev->nl_reply_supported);
>> +}
>> +
>> +static ssize_t tcmu_nl_reply_supported_store(struct config_item *item,
>> + const char *page, size_t count)
>> +{
>> + struct se_dev_attrib *da = container_of(to_config_group(item),
>> + struct se_dev_attrib, da_group);
>> + struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
>> + s8 val;
>> + int ret;
>> +
>> + ret = kstrtos8(page, 0, &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + udev->nl_reply_supported = val;
>> + return count;
>> +}
>> +CONFIGFS_ATTR(tcmu_, nl_reply_supported);
>> +
>> static ssize_t tcmu_emulate_write_cache_show(struct config_item *item,
>> char *page)
>> {
>> @@ -1884,6 +1940,7 @@ static struct configfs_attribute
>> *tcmu_attrib_attrs[] = {
>> &tcmu_attr_dev_config,
>> &tcmu_attr_dev_size,
>> &tcmu_attr_emulate_write_cache,
>> + &tcmu_attr_nl_reply_supported,
>> NULL,
>> };
>>
>
> The problem with this patch is that for tcmu-runner/targetcli-fb users
> the code will depend
> on the genl_info structure that contains whether or not a netlink reply
> is supported. You will
> need to fix that path where if you were to update the configfs to
> disable netlink reply and if a user
> already has stuff setup then things will get messy. Userspace will think
> netlink reply is supported,
> but kernel doesn't expect one.
>
I don't think this will be a problem, because the patch has backwards
compat support and the app that does not support nl replies will only
set the nl_reply_support = -1 for the devices it is managing.
If userspace were to mess up and set nl_reply_suppport = -1 for a device
it did not manage and so a reply is sent when the kernel did not expect
one, the kernel should just spit out an error due to not being able to
find a device or cmd mismatch, or we might do a complete() with no
waiters, or for device removal we might hit the uio crash again.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
` (2 preceding siblings ...)
2017-09-13 16:04 ` Mike Christie
@ 2017-09-13 18:35 ` Nakayama Kenjiro
2017-09-13 18:43 ` Mike Christie
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Nakayama Kenjiro @ 2017-09-13 18:35 UTC (permalink / raw)
To: target-devel
(re-sending this message as mail server returned error)
> I am ok with the idea for the patch, but what type of setup do you have
> where there are multiple applications using the interface and some
> support it and some do not? Is it because tcmu-runner is starting by
> default due to something like a distro systemd unit file and then you
> also have your app running too?
Though I am not going to run multiple applications using the
support/non-support interface atm, I have to note to use the non-support
app as "Please do not run tcmu-runner (or any app that enables
netlink reply support) before/during running this app. If you ran it, you
have to restart the host and never run it again". It sounds like a
little bit silly note...
> Also, if you do not implement sync netlink support, how does your daemon
> tell the kernel if the device failed to setup in the daemon? For
> deletion how did you work around the uio crashes and leaks?
Yes, I admit that the application should implement the netlink sync
support, and I know the issue when I am using the device without
netlink sync. However, I believe that it would be good if there is an
option to (force) disable the netlink sync for the workaround. It
means that not only for the non-supported app, but also for the
trouble shooting. (As I sent timeout patch to add timeout, it sometimes
causes an issue like hang due to the netlink sync failure.)
> > would be expected to support the netlink reply. To make matters worse,
> > users will not be able to add a device via configfs manually.
> >
>
> What is the specific issue with manually setting it up through configfs?
> rtslib/tagretcli use the same configfs operations and does not know what
> tcmu and daemons like tcmu-runner support.
I meant the following steps:
1. Run tcmu-runner (to enable netlink reply support).
// You can stop tcmu-runner process as it keeps the netlink support enabled //
# ./tcmu-runner
2. Enable the new device
// This will hang echo command forever //
mkdir -p /sys/kernel/config/target/core/user_20/test
echo "1" >> /sys/kernel/config/target/core/user_20/test/enable
My application is using same steps inside the code.
Regards,
Kenjiro
On Thu, Sep 14, 2017 at 12:58 AM, Mike Christie <mchristi@redhat.com> wrote:
> On 09/13/2017 12:01 AM, Kenjiro Nakayama wrote:
>> 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
>
> I am ok with the idea for the patch, but what type of setup do you have
> where there are multiple applications using the interface and some
> support it and some do not? Is it because tcmu-runner is starting by
> default due to something like a distro systemd unit file and then you
> also have your app running too?
>
> Also, if you do not implement sync netlink support, how does your daemon
> tell the kernel if the device failed to setup in the daemon? For
> deletion how did you work around the uio crashes and leaks?
>
>> would be expected to support the netlink reply. To make matters worse,
>> users will not be able to add a device via configfs manually.
>>
>
> What is the specific issue with manually setting it up through configfs?
> rtslib/tagretcli use the same configfs operations and does not know what
> tcmu and daemons like tcmu-runner support.
--
Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com>
GPG Key fingerprint = ED8F 049D E67A 727D 9A44 8E25 F44B E208 C946 5EB9
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
` (3 preceding siblings ...)
2017-09-13 18:35 ` Nakayama Kenjiro
@ 2017-09-13 18:43 ` Mike Christie
2017-09-13 19:27 ` Nakayama Kenjiro
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2017-09-13 18:43 UTC (permalink / raw)
To: target-devel
On 09/13/2017 01:28 PM, Nakayama Kenjiro wrote:
>> I am ok with the idea for the patch, but what type of setup do you have
>> where there are multiple applications using the interface and some
>> support it and some do not? Is it because tcmu-runner is starting by
>> default due to something like a distro systemd unit file and then you
>> also have your app running too?
>
> Though I am not going to run multiple applications using the
> support/non-support interface atm, I have to note to use the non-support
> app as "Please do not run tcmu-runner (or any app that enables
> netlink reply support) before/during running this app. If you ran it, you
> have to restart the host and never run it again". It sounds like a
> little bit silly note...
Yes. Agree.
>
>> Also, if you do not implement sync netlink support, how does your daemon
>> tell the kernel if the device failed to setup in the daemon? For
>> deletion how did you work around the uio crashes and leaks?
>
> Yes, I admit that the application should implement the netlink sync
> support,
Actually maybe I would not add support for it at all if I were you.
Some other questions/comments, because how the daemons like tcmu-runner
do setup might be a result of using targetcli/rtslib, and if you are not
going to use that then you can simplfy things.
The current normal way to create a device is:
1. Start app which listens on nl.
2. targetcli/rtslib run configfs commands to add and enable tcmu device.
3. tcmu sends nl event to app which sets up its objects for the device
and returns success/failure.
This approach works well for the targetcli/rtslib device management
flow, but has those extra steps.
If you are not going to use targetcli/rtslib then you do not need
netlink and to go through those extra steps.
It sounds like you want to do:
1. app runs configfs commands to add and enable tcmu device.
2. app knows tcmu device name and other info so it can just setup its
objects on successful writing to the enable file.
3. It sounds like you do not need the nl events at all at this point right.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
` (4 preceding siblings ...)
2017-09-13 18:43 ` Mike Christie
@ 2017-09-13 19:27 ` Nakayama Kenjiro
2017-09-13 19:31 ` Mike Christie
2017-09-16 1:01 ` Mike Christie
7 siblings, 0 replies; 9+ messages in thread
From: Nakayama Kenjiro @ 2017-09-13 19:27 UTC (permalink / raw)
To: target-devel
> If you are not going to use targetcli/rtslib then you do not need
> netlink and to go through those extra steps.
>
> It sounds like you want to do:
>
> 1. app runs configfs commands to add and enable tcmu device.
> 2. app knows tcmu device name and other info so it can just setup its
> objects on successful writing to the enable file.
> 3. It sounds like you do not need the nl events at all at this point
> right.
Yes, that's correct I don't need any nl events actually. But my point
is that when netlink reply support is enabled on the host by other app,
writing to the enable file ($TARGET/user_N/device/enable) hangs because
of waiting for the netlink sync in kernel. That's why I thought I
should implement it.
Currently there are no way to disable the netlink sync once enabled
it and even no way to check if it is enable/disabled on the host. You
don't think that it is necessary? At least, I would like to confirm
the netlink sync was disabled before running a non-support netlink app.
Regards,
Kenjiro
On Thu, Sep 14, 2017 at 3:43 AM, Mike Christie <mchristi@redhat.com> wrote:
> On 09/13/2017 01:28 PM, Nakayama Kenjiro wrote:
>>> I am ok with the idea for the patch, but what type of setup do you have
>>> where there are multiple applications using the interface and some
>>> support it and some do not? Is it because tcmu-runner is starting by
>>> default due to something like a distro systemd unit file and then you
>>> also have your app running too?
>>
>> Though I am not going to run multiple applications using the
>> support/non-support interface atm, I have to note to use the non-support
>> app as "Please do not run tcmu-runner (or any app that enables
>> netlink reply support) before/during running this app. If you ran it, you
>> have to restart the host and never run it again". It sounds like a
>> little bit silly note...
>
> Yes. Agree.
>
>>
>>> Also, if you do not implement sync netlink support, how does your daemon
>>> tell the kernel if the device failed to setup in the daemon? For
>>> deletion how did you work around the uio crashes and leaks?
>>
>> Yes, I admit that the application should implement the netlink sync
>> support,
>
> Actually maybe I would not add support for it at all if I were you.
>
> Some other questions/comments, because how the daemons like tcmu-runner
> do setup might be a result of using targetcli/rtslib, and if you are not
> going to use that then you can simplfy things.
>
> The current normal way to create a device is:
>
> 1. Start app which listens on nl.
> 2. targetcli/rtslib run configfs commands to add and enable tcmu device.
> 3. tcmu sends nl event to app which sets up its objects for the device
> and returns success/failure.
>
> This approach works well for the targetcli/rtslib device management
> flow, but has those extra steps.
>
> If you are not going to use targetcli/rtslib then you do not need
> netlink and to go through those extra steps.
>
> It sounds like you want to do:
>
> 1. app runs configfs commands to add and enable tcmu device.
> 2. app knows tcmu device name and other info so it can just setup its
> objects on successful writing to the enable file.
> 3. It sounds like you do not need the nl events at all at this point right.
--
Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com>
GPG Key fingerprint = ED8F 049D E67A 727D 9A44 8E25 F44B E208 C946 5EB9
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
` (5 preceding siblings ...)
2017-09-13 19:27 ` Nakayama Kenjiro
@ 2017-09-13 19:31 ` Mike Christie
2017-09-16 1:01 ` Mike Christie
7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2017-09-13 19:31 UTC (permalink / raw)
To: target-devel
On 09/13/2017 02:27 PM, Nakayama Kenjiro wrote:
> Currently there are no way to disable the netlink sync once enabled
> it and even no way to check if it is enable/disabled on the host. You
> don't think that it is necessary? At least, I would like to confirm
I think your mailer is messing things up and you missed where I said I
agree :) See:
>>> have to restart the host and never run it again". It sounds like a
>>> little bit silly note...
>>
>> Yes. Agree.
>>
I just want to make sure I understood your use case and what you really
need in case there is a better way to do this.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] target: Add netlink command reply supported option for each device
2017-09-13 5:01 [PATCH] target: Add netlink command reply supported option for each device Kenjiro Nakayama
` (6 preceding siblings ...)
2017-09-13 19:31 ` Mike Christie
@ 2017-09-16 1:01 ` Mike Christie
7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2017-09-16 1:01 UTC (permalink / raw)
To: target-devel
On 09/13/2017 12:01 AM, Kenjiro Nakayama wrote:
> 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.
>
Patch is ok with me.
Reviewed-by: Mike Christie <mchristi@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread