attached integrate-iscsi-netlink.patch - incorporate the
open-iscsi/linux-iscsi netlink interface into the iscsi transport class.
This patch also removes the iscsi_host class and breaks the
iscsi_transport class into a iscsi_connection and iscsi_session class.
The iscsi_host class is removed becuase the open-iscsi/linux-iscsi
netlink framework pushes login to usersapce so there is no need to store
long strings like the initiatorname and alias in the kernel anymore. And
breaking out the session and connection from the old
/sys/class/iscsi_transport was necessary becuase the session and
connection creation and destruction is driven from userspace and it may
end up where you have a session but no scsi target (in which case
/sys/class/iscsi_transport would not show up previously). Having the
session/connection represented in this case is useful for iscsi async
events and their userpace handlers.
We followed the FC transport class strategy for the sysfs layout which
uses struct devices and driver model transport_classes so the format
looks like the following:
[root@mina class]# cd iscsi_session/
[root@mina iscsi_session]# ls
session2
[root@mina iscsi_session]# cd session2/
[root@mina session2]# ls
data_pdu_in_order device first_burst_len initial_r2t
max_outstanding_r2t
data_seq_in_order erl immediate_data max_burst_len
[root@mina class]# cd iscsi_connection/
[root@mina iscsi_connection]# ls
connection2:0
[root@mina iscsi_connection]# cd connection2\:0/
[root@mina connection2:0]# ls
device header_digest ifmarker max_recv_dlength max_xmit_dlength
ofmarker
A session's connection is linked by the device links:
[root@mina host2]# tree
.
|-- detach_state
|-- power
| `-- state
`-- session2
|-- connection2:0
| |-- detach_state
| `-- power
| `-- state
|-- detach_state
`-- power
`-- state
We are perfectly fine with adding a different parent of the session or
making the class_devices linked rather than the devices or ditching the
device usage for kobjects or something else. We were not sure if the FC
transport class model was the preferred model for us to follow though.
Thanks,
Linux-iscsi Team
Signed-off-by: Alex Aizman <[email protected]>
Signed-off-by: Dmitry Yusupov <[email protected]>
Signed-off-by: Mike Christie <[email protected]>
* Mike Christie ([email protected]) wrote:
> +static int
> +iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> +{
> + int err = 0;
> + struct iscsi_uevent *ev = NLMSG_DATA(nlh);
> + struct iscsi_transport *transport = iscsi_ptr(ev->transport_handle);
> +
> + if (nlh->nlmsg_type != ISCSI_UEVENT_TRANS_LIST &&
> + iscsi_if_transport_lookup(transport) < 0)
> + return -EEXIST;
> +
> + daemon_pid = NETLINK_CREDS(skb)->pid;
> +
Are any of these message types privileged operations? I didn't notice
any real credential check.
> + switch (nlh->nlmsg_type) {
> + case ISCSI_UEVENT_TRANS_LIST: {
> + int i;
> +
> + for (i = 0; i < ISCSI_TRANSPORT_MAX; i++) {
> + if (transport_table[i]) {
> + ev->r.t_list.elements[i].trans_handle =
> + iscsi_handle(transport_table[i]);
> + strncpy(ev->r.t_list.elements[i].name,
> + transport_table[i]->name,
> + ISCSI_TRANSPORT_NAME_MAXLEN);
> + } else
> + ev->r.t_list.elements[i].trans_handle =
> + iscsi_handle(NULL);
> + }
> + } break;
> + case ISCSI_UEVENT_CREATE_SESSION:
> + err = iscsi_if_create_snx(transport, ev);
> + break;
> + case ISCSI_UEVENT_DESTROY_SESSION:
> + err = iscsi_if_destroy_snx(transport, ev);
> + break;
> + case ISCSI_UEVENT_CREATE_CNX:
> + err = iscsi_if_create_cnx(transport, ev);
> + break;
> + case ISCSI_UEVENT_DESTROY_CNX:
> + err = iscsi_if_destroy_cnx(transport, ev);
> + break;
> + case ISCSI_UEVENT_BIND_CNX:
> + if (!iscsi_if_find_cnx(ev->u.b_cnx.cnx_handle, H_TYPE_TRANS))
> + return -EEXIST;
> + ev->r.retcode = transport->bind_cnx(
> + ev->u.b_cnx.session_handle,
> + ev->u.b_cnx.cnx_handle,
> + ev->u.b_cnx.transport_fd,
> + ev->u.b_cnx.is_leading);
> + break;
> + case ISCSI_UEVENT_SET_PARAM:
> + if (!iscsi_if_find_cnx(ev->u.set_param.cnx_handle,
> + H_TYPE_TRANS))
> + return -EEXIST;
> + ev->r.retcode = transport->set_param(
> + ev->u.set_param.cnx_handle,
> + ev->u.set_param.param, ev->u.set_param.value);
> + break;
> + case ISCSI_UEVENT_START_CNX:
> + if (!iscsi_if_find_cnx(ev->u.start_cnx.cnx_handle,
> + H_TYPE_TRANS))
> + return -EEXIST;
> + ev->r.retcode = transport->start_cnx(
> + ev->u.start_cnx.cnx_handle);
> + break;
> + case ISCSI_UEVENT_STOP_CNX:
> + if (!iscsi_if_find_cnx(ev->u.stop_cnx.cnx_handle, H_TYPE_TRANS))
> + return -EEXIST;
> + transport->stop_cnx(ev->u.stop_cnx.cnx_handle,
> + ev->u.stop_cnx.flag);
> + break;
> + case ISCSI_UEVENT_SEND_PDU:
> + if (!iscsi_if_find_cnx(ev->u.send_pdu.cnx_handle,
> + H_TYPE_TRANS))
> + return -EEXIST;
> + ev->r.retcode = transport->send_pdu(
> + ev->u.send_pdu.cnx_handle,
> + (struct iscsi_hdr*)((char*)ev + sizeof(*ev)),
> + (char*)ev + sizeof(*ev) + ev->u.send_pdu.hdr_size,
> + ev->u.send_pdu.data_size);
> + break;
> + case ISCSI_UEVENT_GET_STATS: {
> + struct iscsi_stats *stats;
> + struct sk_buff *skbstat;
> + struct iscsi_if_cnx *cnx;
> + struct nlmsghdr *nlhstat;
> + struct iscsi_uevent *evstat;
> + int len = NLMSG_SPACE(sizeof(*ev) +
> + sizeof(struct iscsi_stats) +
> + sizeof(struct iscsi_stats_custom) *
> + ISCSI_STATS_CUSTOM_MAX);
> + int err;
> +
> + cnx = iscsi_if_find_cnx(ev->u.get_stats.cnx_handle,
> + H_TYPE_TRANS);
> + if (!cnx)
> + return -EEXIST;
> +
> + do {
> + int actual_size;
> +
> + skbstat = mempool_zone_get_skb(&cnx->z_pdu);
> + if (!skbstat) {
> + printk("iscsi%d: can not deliver stats: OOM\n",
> + cnx->host->host_no);
> + return -ENOMEM;
> + }
> +
> + nlhstat = __nlmsg_put(skbstat, daemon_pid, 0, 0,
> + (len - sizeof(*nlhstat)));
> + evstat = NLMSG_DATA(nlhstat);
> + memset(evstat, 0, sizeof(*evstat));
> + evstat->transport_handle = iscsi_handle(cnx->transport);
> + evstat->type = nlh->nlmsg_type;
> + if (cnx->z_pdu.allocated >= cnx->z_pdu.hiwat)
> + evstat->iferror = -ENOMEM;
> + evstat->u.get_stats.cnx_handle =
> + ev->u.get_stats.cnx_handle;
> + stats = (struct iscsi_stats *)
> + ((char*)evstat + sizeof(*evstat));
> + memset(stats, 0, sizeof(*stats));
> +
> + transport->get_stats(ev->u.get_stats.cnx_handle, stats);
> + actual_size = NLMSG_SPACE(sizeof(struct iscsi_uevent) +
> + sizeof(struct iscsi_stats) +
> + sizeof(struct iscsi_stats_custom) *
> + stats->custom_length);
> + actual_size -= sizeof(*nlhstat);
> + actual_size = NLMSG_LENGTH(actual_size);
> + skb_trim(skb, NLMSG_ALIGN(actual_size));
> + nlhstat->nlmsg_len = actual_size;
> +
> + err = iscsi_unicast_skb(&cnx->z_pdu, skbstat);
> + } while (err < 0 && err != -ECONNREFUSED);
> + } break;
> + default:
> + err = -EINVAL;
> + break;
> + }
> +
> + return err;
> +}
[Mike, can you please insert the patches inline? I hate needing to save
attachment first to be able to comment on the patch]
- * Copyright (C) Mike Christie, 2004
+ * Copyright (C) Mike Christie, Dmitry Yusupov, Alex Aizman, 2004 - 2005
should probably be separate Copyright lines for everyone involved.
+static struct iscsi_transport *transport_table[ISCSI_TRANSPORT_MAX];
please avoid static limits for the number of transports, e.g. use the
lib/idr.c helpers.
+static DECLARE_MUTEX(callsema);
horrible name for a lock, even a static one.
+
+struct mempool_zone {
+ mempool_t *pool;
+ volatile int allocated;
don't use volatile, either atomic_t or if it's properly locked just int
+
+#define Z_SIZE_REPLY NLMSG_SPACE(sizeof(struct iscsi_uevent))
+#define Z_MAX_REPLY 8
+#define Z_HIWAT_REPLY 6
+
+#define Z_SIZE_PDU NLMSG_SPACE(sizeof(struct iscsi_uevent) + \
+ sizeof(struct iscsi_hdr) + \
+ DEFAULT_MAX_RECV_DATA_SEGMENT_LENGTH)
+#define Z_MAX_PDU 8
+#define Z_HIWAT_PDU 6
+
+#define Z_SIZE_ERROR NLMSG_SPACE(sizeof(struct iscsi_uevent))
+#define Z_MAX_ERROR 16
+#define Z_HIWAT_ERROR 12
At least the *_Z_SIZE defines are unneeded, just pass them to the functions
directly. And please add some explanations for the MAX and HIWAT defines.
+struct iscsi_if_cnx {
+ struct list_head item; /* item in cnxlist */
+ struct list_head snxitem; /* item in snx->connections */
please rename cnx to conn and snx to session everywhere, keeps the code
a lot more readable.
+ iscsi_cnx_t cnxh;
+ volatile int active;
volatile usage again
+#define H_TYPE_TRANS 1
+#define H_TYPE_HOST 2
+static struct iscsi_if_cnx*
+iscsi_if_find_cnx(uint64_t key, int type)
+{
+ unsigned long flags;
+ struct iscsi_if_cnx *cnx;
+
+ spin_lock_irqsave(&cnxlock, flags);
+ list_for_each_entry(cnx, &cnxlist, item) {
+ if ((type == H_TYPE_TRANS && cnx->cnxh == key) ||
+ (type == H_TYPE_HOST && cnx->host == iscsi_ptr(key))) {
+ spin_unlock_irqrestore(&cnxlock, flags);
+ return cnx;
+ }
+ }
+ spin_unlock_irqrestore(&cnxlock, flags);
+ return NULL;
}
please use two separate helpers for transport/host instead of the H_TYPE
thing.
+ list_del((void*)&skb->cb);
please add some inline helpers for using skb->cb as list instead of
spreading the casts all over.
+static int zone_init(struct mempool_zone *zp, unsigned max,
+ unsigned size, unsigned hiwat)
should probably become mempool_zone_init to match the other functions
operating on struct mempool_zone.
+static int
+iscsi_if_destroy_snx(struct iscsi_transport *transport, struct iscsi_uevent *ev)
+{
+ struct Scsi_Host *shost;
+ struct iscsi_if_snx *snx;
+ unsigned long flags;
+ struct iscsi_if_cnx *cnx;
+
+ shost = scsi_host_lookup(ev->u.d_session.sid);
+ if (shost == ERR_PTR(-ENXIO))
+ return -EEXIST;
+ scsi_host_put(shost);
you must keep the reference until you're done.
+ spin_unlock_irqrestore(&cnxlock, flags);
+
+ scsi_remove_host(shost);
+ transport->destroy_session(ev->u.d_session.session_handle);
can we please move the scsi_remove_host into the individual ->destroy_session
methods? dito for the scsi_host_add
+static int
+iscsi_if_create_cnx(struct iscsi_transport *transport, struct iscsi_uevent *ev)
+{
+ struct iscsi_if_snx *snx;
+ struct Scsi_Host *shost;
+ struct iscsi_if_cnx *cnx;
+ unsigned long flags;
+ int error;
+
+ shost = scsi_host_lookup(ev->u.c_cnx.sid);
+ if (shost == ERR_PTR(-ENXIO))
+ return -EEXIST;
+ scsi_host_put(shost);
again, please keep the reference until you're done.