Re: [PATCHv2] tcmu: allow userspace to reset netlink

[Mike Christie <mchristi@redhat.com>]

On 04/12/2018 10:08 PM, Xiubo Li wrote:
> 
>>> +
>>> +    if (val != 1) {
>>> +        pr_err("Invalid block value %d\n", val);
>> I think you wanted
>>
>> "Invalid reset value %d\n"
> Yeah, just copied it from other place.
>>
>>> +        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.
> Will fix this.
> 
>>
>>> +    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);
>>          }
>>
> Sorry, this part I couldn't totally understand.
> 
> Do you mean take the code above you pasted by introducing
> target_undepend_item() for none removal paths just like in
> tcmu_genl_cmd_done() does?
> While in tcmu_genl_cmd_done(), the target_depend_item() is called by
> target_find_device() and then it should do target_undepend_item() after
> that, but in
> reset_netlink_store(), should it need target_undepend_item() ? If so
> they won't be in pairs.
> 

Yes, you are right. I thought we took a count when we sent the nl cmd.

> I think we should only take this part:
> 
>         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;
>         }
> 
> from the tcmu_genl_cmd_done(), right ?
> 

We want the complete and wake up part too right?

>>                  nl_cmd->complete = true;
>>                  wake_up(&udev->complete_wq);


> Thanks,
> BRs
> 
> 
>> 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,
>>>   };
>>>  
>