On 01/11/2013 05:50 AM, [email protected] wrote:
> From: Adheer Chandravanshi <[email protected]>
> 
> This patch allows iscsiadm to manage iSCSI target information stored on
> adapter flash on per host basis.
> 
> The sysfs entries will look as cited below:
>     /sys/class/iscsi_flash_tgt/tgt-<host_no>-<target_no>/
> 
> Signed-off-by: Adheer Chandravanshi <[email protected]>
> Signed-off-by: Vikas Chaudhary <[email protected]>
> ---
>  drivers/scsi/scsi_transport_iscsi.c |  690 
> ++++++++++++++++++++++++++++++++++-
>  include/scsi/iscsi_if.h             |  106 ++++++
>  include/scsi/scsi_transport_iscsi.h |   35 ++
>  3 files changed, 830 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index dac7f8d..028f2c3 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -25,6 +25,7 @@
>  #include <linux/slab.h>
>  #include <linux/bsg-lib.h>
>  #include <linux/idr.h>
> +#include <linux/list.h>
>  #include <net/tcp.h>
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_host.h>
> @@ -460,6 +461,431 @@ void iscsi_destroy_iface(struct iscsi_iface *iface)
>  EXPORT_SYMBOL_GPL(iscsi_destroy_iface);
>  
>  /*
> + * Interface to display flash tgt params to sysfs
> + */
> +
> +static void iscsi_flash_tgt_release(struct device *dev)
> +{
> +     struct iscsi_flash_tgt *tgt = iscsi_dev_to_flash_tgt(dev);
> +     struct device *parent = tgt->dev.parent;
> +
> +     kfree(tgt);
> +     put_device(parent);
> +}
> +
> +
> +static struct class iscsi_flash_tgt_class = {
> +     .name = "iscsi_flash_tgt",
> +     .dev_release = iscsi_flash_tgt_release,
> +};
> +


I guess we are supposed to be using buses now. See the recent fcoe
discussions and the code that got merged.


> +#define ISCSI_FLASHTGT_ATTR(_prefix, _name, _mode, _show, _store)    \
> +struct device_attribute dev_attr_##_prefix##_##_name =                       
> \
> +     __ATTR(_name, _mode, _show, _store)
> +
> +/* flash tgt attrs show */
> +#define iscsi_flash_tgt_attr_show(type, name, param)                 \
> +static ssize_t                                                               
> \
> +show_##type##_##name(struct device *dev, struct device_attribute *attr,      
> \
> +                  char *buf)                                         \
> +{                                                                    \
> +     struct iscsi_flash_tgt *tgt = iscsi_dev_to_flash_tgt(dev);      \
> +     struct iscsi_transport *t = tgt->transport;                     \
> +     return t->get_flash_tgt_param(tgt, param, buf);                 \
> +}                                                                    \
> +
> +
> +#define iscsi_flash_tgt_attr(type, name, param)                              
> \
> +     iscsi_flash_tgt_attr_show(type, name, param)                    \
> +static ISCSI_FLASHTGT_ATTR(type, name, S_IRUGO,                              
> \
> +                         show_##type##_##name, NULL);
> +
> +/* Target attributes */
> +
> +iscsi_flash_tgt_attr(tgt, is_fw_assigned_ipv6,
> +                  ISCSI_FLASHTGT_IS_FW_ASSIGNED_IPV6);


Could you give me more info on this one? I saw on the follow on pathc it
mentions link local ipv6 addrs. Can the card have multiple link local
ipv6 addrs some are assigned my fw and some are assigned by the driver?


> +iscsi_flash_tgt_attr(tgt, dev_type, ISCSI_FLASHTGT_DEV_TYPE);


Could you rename to portal addr type or something like that so it is
clear it is for the portal address.


> +iscsi_flash_tgt_attr(tgt, dif, ISCSI_FLASHTGT_DIF_EN);
> +iscsi_flash_tgt_attr(tgt, auto_snd_tgt_disable,
> +                  ISCSI_FLASHTGT_AUTO_SND_TGT_DISABLE);
> +iscsi_flash_tgt_attr(tgt, discovery_session, ISCSI_FLASHTGT_DISCOVERY_SESS);

This indicates if it is discovery session vs normal session right? Could
we rename to session_type?


> +iscsi_flash_tgt_attr(tgt, entry_enable, ISCSI_FLASHTGT_ENTRY_EN);
> +iscsi_flash_tgt_attr(tgt, dev_assoc_target, ISCSI_FLASHTGT_DEV_ASSOC_TARGET);
> +iscsi_flash_tgt_attr(tgt, dev_assoc_initiator,
> +                  ISCSI_FLASHTGT_DEV_ASSOC_INITIATOR);

I did not get this. In the iscsi spec can we discover initiators like
how with FC we can see initiator and target ports? Is this for iSNS?
Does it work such that iSNS in fw, will discovery initaitor and target
nodes and they can be exported in this flash tgt mgmt interface?

Can we rename to node_type?

Also then, do we want to rename all of this to flash_node/FLASHNODE
instead of flash_tgt?

> +iscsi_flash_tgt_attr(tgt, exec_throttle, ISCSI_FLASHTGT_EXEC_THROTTLE);

Is this scsi commands or all types of iscsi pdus? So scsi commands with
say something like nops?

If it is the limit of all iscsi and scsi commands combined, can we
rename to cmds_max so it matches the libiscsi/iscsid/iscsiadm name?

If it is the limit of just the scsi commands, can we rename to can_queue
so it matches the scsi layer setting name.


> +iscsi_flash_tgt_attr(tgt, exec_count, ISCSI_FLASHTGT_EXEC_COUNT);

I am not sure I understand this one. Is it the number of commands that
are currently running when you read the sysfs file?

I see we write the value to the fw, but I do not see we ever update it
in qla4xxx.

> +iscsi_flash_tgt_attr(tgt, retry_count, ISCSI_FLASHTGT_RETRY_COUNT);

Is this for scsi command retry type of things or for session relogin
type of events or both or neither? Same for delay.

> +iscsi_flash_tgt_attr(tgt, retry_delay, ISCSI_FLASHTGT_RETRY_DELAY);
> +iscsi_flash_tgt_attr(tgt, header_digest, ISCSI_FLASHTGT_HDR_DGST_EN);
> +iscsi_flash_tgt_attr(tgt, data_digest, ISCSI_FLASHTGT_DATA_DGST_EN);
> +iscsi_flash_tgt_attr(tgt, immediate_data, ISCSI_FLASHTGT_IMM_DATA_EN);
> +iscsi_flash_tgt_attr(tgt, initial_r2t, ISCSI_FLASHTGT_INITIAL_R2T_EN);
> +iscsi_flash_tgt_attr(tgt, data_seq_in_order, ISCSI_FLASHTGT_DATASEQ_INORDER);
> +iscsi_flash_tgt_attr(tgt, data_pdu_in_order, ISCSI_FLASHTGT_PDU_INORDER);
> +iscsi_flash_tgt_attr(tgt, chap_auth, ISCSI_FLASHTGT_CHAP_AUTH_EN);
> +iscsi_flash_tgt_attr(tgt, snack_req, ISCSI_FLASHTGT_SNACK_REQ_EN);
> +iscsi_flash_tgt_attr(tgt, discovery_logout,
> +                  ISCSI_FLASHTGT_DISCOVERY_LOGOUT_EN);

If this is not set what happens? Do you guys keep the session logged in?
Is this like the cisco long lived discovery session feature in the
linux-iscsi driver?

Why do you keep it open or why do you want to?

It's fine. I am just wondering.


> +iscsi_flash_tgt_attr(tgt, bidi_chap, ISCSI_FLASHTGT_BIDI_CHAP_EN);
> +iscsi_flash_tgt_attr(tgt, discovery_auth_optional,
> +                  ISCSI_FLASHTGT_DISCOVERY_AUTH_OPTIONAL);
> +iscsi_flash_tgt_attr(tgt, erl, ISCSI_FLASHTGT_ERL);
> +iscsi_flash_tgt_attr(tgt, tcp_timestamp_stat,
> +                  ISCSI_FLASHTGT_TCP_TIMESTAMP_STAT);
> +iscsi_flash_tgt_attr(tgt, tcp_nagle_disable, 
> ISCSI_FLASHTGT_TCP_NAGLE_DISABLE);
> +iscsi_flash_tgt_attr(tgt, tcp_wsf_disable, ISCSI_FLASHTGT_TCP_WSF_DISABLE);
> +iscsi_flash_tgt_attr(tgt, tcp_timer_scale, ISCSI_FLASHTGT_TCP_TIMER_SCALE);
> +iscsi_flash_tgt_attr(tgt, tcp_timestamp_enable,
> +                  ISCSI_FLASHTGT_TCP_TIMESTAMP_EN);
> +iscsi_flash_tgt_attr(tgt, ip_frag_disable, ISCSI_FLASHTGT_IP_FRAG_DISABLE);
> +iscsi_flash_tgt_attr(tgt, max_recv_dlength, ISCSI_FLASHTGT_MAX_RECV_DLENGTH);
> +iscsi_flash_tgt_attr(tgt, max_xmit_dlength, ISCSI_FLASHTGT_MAX_XMIT_DLENGTH);
> +iscsi_flash_tgt_attr(tgt, first_burst, ISCSI_FLASHTGT_FIRST_BURST);
> +iscsi_flash_tgt_attr(tgt, default_time2wait, ISCSI_FLASHTGT_DEF_TIME2WAIT);
> +iscsi_flash_tgt_attr(tgt, default_time2retain, 
> ISCSI_FLASHTGT_DEF_TIME2RETAIN);
> +iscsi_flash_tgt_attr(tgt, max_r2t, ISCSI_FLASHTGT_MAX_R2T);
> +iscsi_flash_tgt_attr(tgt, keepalive_timeout, ISCSI_FLASHTGT_KEEPALIVE_TMO);
> +iscsi_flash_tgt_attr(tgt, isid, ISCSI_FLASHTGT_ISID);

What format does this get printed out as?

> +iscsi_flash_tgt_attr(tgt, tsid, ISCSI_FLASHTGT_TSID);
> +iscsi_flash_tgt_attr(tgt, port, ISCSI_FLASHTGT_PORT);
> +iscsi_flash_tgt_attr(tgt, max_burst, ISCSI_FLASHTGT_MAX_BURST);
> +iscsi_flash_tgt_attr(tgt, def_tmf_timeout, ISCSI_FLASHTGT_DEF_TMF_TMO);
> +iscsi_flash_tgt_attr(tgt, ipaddress, ISCSI_FLASHTGT_IPADDR);
> +iscsi_flash_tgt_attr(tgt, targetalias, ISCSI_FLASHTGT_ALIAS);
> +iscsi_flash_tgt_attr(tgt, redirect_ipaddr, ISCSI_FLASHTGT_REDIRECT_IPADDR);

What is this? Is it the address we are redirected to or the address we
initially log in to? For the latter, isn't it then the same as ipaddress
above?

In the other iscsi code the persistent_address/port is the one we
initially log into. The address/port is then the one we are currently
logged into.


> +iscsi_flash_tgt_attr(tgt, max_segment_size, ISCSI_FLASHTGT_MAX_SEGMENT_SIZE);
> +iscsi_flash_tgt_attr(tgt, local_port, ISCSI_FLASHTGT_LOCAL_PORT);

So why is the port printed but not the local ipaddress? It is due to how
your card supports addresses right?

I think we want a link to the iface. Is that not possible?



> +iscsi_flash_tgt_attr(tgt, ipv4_tos, ISCSI_FLASHTGT_IPV4_TOS);
> +iscsi_flash_tgt_attr(tgt, ipv6_flow_label, ISCSI_FLASHTGT_IPV6_FLOW_LABEL);
> +iscsi_flash_tgt_attr(tgt, name, ISCSI_FLASHTGT_NAME);
> +iscsi_flash_tgt_attr(tgt, tpgt, ISCSI_FLASHTGT_TPGT);
> +iscsi_flash_tgt_attr(tgt, link_local_ipv6, ISCSI_FLASHTGT_LINK_LOCAL_IPV6);

Is this the link local address we are connected through on the initiator
side?



> +iscsi_flash_tgt_attr(tgt, discovery_parent, ISCSI_FLASHTGT_DISCOVERY_PARENT);

Could you make this print out a string like "send_targets" and 'isns"?



>  
>  /**
>   * struct iscsi_transport - iSCSI Transport template
> @@ -150,6 +151,16 @@ struct iscsi_transport {
>       int (*get_chap) (struct Scsi_Host *shost, uint16_t chap_tbl_idx,
>                        uint32_t *num_entries, char *buf);
>       int (*delete_chap) (struct Scsi_Host *shost, uint16_t chap_tbl_idx);
> +     int (*get_flash_tgt_param) (struct iscsi_flash_tgt *tgt, int param,
> +                                 char *buf);
> +     int (*set_flash_tgt_param) (struct iscsi_flash_tgt *tgt, int param,
> +                                 const char *buf, int len);

Make this work like the iface set params where multiple params are set
in one call.


> +     struct iscsi_flash_tgt *(*new_flash_tgt) (struct Scsi_Host *shost,
> +                                               const char *buf, int len);
> +     void (*del_flash_tgt) (struct iscsi_flash_tgt *tgt);
> +     int (*apply_flash_tgt) (struct iscsi_flash_tgt *tgt);

Kill this.

> +     int (*login_flash_tgt) (struct iscsi_flash_tgt *tgt);
> +     int (*logout_flash_tgt) (struct iscsi_cls_session *cls_sess);
>  };


My only other comment is why are the portal/connection settings mixed in
with the node/session? I think it migt be cleaner to seperate them. Can
we just put the portal/conn attrs in another attribute group? It will
match how we read other sysfs conn/session params then too.


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to 
[email protected].
Visit this group at http://groups.google.com/group/open-iscsi?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to