On 04/04/2018 11:39 PM, xiubli@xxxxxxxxxx wrote:
> From: Xiubo Li <xiubli@xxxxxxxxxx>
>
> This patch adds 1 tcmu attr to reset and complete all the blocked
> netlink waiting threads. It's used when the userspace daemon like
> tcmu-runner has crashed or forced to shutdown just before the
> netlink requests be replied to the kernel, then the netlink requeting
> threads will get stuck forever. We must reboot the machine to recover
> from it and by this the rebootng is not a must then.
>
> The Call Trace will be something like:
> ==============
> INFO: task targetctl:22655 blocked for more than 120 seconds.
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> targetctl D ffff880169718fd0 0 22655 17249 0x00000080
> Call Trace:
> [<ffffffff816ab6d9>] schedule+0x29/0x70
> [<ffffffff816a90e9>] schedule_timeout+0x239/0x2c0
> [<ffffffff81574d42>] ? skb_release_data+0xf2/0x140
> [<ffffffff816aba8d>] wait_for_completion+0xfd/0x140
> [<ffffffff810c6440>] ? wake_up_state+0x20/0x20
> [<ffffffffc0159f5a>] tcmu_netlink_event+0x26a/0x3a0 [target_core_user]
> [<ffffffff810b34b0>] ? wake_up_atomic_t+0x30/0x30
> [<ffffffffc015a2c6>] tcmu_configure_device+0x236/0x350 [target_core_user]
> [<ffffffffc05085df>] target_configure_device+0x3f/0x3b0 [target_core_mod]
> [<ffffffffc0502e7c>] target_core_store_dev_enable+0x2c/0x60 [target_core_mod]
> [<ffffffffc0501244>] target_core_dev_store+0x24/0x40 [target_core_mod]
> [<ffffffff8128a0e4>] configfs_write_file+0xc4/0x130
> [<ffffffff81202aed>] vfs_write+0xbd/0x1e0
> [<ffffffff812038ff>] SyS_write+0x7f/0xe0
> [<ffffffff816b89fd>] system_call_fastpath+0x16/0x1b
> ==============
>
> Be careful of using this, it could reset the normal netlink requesting
> operations, so we should use this only when the user space daemon from
> starting and just before the daemon could receive and handle the nl
> requests.
>
> Changes since v1(suggested by Mike Christie):
> v2: - Makes the reset per device.
>
> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> ---
> drivers/target/target_core_user.c | 52 +++++++++++++++++++++++++++++++++------
> 1 file changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 4ad89ea..7271da8 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -105,7 +105,8 @@ struct tcmu_hba {
>
> struct tcmu_nl_cmd {
> /* wake up thread waiting for reply */
> - struct completion complete;
> + bool complete;
> +
> int cmd;
> int status;
> };
> @@ -159,9 +160,12 @@ struct tcmu_dev {
>
> spinlock_t nl_cmd_lock;
> struct tcmu_nl_cmd curr_nl_cmd;
> - /* wake up threads waiting on curr_nl_cmd */
> + /* wake up threads waiting on nl_cmd_wq */
> wait_queue_head_t nl_cmd_wq;
>
> + /* complete thread waiting complete_wq */
> + wait_queue_head_t complete_wq;
> +
> char dev_config[TCMU_CONFIG_LEN];
>
> int nl_reply_supported;
> @@ -307,11 +311,13 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int completed_cmd)
> nl_cmd->status = rc;
> }
>
> - spin_unlock(&udev->nl_cmd_lock);
> if (!is_removed)
> target_undepend_item(&dev->dev_group.cg_item);
> - if (!ret)
> - complete(&nl_cmd->complete);
> + if (!ret) {
> + nl_cmd->complete = true;
> + wake_up(&udev->complete_wq);
> + }
> + spin_unlock(&udev->nl_cmd_lock);
> return ret;
> }
>
> @@ -1258,6 +1264,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name)
> timer_setup(&udev->cmd_timer, tcmu_cmd_timedout, 0);
>
> init_waitqueue_head(&udev->nl_cmd_wq);
> + init_waitqueue_head(&udev->complete_wq);
> spin_lock_init(&udev->nl_cmd_lock);
>
> INIT_RADIX_TREE(&udev->data_blocks, GFP_KERNEL);
> @@ -1555,7 +1562,6 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd)
>
> memset(nl_cmd, 0, sizeof(*nl_cmd));
> nl_cmd->cmd = cmd;
> - init_completion(&nl_cmd->complete);
>
> spin_unlock(&udev->nl_cmd_lock);
> }
> @@ -1573,9 +1579,10 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev)
> return 0;
>
> pr_debug("sleeping for nl reply\n");
> - wait_for_completion(&nl_cmd->complete);
> + wait_event(udev->complete_wq, nl_cmd->complete);
>
> spin_lock(&udev->nl_cmd_lock);
> + nl_cmd->complete = false;
> nl_cmd->cmd = TCMU_CMD_UNSPEC;
> ret = nl_cmd->status;
> nl_cmd->status = 0;
> @@ -2323,6 +2330,36 @@ static ssize_t tcmu_block_dev_store(struct config_item *item, const char *page,
> }
> CONFIGFS_ATTR(tcmu_, block_dev);
>
> +static ssize_t tcmu_reset_netlink_store(struct config_item *item, const char *page,
> + size_t count)
> +{
> + struct se_device *se_dev = container_of(to_config_group(item),
> + struct se_device,
> + dev_action_group);
> + struct tcmu_dev *udev = TCMU_DEV(se_dev);
> + struct tcmu_nl_cmd *nl_cmd = &udev->curr_nl_cmd;
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8(page, 0, &val);
> + if (ret < 0)
> + return ret;
> +
> + if (val != 1) {
> + pr_err("Invalid block value %d\n", val);
I think you wanted
"Invalid reset value %d\n"
> + return -EINVAL;
> + }
> +
> + spin_unlock(&udev->nl_cmd_lock);
Need spin_lock() instead of unlock.
I think before calling the code below you need to check if a nl command
is even waiting. If you just run this with no nl commands waiting then
next time we send a command nl_cmd->complete will be true and
tcmu_wait_genl_cmd_reply's wait_event call will return right away.
> + nl_cmd->complete = true;
> + nl_cmd->status = -EINTR;
> + wake_up(&udev->complete_wq);
Instead of the code above, I think you need to take some of the guts out
of tcmu_genl_cmd_done to handle removal which is an odd cases due to the
refcoutning/locking and configfs use.
For non removal commands we need to do a target_undepend_item. For
remove, we don't want to since we came through configfs and teardown
already started.
So instead of the above lines take:
if (nl_cmd->cmd != completed_cmd) {
printk(KERN_ERR "Mismatched commands (Expecting reply
for %d. Current %d).\n",
completed_cmd, nl_cmd->cmd);
ret = -EINVAL;
} else {
nl_cmd->status = rc;
}
// Note: I changed this from the is_removed
if (completed_cmd != TCMU_CMD_REMOVED_DEVICE)
target_undepend_item(&dev->dev_group.cg_item);
if (!ret) {
nl_cmd->complete = true;
wake_up(&udev->complete_wq);
}
from tcmu_genl_cmd_done and make a function that takes the status code
and completed_cmd. You would just then do
__tcmu_genl_cmd_done(&udev->curr_nl_cmd.cmd, -EINTR);
> + spin_unlock(&udev->nl_cmd_lock);
> +
> + return count;
> +}
> +CONFIGFS_ATTR_WO(tcmu_, reset_netlink);
> +
> static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page,
> size_t count)
> {
> @@ -2363,6 +2400,7 @@ static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page,
> static struct configfs_attribute *tcmu_action_attrs[] = {
> &tcmu_attr_block_dev,
> &tcmu_attr_reset_ring,
> + &tcmu_attr_reset_netlink,
> NULL,
> };
>
>
