[4/4] tcmu: make cmd timeout configurable
diff mbox

Message ID 1490070061.8236.16.camel@haakon3.risingtidesystems.com
State New, archived
Headers show

Commit Message

Nicholas A. Bellinger March 21, 2017, 4:21 a.m. UTC
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.


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..?

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mike Christie March 21, 2017, 10:52 a.m. UTC | #1
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 }