On 03/25/2016 04:08 AM, Adheer Chandravanshi wrote:
> 
> 
> From: "open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>"
> <open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>> on
> behalf of Adheer Chandravanshi <adheer.chandravan...@qlogic.com
> <mailto:adheer.chandravan...@qlogic.com>>
> Reply-To: "open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>" <open-iscsi@googlegroups.com
> <mailto:open-iscsi@googlegroups.com>>
> Date: Thursday, 10 March 2016 9:54 am
> To: Mike Christie <micha...@cs.wisc.edu <mailto:micha...@cs.wisc.edu>>,
> "open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>"
> <open-iscsi@googlegroups.com <mailto:open-iscsi@googlegroups.com>>
> Cc: Shyam Sundar <shyam.sun...@qlogic.com
> <mailto:shyam.sun...@qlogic.com>>, Saurav Kashyap
> <saurav.kash...@qlogic.com <mailto:saurav.kash...@qlogic.com>>
> Subject: Re: [PATCH] iscsi tools: Add support for some host statistics
> 
> 
> 
>     From: Mike Christie <micha...@cs.wisc.edu <mailto:micha...@cs.wisc.edu>>
>     Date: Friday, 4 March 2016 6:11 am
>     To: "open-iscsi@googlegroups.com
>     <mailto:open-iscsi@googlegroups.com>" <open-iscsi@googlegroups.com
>     <mailto:open-iscsi@googlegroups.com>>
>     Cc: Shyam Sundar <shyam.sun...@qlogic.com
>     <mailto:shyam.sun...@qlogic.com>>, Saurav Kashyap
>     <saurav.kash...@qlogic.com <mailto:saurav.kash...@qlogic.com>>,
>     Adheer Chandravanshi <adheer.chandravan...@qlogic.com
>     <mailto:adheer.chandravan...@qlogic.com>>
>     Subject: Re: [PATCH] iscsi tools: Add support for some host statistics
> 
>         On 03/02/2016 02:04 AM, adheer.chandravan...@qlogic.com
>         <mailto:adheer.chandravan...@qlogic.com> wrote:
> 
>             From: Adheer Chandravanshi <adheer.chandravan...@qlogic.com
>             <mailto:adheer.chandravan...@qlogic.com>>
>             Add support to maintain and show some host statistics in iscsid.
>             This provides following host specific stats:
>             iscsi_login_accept_rsps
>             iscsi_login_other_fail_rsps
>             iscsi_login_negotiate_fails
>             iscsi_login_authenticate_fails
>             iscsi_login_auth_fail_rsps
>             iscsi_login_redirect_rsps
>             iscsi_logout_normals
>             iscsi_logout_others
>             iscsi_connection_timeout_errors
>             iscsi_session_failures
> 
> 
>         What happened? I actually liked your guys's originally attempt
>         where it
>         was kernel based better.
> 
> 
>     We think that putting the host statistics in userspace looks cleaner
>     and is also inline with current design of partial offload solutions
>     (bnx2i and likes) and software initiator.
>     The stats to be supported currently can be easily managed with this
>     patch as they are updated with the current PDU parsing flow in
>     userspace.
> 
> 
>         How are you going to support qla4xxx session with this approach?
>         Were
>         you going to do this and also a kernel interface for that type
>         of driver?
> 
> 
>     For qla4xxx, it already has support for host stats through current
>     offload host stats interface.
>     And in case its needed, we can support extra stats using
>     iscsi_host_stats_custom field of struct iscsi_offload_host_stats.
> 
> 
>         For this userspace based patch, what was the last issue with it?
>         Were
>         you guys having some issue about when to add the host or delete
>         it or
>         something and did you solve the issue?
> 
> 
>     Yes, it was related to host deletion and dropping the reference.
>     It has been solved by listening to udev events for iscsi_host removal.
> 
>     As correctly pointed out by Chris, following points should have been
>     added to description.
>     1) adds a refcounted iscsi_host structure
>     2) adds a uevent listener to iscsid
> 
> 
> 
> Hello,
> 
> Any other feedback for this patch?
> 

Hey,

I think the patch is ugly. I know this is partially my fault because I
worked on it :) It is just a lot of code to add login stats. I can live
with it though. If however, we end up needing stats like these for
qla4xxx type drivers I will block any patches that try to do it using
custom stats. We will need to do it generically in kernel. In userspace
we have flexibility.

Could you break up the patch like Chris requested. I have not done a
line by line check of the patch.


-- 
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+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Reply via email to