On 03/21/2017 12:21 AM, Nicholas A. Bellinger wrote:
> On Mon, 2017-03-20 at 00:05 -0500, Mike Christie wrote:
>> On 03/18/2017 06:26 PM, Nicholas A. Bellinger wrote:
>>> +static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *page,
>>> + size_t count)
>>> +{
>>
>>
>> ...
>>
>>
>>> +
>>> + if (!val) {
>>> + pr_err("Illegal value for cmd_time_out\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>
>> Thanks for the patch! I tested with this chunk removed so you can
>> disable the timer (0 == disabled), and it all works as expected.
>>
>
> Thanks. Here's the last patch to drop that part, and avoid the possible
> divide-by-zero in the configfs attribute show handler.
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index c6874c3..1d12081 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1174,7 +1174,8 @@ static ssize_t tcmu_cmd_time_out_show(struct config_item *item, char *page)
> struct tcmu_dev *udev = container_of(da->da_dev,
> struct tcmu_dev, se_dev);
>
> - return snprintf(page, PAGE_SIZE, "%lu\n", udev->cmd_time_out / MSEC_PER_SEC);
> + return snprintf(page, PAGE_SIZE, "%lu\n", (!udev->cmd_time_out) ? 0:
> + udev->cmd_time_out / MSEC_PER_SEC);
> }
>
> static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *page,
> @@ -1196,11 +1197,6 @@ static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *pag
> if (ret < 0)
> return ret;
>
> - if (!val) {
> - pr_err("Illegal value for cmd_time_out\n");
> - return -EINVAL;
> - }
> -
> udev->cmd_time_out = val * MSEC_PER_SEC;
> return count;
> }
>
Works ok for me. Thanks.
> Btw, one other thing..
>
> The default TCMU_TIME_OUT = 30 seconds value is still used by
> tcmu_queue_cmd_ring() when !is_ring_space_avail(), even when the new
> cmd_time_out attribute has been set to zero.
>
> AFAICT it should be using a different value than cmd_time_out, and 30
> seconds default as-is seems way too high for non Linux initiators..
>
> Perhaps this is a good canidate to become it's own TCMU backend
> attribute separate from cmd_time_out..?
Yeah, it should be a new timeout eventually. I left it there because it
currently does not hit issues like the other case, and I was not sure if
when the userspace based timer is implemented that it will use that
value or another new value that monitors this specific problem where we
cannot get access to userspace.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
