Re: [RFC-v4 11/12] iscsi-target: Add misc utility and debug logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/20/2011 04:31 AM, Nicholas A. Bellinger wrote

+
+/*
+ *	Called with cmd->r2t_lock held.
+ */
+void iscsit_free_r2t(struct iscsi_r2t *r2t, struct iscsi_cmd *cmd)
+{
+	list_del(&r2t->r2t_list);
+	kmem_cache_free(lio_r2t_cache, r2t);
+}
+
+void iscsit_free_r2ts_from_list(struct iscsi_cmd *cmd)
+{
+	struct iscsi_r2t *r2t, *r2t_tmp;
+
+	spin_lock_bh(&cmd->r2t_lock);
+	list_for_each_entry_safe(r2t, r2t_tmp,&cmd->cmd_r2t_list, r2t_list) {
+		list_del(&r2t->r2t_list);
+		kmem_cache_free(lio_r2t_cache, r2t);

I think that is iscsit_free_r2t()

+	}
+	spin_unlock_bh(&cmd->r2t_lock);
+}
+
+/*
+ *	May be called from interrupt context.


How does this get called from interrupt context? Is it a real interrupt or a soft irq? I could not find the code path.





+
+struct iscsi_r2t *iscsit_get_holder_for_r2tsn(
+	struct iscsi_cmd *cmd,
+	u32 r2t_sn)
+{
+	struct iscsi_r2t *r2t;
+
+	spin_lock_bh(&cmd->r2t_lock);
+	list_for_each_entry(r2t,&cmd->cmd_r2t_list, r2t_list) {
+		if (r2t->r2t_sn == r2t_sn)
+			break;
+	}
+	spin_unlock_bh(&cmd->r2t_lock);
+
+	return (r2t) ? r2t : NULL;

Don't need "( ")".


+}
+
+#define SERIAL_BITS	31
+#define MAX_BOUND	(u32)2147483647UL
+
+static inline int serial_lt(u32 x, u32 y)
+{
+	return (x != y)&&  (((x<  y)&&  ((y - x)<  MAX_BOUND)) ||
+		((x>  y)&&  ((x - y)>  MAX_BOUND)));
+}
+
+static inline int serial_lte(u32 x, u32 y)
+{
+	return (x == y) ? 1 : serial_lt(x, y);
+}
+
+static inline int serial_gt(u32 x, u32 y)
+{
+	return (x != y)&&  (((x<  y)&&  ((y - x)>  MAX_BOUND)) ||
+		((x>  y)&&  ((x - y)<  MAX_BOUND)));
+}
+
+static inline int serial_gte(u32 x, u32 y)
+{
+	return (x == y) ? 1 : serial_gt(x, y);
+}


You should merged these with libiscsi.c's iscsi_sna_* and put in iscsi_proto.h.

I think this is not iscsi specific is it? It seems like it could go someone more generic.



+void iscsit_release_cmd_direct(struct iscsi_cmd *cmd)
+{
+	iscsit_free_r2ts_from_list(cmd);
+	iscsit_free_all_datain_reqs(cmd);
+
+	kfree(cmd->buf_ptr);
+	kfree(cmd->pdu_list);
+	kfree(cmd->seq_list);
+	kfree(cmd->tmr_req);
+	kfree(cmd->iov_data);
+
+	kmem_cache_free(lio_cmd_cache, cmd);
+}
+
+void __iscsit_release_cmd_to_pool(struct iscsi_cmd *cmd, struct iscsi_session *sess)
+{
+	struct iscsi_conn *conn = cmd->conn;
+
+	iscsit_free_r2ts_from_list(cmd);
+	iscsit_free_all_datain_reqs(cmd);
+
+	kfree(cmd->buf_ptr);
+	kfree(cmd->pdu_list);
+	kfree(cmd->seq_list);
+	kfree(cmd->tmr_req);
+	kfree(cmd->iov_data);
+
+	if (conn) {
+		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
+		iscsit_remove_cmd_from_response_queue(cmd, conn);
+	}
+
+	kmem_cache_free(lio_cmd_cache, cmd);
+}

sess is not used and I could not figure the _to_pool part of the name.

This is not some sort of mistake, right? iscsit_release_cmd_direct and __iscsit_release_cmd_to_pool look alike except for the conn check related code. Did you mean to merge those functions?



+
+void iscsit_release_cmd_to_pool(struct iscsi_cmd *cmd)
+{
+	if (!cmd->conn&&  !cmd->sess) {
+		iscsit_release_cmd_direct(cmd);
+	} else {
+		__iscsit_release_cmd_to_pool(cmd, (cmd->conn) ?
+			cmd->conn->sess : cmd->sess);
+	}
+}
+
+/*
+ *	Routine to pack an ordinary (LINUX) LUN 32-bit number
+ *		into an 8-byte LUN structure
+ *	(see SAM-2, Section 4.12.3 page 39)
+ *	Thanks to UNH for help with this :-).
+ */
+inline u64 iscsit_pack_lun(unsigned int lun)
+{
+	u64	result;
+
+	result = ((lun&  0xff)<<  8);	/* LSB of lun into byte 1 big-endian */
+
+	if (0) {
+		/* use flat space addressing method, SAM-2 Section 4.12.4
+			-	high-order 2 bits of byte 0 are 01
+			-	low-order 6 bits of byte 0 are MSB of the lun
+			-	all 8 bits of byte 1 are LSB of the lun
+			-	all other bytes (2 thru 7) are 0
+		 */
+		result |= 0x40 | ((lun>>  8)&  0x3f);
+	}
+	/* else use peripheral device addressing method, Sam-2 Section 4.12.5
+			-	high-order 2 bits of byte 0 are 00
+			-	low-order 6 bits of byte 0 are all 0
+			-	all 8 bits of byte 1 are the lun
+			-	all other bytes (2 thru 7) are 0
+	*/
+
+	return cpu_to_le64(result);
+}

Is this int_to_scsilun?


+
+/*
+ *	Routine to pack an 8-byte LUN structure into a ordinary (LINUX) 32-bit
+ *	LUN number (see SAM-2, Section 4.12.3 page 39)
+ *	Thanks to UNH for help with this :-).
+ */
+inline u32 iscsit_unpack_lun(unsigned char *lun_ptr)
+{
+	u32	result, temp;
+
+	result = *(lun_ptr+1);  /* LSB of lun from byte 1 big-endian */
+
+	switch (temp = ((*lun_ptr)>>6)) { /* high 2 bits of byte 0 big-endian */
+	case 0: /* peripheral device addressing method, Sam-2 Section 4.12.5
+		-	high-order 2 bits of byte 0 are 00
+		-	low-order 6 bits of byte 0 are all 0
+		-	all 8 bits of byte 1 are the lun
+		-	all other bytes (2 thru 7) are 0
+		 */
+		if (*lun_ptr != 0) {
+			printk(KERN_ERR "Illegal Byte 0 in LUN peripheral"
+				" device addressing method %u, expected 0\n",
+				*lun_ptr);
+		}
+		break;
+	case 1: /* flat space addressing method, SAM-2 Section 4.12.4
+		-	high-order 2 bits of byte 0 are 01
+		-	low-order 6 bits of byte 0 are MSB of the lun
+		-	all 8 bits of byte 1 are LSB of the lun
+		-	all other bytes (2 thru 7) are 0
+		 */
+		result += ((*lun_ptr)&  0x3f)<<  8;
+		break;
+	default: /* (extended) logical unit addressing */
+		printk(KERN_ERR "Unimplemented LUN addressing method %u, "
+			"PDA method used instead\n", temp);
+		break;
+	}
+
+	return result;
+}


scsilun_to_int?





+
+int iscsit_check_session_usage_count(struct iscsi_session *sess)
+{
+	spin_lock_bh(&sess->session_usage_lock);
+	if (atomic_read(&sess->session_usage_count)) {
+		atomic_set(&sess->session_waiting_on_uc, 1);
+		spin_unlock_bh(&sess->session_usage_lock);
+		if (in_interrupt())
+			return 2;
+
+		wait_for_completion(&sess->session_waiting_on_uc_comp);
+		return 1;
+	}
+	spin_unlock_bh(&sess->session_usage_lock);
+
+	return 0;
+}
+
+void iscsit_dec_session_usage_count(struct iscsi_session *sess)
+{
+	spin_lock_bh(&sess->session_usage_lock);
+	atomic_dec(&sess->session_usage_count);
+
+	if (!atomic_read(&sess->session_usage_count)&&
+	     atomic_read(&sess->session_waiting_on_uc))
+		complete(&sess->session_waiting_on_uc_comp);
+
+	spin_unlock_bh(&sess->session_usage_lock);
+}
+
+void iscsit_inc_session_usage_count(struct iscsi_session *sess)
+{
+	spin_lock_bh(&sess->session_usage_lock);
+	atomic_inc(&sess->session_usage_count);
+	spin_unlock_bh(&sess->session_usage_lock);
+}



It seems strange for the session_usage_count and waiting_on_uc to be atomic and accessed under the spin lock. I think you normally do one or the other.



+
+unsigned char *iscsit_ntoa(u32 ip)
+{
+	static unsigned char buf[18];
+
+	memset(buf, 0, 18);
+	sprintf(buf, "%u.%u.%u.%u", ((ip>>  24)&  0xff), ((ip>>  16)&  0xff),
+			((ip>>  8)&  0xff), (ip&  0xff));
+
+	return buf;
+}

Nothing is using this. Remove.


+
+void iscsit_ntoa2(unsigned char *buf, u32 ip)
+{
+	memset(buf, 0, 18);
+	sprintf(buf, "%u.%u.%u.%u", ((ip>>  24)&  0xff), ((ip>>  16)&  0xff),
+			((ip>>  8)&  0xff), (ip&  0xff));
+}

I think we have a function like this already.


If not, I think this should be:

sprintf(buf, "%pI4",

What s up with ipv6 btw? That uses %pI6.


+
+#define NS_INT16SZ	 2
+#define NS_INADDRSZ	 4
+#define NS_IN6ADDRSZ	16
+
+/* const char *
+ * inet_ntop4(src, dst, size)
+ *	format an IPv4 address
+ * return:
+ *	`dst' (as a const)
+ * notes:
+ *	(1) uses no statics
+ *	(2) takes a unsigned char* not an in_addr as input
+ * author:
+ *	Paul Vixie, 1996.
+ */
+static const char *iscsit_ntop4(


Only used by iscsit_ntop6


+/* const char *
+ * isc_inet_ntop6(src, dst, size)
+ * convert IPv6 binary address into presentation (printable) format
+ * author:
+ *	Paul Vixie, 1996.
+ */
+const char *iscsit_ntop6(const unsigned char *src, char *dst, size_t size)
+{


Not used.


+
+/* int
+ * inet_pton4(src, dst)
+ *	like inet_aton() but without all the hexadecimal and shorthand.
+ * return:
+ *	1 if `src' is a valid dotted quad, else 0.
+ * notice:
+ *	does not touch `dst' unless it's returning 1.
+ * author:
+ *	Paul Vixie, 1996.
+ */
+static int iscsit_pton4(const char *src, unsigned char *dst)
+{



Only used by inet_pton6

+
+/* int
+ * inet_pton6(src, dst)
+ *	convert presentation level address to network order binary form.
+ * return:
+ *	1 if `src' is a valid [RFC1884 2.2] address, else 0.
+ * notice:
+ *	(1) does not touch `dst' unless it's returning 1.
+ *	(2) :: in a full address is silently ignored.
+ * credit:
+ *	inspired by Mark Andrews.
+ * author:
+ *	Paul Vixie, 1996.
+ */
+int iscsit_pton6(const char *src, unsigned char *dst)
+{
+	static const char xdigits_l[] = "0123456789abcdef",



Not used.

I think if you needed these functions then they should go into some network code. There were not specific to a iscsi target were they.



+
+	return NULL;
+}
+
+void iscsit_check_conn_usage_count(struct iscsi_conn *conn)
+{
+	spin_lock_bh(&conn->conn_usage_lock);
+	if (atomic_read(&conn->conn_usage_count)) {
+		atomic_set(&conn->conn_waiting_on_uc, 1);
+		spin_unlock_bh(&conn->conn_usage_lock);
+
+		wait_for_completion(&conn->conn_waiting_on_uc_comp);
+		return;
+	}
+	spin_unlock_bh(&conn->conn_usage_lock);
+}
+
+void iscsit_dec_conn_usage_count(struct iscsi_conn *conn)
+{
+	spin_lock_bh(&conn->conn_usage_lock);
+	atomic_dec(&conn->conn_usage_count);
+
+	if (!atomic_read(&conn->conn_usage_count)&&
+	     atomic_read(&conn->conn_waiting_on_uc))
+		complete(&conn->conn_waiting_on_uc_comp);
+
+	spin_unlock_bh(&conn->conn_usage_lock);
+}
+
+void iscsit_inc_conn_usage_count(struct iscsi_conn *conn)
+{
+	spin_lock_bh(&conn->conn_usage_lock);
+	atomic_inc(&conn->conn_usage_count);
+	spin_unlock_bh(&conn->conn_usage_lock);
+}

Something about atomics and always being accessed under a lock. I think this happens in other places too.

Could this also be done with krefs?



+
+int iscsit_check_for_active_network_device(struct iscsi_conn *conn)
+{
+	struct net_device *net_dev;
+
+	if (!conn->net_if) {
+		printk(KERN_ERR "struct iscsi_conn->net_if is NULL for CID:"
+			" %hu\n", conn->cid);
+		return 0;
+	}
+	net_dev = conn->net_if;
+
+	return netif_carrier_ok(net_dev);
+}
+
+static void iscsit_handle_netif_timeout(unsigned long data)
+{
+	struct iscsi_conn *conn = (struct iscsi_conn *) data;
+
+	iscsit_inc_conn_usage_count(conn);
+
+	spin_lock_bh(&conn->netif_lock);
+	if (conn->netif_timer_flags&  ISCSI_TF_STOP) {
+		spin_unlock_bh(&conn->netif_lock);
+		iscsit_dec_conn_usage_count(conn);
+		return;
+	}
+	conn->netif_timer_flags&= ~ISCSI_TF_RUNNING;
+
+	if (iscsit_check_for_active_network_device((void *)conn)) {
+		iscsit_start_netif_timer(conn);
+		spin_unlock_bh(&conn->netif_lock);
+		iscsit_dec_conn_usage_count(conn);
+		return;
+	}
+
+	printk(KERN_ERR "Detected PHY loss on Network Interface: %s for iSCSI"
+		" CID: %hu on SID: %u\n", conn->net_dev, conn->cid,
+			conn->sess->sid);
+
+	spin_unlock_bh(&conn->netif_lock);
+
+	iscsit_cause_connection_reinstatement(conn, 0);
+	iscsit_dec_conn_usage_count(conn);
+}


I think instead of polling the device you can use register_netdevice_notifier. See fcoe.c for an example.



+
+static inline int iscsit_do_rx_data(
+	struct iscsi_conn *conn,
+	struct iscsi_data_count *count)
+{



+
+	while (total_rx<  data) {
+		oldfs = get_fs();
+		set_fs(get_ds());
+
+		conn->sock->sk->sk_allocation = GFP_ATOMIC;


I do not think you need GFP_ATOMIC. If you cannot sleep then I think you are in trouble below, because you pass in MSG_WAITALL.

I think since this is the target side you can use GFP_KERNEL. Target mode should not have the same allocation swinging around on you dependency problem like a initiator does, does it?

If it does at least use GFP_NOIO (I think iscsi_tcp.c should be using GFP_NOIO and not GFP_ATOMIC).

And I think we are supposed to be using kernel_recvmsg. It does the get/set_ds stuff for you.


+		rx_loop = sock_recvmsg(conn->sock,&msg,
+				(data - total_rx), MSG_WAITALL);
+




+static inline int iscsit_do_tx_data(
+	struct iscsi_conn *conn,
+	struct iscsi_data_count *count)
+{



+
+	while (total_tx<  data) {
+		oldfs = get_fs();
+		set_fs(get_ds());
+
+		conn->sock->sk->sk_allocation = GFP_ATOMIC;

Same comment as the recv side. I think you can also move this and set it in one place.

And I think we are supposed to be using kernel_sendmsg. That will also do the get/set_fs stuff for you.


+		tx_loop = sock_sendmsg(conn->sock,&msg, (data - total_tx));
+
+		set_fs(oldfs);
+







+extern int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
+{
+	char *ip, *payload = NULL;
+	struct iscsi_conn *conn = cmd->conn;
+	struct iscsi_portal_group *tpg;
+	struct iscsi_tiqn *tiqn;
+	struct iscsi_tpg_np *tpg_np;
+	int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
+	unsigned char buf[256];
+	unsigned char buf_ipv4[IPV4_BUF_SIZE];
+
+	buffer_len = (conn->conn_ops->MaxRecvDataSegmentLength>  32768) ?
+			32768 : conn->conn_ops->MaxRecvDataSegmentLength;
+
+	payload = kzalloc(buffer_len, GFP_KERNEL);
+	if (!payload) {
+		printk(KERN_ERR "Unable to allocate memory for sendtargets"
+			" response.\n");
+		return -1;
+	}
+
+	spin_lock(&tiqn_lock);
+	list_for_each_entry(tiqn,&g_tiqn_list, tiqn_list) {
+		memset(buf, 0, 256);
+
+		len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
+		len += 1;
+
+		if ((len + payload_len)>  buffer_len) {
+			spin_unlock(&tiqn->tiqn_tpg_lock);
+			end_of_buf = 1;
+			goto eob;
+		}
+		memcpy((void *)payload + payload_len, buf, len);
+		payload_len += len;
+
+		spin_lock(&tiqn->tiqn_tpg_lock);
+		list_for_each_entry(tpg,&tiqn->tiqn_tpg_list, tpg_list) {
+
+			spin_lock(&tpg->tpg_state_lock);
+			if ((tpg->tpg_state == TPG_STATE_FREE) ||
+			    (tpg->tpg_state == TPG_STATE_INACTIVE)) {
+				spin_unlock(&tpg->tpg_state_lock);
+				continue;
+			}
+			spin_unlock(&tpg->tpg_state_lock);
+
+			spin_lock(&tpg->tpg_np_lock);
+			list_for_each_entry(tpg_np,&tpg->tpg_gnp_list,
+					tpg_np_list) {
+				memset(buf, 0, 256);
+
+				if (tpg_np->tpg_np->np_sockaddr.ss_family == AF_INET6) {
+					ip =&tpg_np->tpg_np->np_ipv6[0];


Is ip supposed to be a string with the ip address in it? If so is that right? Is np_ipv6 a string with the ip address in human readable format, but below np_ipv4 is the integer representation then you convert it.


+				} else {
+					memset(buf_ipv4, 0, IPV4_BUF_SIZE);
+					iscsit_ntoa2(buf_ipv4,
+						tpg_np->tpg_np->np_ipv4);
+					ip =&buf_ipv4[0];
+				}
+
+				len = sprintf(buf, "TargetAddress="
+					"%s%s%s:%hu,%hu",
+					(tpg_np->tpg_np->np_sockaddr.ss_family == AF_INET6) ?
+					"[" : "", ip,
+					(tpg_np->tpg_np->np_sockaddr.ss_family == AF_INET6) ?
+					"]" : "", tpg_np->tpg_np->np_port,
+					tpg->tpgt);
+				len += 1;
+
+				if ((len + payload_len)>  buffer_len) {
+					spin_unlock(&tpg->tpg_np_lock);
+					spin_unlock(&tiqn->tiqn_tpg_lock);
+					end_of_buf = 1;
+					goto eob;
+				}
+
+				memcpy((void *)payload + payload_len, buf, len);
+				payload_len += len;
+			}
+			spin_unlock(&tpg->tpg_np_lock);
+		}
+		spin_unlock(&tiqn->tiqn_tpg_lock);
+eob:
+		if (end_of_buf)
+			break;
+	}
+	spin_unlock(&tiqn_lock);
+
+	cmd->buf_ptr = payload;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux