Re: [PATCH 1/3] scsi_transport_iscsi: Add stats support for iscsi host

[Hannes Reinecke <hare-l3A5Bk7waGM@public.gmane.org>]

On 04/22/2016 08:37 PM, Mike Christie wrote:
> On 04/22/2016 08:39 AM, adheer.chandravanshi-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org wrote:
>> From: Adheer Chandravanshi <adheer.chandravanshi-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
>>
>> Add stats for iscsi initiator that will be maintained at iscsi host level
>> and will be exported as iscsi_host sysfs attributes.
>>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/scsi/scsi_transport_iscsi.c | 51 +++++++++++++++++++++++++++++++++++++
>>  include/scsi/iscsi_if.h             | 24 +++++++++++++++++
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>> index 42bca61..076843c 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -4253,6 +4253,21 @@ iscsi_host_attr(ipaddress, ISCSI_HOST_PARAM_IPADDRESS);
>>  iscsi_host_attr(initiatorname, ISCSI_HOST_PARAM_INITIATOR_NAME);
>>  iscsi_host_attr(port_state, ISCSI_HOST_PARAM_PORT_STATE);
>>  iscsi_host_attr(port_speed, ISCSI_HOST_PARAM_PORT_SPEED);
>> +iscsi_host_attr(login_accept_rsps, ISCSI_HOST_PARAM_LOGIN_ACCEPT_RSPS);
>> +iscsi_host_attr(login_other_fails, ISCSI_HOST_PARAM_LOGIN_OTHER_FAILS);
>> +iscsi_host_attr(login_authentication_fails,
>> +		ISCSI_HOST_PARAM_LOGIN_AUTHENTICATION_FAILS);
>> +iscsi_host_attr(login_authorization_fails,
>> +		ISCSI_HOST_PARAM_LOGIN_AUTHORIZATION_FAILS);
>> +iscsi_host_attr(login_negotiation_fails,
>> +		ISCSI_HOST_PARAM_LOGIN_NEGOTIATION_FAILS);
>> +iscsi_host_attr(login_redirect_rsps, ISCSI_HOST_PARAM_LOGIN_REDIRECT_RSPS);
>> +iscsi_host_attr(logout_normal_rsps, ISCSI_HOST_PARAM_LOGOUT_NORMAL_RSPS);
>> +iscsi_host_attr(logout_other_rsps, ISCSI_HOST_PARAM_LOGOUT_OTHER_RSPS);
>> +iscsi_host_attr(digest_err, ISCSI_HOST_PARAM_DIGEST_ERR);
>> +iscsi_host_attr(timeout_err, ISCSI_HOST_PARAM_TIMEOUT_ERR);
>> +iscsi_host_attr(format_err, ISCSI_HOST_PARAM_FORMAT_ERR);
>> +iscsi_host_attr(session_fails, ISCSI_HOST_PARAM_SESSION_FAILS);
>>  
>>  static struct attribute *iscsi_host_attrs[] = {
>>  	&dev_attr_host_netdev.attr,
>> @@ -4261,6 +4276,18 @@ static struct attribute *iscsi_host_attrs[] = {
>>  	&dev_attr_host_initiatorname.attr,
>>  	&dev_attr_host_port_state.attr,
>>  	&dev_attr_host_port_speed.attr,
>> +	&dev_attr_host_login_accept_rsps.attr,
>> +	&dev_attr_host_login_other_fails.attr,
>> +	&dev_attr_host_login_authentication_fails.attr,
>> +	&dev_attr_host_login_authorization_fails.attr,
>> +	&dev_attr_host_login_negotiation_fails.attr,
>> +	&dev_attr_host_login_redirect_rsps.attr,
>> +	&dev_attr_host_logout_normal_rsps.attr,
>> +	&dev_attr_host_logout_other_rsps.attr,
>> +	&dev_attr_host_digest_err.attr,
>> +	&dev_attr_host_timeout_err.attr,
>> +	&dev_attr_host_format_err.attr,
>> +	&dev_attr_host_session_fails.attr,
>>  	NULL,
>>  };
>>  
>> @@ -4284,6 +4311,30 @@ static umode_t iscsi_host_attr_is_visible(struct kobject *kobj,
>>  		param = ISCSI_HOST_PARAM_PORT_STATE;
>>  	else if (attr == &dev_attr_host_port_speed.attr)
>>  		param = ISCSI_HOST_PARAM_PORT_SPEED;
>> +	else if (attr == &dev_attr_host_login_accept_rsps.attr)
>> +		param = ISCSI_HOST_PARAM_LOGIN_ACCEPT_RSPS;
>> +	else if (attr == &dev_attr_host_login_other_fails.attr)
>> +		param = ISCSI_HOST_PARAM_LOGIN_OTHER_FAILS;
>> +	else if (attr == &dev_attr_host_login_authentication_fails.attr)
>> +		param = ISCSI_HOST_PARAM_LOGIN_AUTHENTICATION_FAILS;
>> +	else if (attr == &dev_attr_host_login_authorization_fails.attr)
>> +		param = ISCSI_HOST_PARAM_LOGIN_AUTHORIZATION_FAILS;
>> +	else if (attr == &dev_attr_host_login_negotiation_fails.attr)
>> +		param = ISCSI_HOST_PARAM_LOGIN_NEGOTIATION_FAILS;
>> +	else if (attr == &dev_attr_host_login_redirect_rsps.attr)
>> +		param = ISCSI_HOST_PARAM_LOGIN_REDIRECT_RSPS;
>> +	else if (attr == &dev_attr_host_logout_normal_rsps.attr)
>> +		param = ISCSI_HOST_PARAM_LOGOUT_NORMAL_RSPS;
>> +	else if (attr == &dev_attr_host_logout_other_rsps.attr)
>> +		param = ISCSI_HOST_PARAM_LOGOUT_OTHER_RSPS;
>> +	else if (attr == &dev_attr_host_digest_err.attr)
>> +		param = ISCSI_HOST_PARAM_DIGEST_ERR;
>> +	else if (attr == &dev_attr_host_timeout_err.attr)
>> +		param = ISCSI_HOST_PARAM_TIMEOUT_ERR;
>> +	else if (attr == &dev_attr_host_format_err.attr)
>> +		param = ISCSI_HOST_PARAM_FORMAT_ERR;
>> +	else if (attr == &dev_attr_host_session_fails.attr)
>> +		param = ISCSI_HOST_PARAM_SESSION_FAILS;
>>  	else {
>>  		WARN_ONCE(1, "Invalid host attr");
>>  		return 0;
>> diff --git a/include/scsi/iscsi_if.h b/include/scsi/iscsi_if.h
>> index d66c070..d6a3909 100644
>> --- a/include/scsi/iscsi_if.h
>> +++ b/include/scsi/iscsi_if.h
>> @@ -632,6 +632,18 @@ enum iscsi_host_param {
>>  	ISCSI_HOST_PARAM_IPADDRESS,
>>  	ISCSI_HOST_PARAM_PORT_STATE,
>>  	ISCSI_HOST_PARAM_PORT_SPEED,
>> +	ISCSI_HOST_PARAM_LOGIN_ACCEPT_RSPS,
>> +	ISCSI_HOST_PARAM_LOGIN_OTHER_FAILS,
>> +	ISCSI_HOST_PARAM_LOGIN_AUTHENTICATION_FAILS,
>> +	ISCSI_HOST_PARAM_LOGIN_AUTHORIZATION_FAILS,
>> +	ISCSI_HOST_PARAM_LOGIN_NEGOTIATION_FAILS,
>> +	ISCSI_HOST_PARAM_LOGIN_REDIRECT_RSPS,
>> +	ISCSI_HOST_PARAM_LOGOUT_NORMAL_RSPS,
>> +	ISCSI_HOST_PARAM_LOGOUT_OTHER_RSPS,
>> +	ISCSI_HOST_PARAM_DIGEST_ERR,
>> +	ISCSI_HOST_PARAM_TIMEOUT_ERR,
>> +	ISCSI_HOST_PARAM_FORMAT_ERR,
>> +	ISCSI_HOST_PARAM_SESSION_FAILS,
>>  	ISCSI_HOST_PARAM_MAX,
>>  };
>>  
>> @@ -819,6 +831,18 @@ struct iscsi_stats {
>>  	/* errors */
>>  	uint32_t digest_err;
>>  	uint32_t timeout_err;
>> +	uint32_t format_err;
>> +	uint32_t session_fails;
>> +
>> +	/* login/logout stats */
>> +	uint32_t login_accept_rsps;
>> +	uint32_t login_other_fails;
>> +	uint32_t login_authentication_fails;
>> +	uint32_t login_authorization_fails;
>> +	uint32_t login_negotiation_fails;
>> +	uint32_t login_redirect_rsps;
>> +	uint32_t logout_normal_rsps;
>> +	uint32_t logout_other_rsps;
>>  
> 
> I do not think we can add new fields here because this is passed between
> the kernel and userspace.
> 
> Also, lets either go with the get stats nl type of approach or say we
> are depreciating that and going with sysfs now and the future. Not a mix
> based on who is submitting patches.
> 
> I can see why the get stats approach is nasty, because it is difficult
> to extend. If we want to go sysfs based approach then we should probably
> make it more like the other transports where there is a stats dir and a
> reset admin file.
> 
> Lee and Chris should probably decide how to proceed.
> 
Hmm.

For these kind of things I wonder whether debugfs isn't the correct way
of doing. Thing is, sysfs is complicated enough already, and is
considered a _stable_ kernel API.
So everytime you add fields there you have to fight it out with
upstream. It becomes especially bad when you have to _change_ fields, or
even remove them.

>From my POV statistics are unstable, as not every device/implementation
might be able to deliver the same set. Plus I really would like to
reserve the right to change them as needed; this patchset is the best
example for it.

Hence I would not be using sysfs for it, but rather debugfs or something
like that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare-l3A5Bk7waGM@public.gmane.org			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.