[PATCH 1/3] Rewrite iSCSI TCP receive handling
Olaf Kirch
The basic idea of this patch is to replace the existing struct
iscsi_tcp_recv with a different object that layers things a little
better. The new struct has two callbacks - a recv callback for receiving
data, and a done() callback that is invoked when data is complete and
can be processed. The done() callback should not do any processing
on the raw data, or mess with buffers and such - it gets called
when all the data has arrived, and the CRC is fine.
The main routine is iscsi_rcv, which just processes an incoming
sk_buff (using the skb_seq_read interface), and repeatedly calls
the current recv() routine, and when the chunk is complete, it calls
done().
There are two different recv handlers - one for plain copy, and one
for copy+crc. Both transparently take care of padding, and walking
a scatterlist (if that is the destination of the copy operation).
There are three different sets of handlers for receiving data:
- for the header
- for data in replies
- for any other command responses
This draft patch does not remove any of the existing code, in order
to keep the patch readable. A follow-up patch will delete any dead and
unused code/variables.
Olaf
---
drivers/scsi/iscsi_tcp.c | 697 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/scsi/iscsi_tcp.h | 30 ++
2 files changed, 723 insertions(+), 4 deletions(-)
Index: iscsi-2.6/drivers/scsi/iscsi_tcp.c
===================================================================
--- iscsi-2.6.orig/drivers/scsi/iscsi_tcp.c
+++ iscsi-2.6/drivers/scsi/iscsi_tcp.c
@@ -46,7 +46,7 @@ MODULE_AUTHOR("Dmitry Yusupov <dmitry_yu
"Alex Aizman <itn...@yahoo.com>");
MODULE_DESCRIPTION("iSCSI/TCP data-path");
MODULE_LICENSE("GPL");
-/* #define DEBUG_TCP */
+#undef DEBUG_TCP
#define DEBUG_ASSERT
#ifdef DEBUG_TCP
@@ -65,6 +65,11 @@ MODULE_LICENSE("GPL");
static unsigned int iscsi_max_lun = 512;
module_param_named(max_lun, iscsi_max_lun, uint, S_IRUGO);
+static int iscsi_tcp_begin_data_in(struct iscsi_tcp_conn *,
+ struct iscsi_cmd_task *);
+static int iscsi_tcp_begin_cmd_rsp(struct iscsi_tcp_conn *, int);
+static int iscsi_tcp_begin_header(struct iscsi_tcp_conn *);
+
static inline void
iscsi_buf_init_iov(struct iscsi_buf *ibuf, char *vbuf, int size)
{
@@ -111,6 +116,298 @@ iscsi_hdr_digest(struct iscsi_conn *conn
buf->sg.length += sizeof(u32);
}
+/*
+ * Scatterlist handling: inside the iscsi_chunk, we
+ * remember an index into the scatterlist, and set data/size
+ * to the current scatterlist entry. For highmem pages, we
+ * kmap as needed.
+ *
+ * Note that the page is unmapped when we return from
+ * TCP's data_ready handler, so we may end up mapping and
+ * unmapping the same page repeatedly. The whole reason
+ * for this is that we shouldn't keep the page mapped
+ * outside the softirq.
+ */
+
+/**
+ * iscsi_tcp_chunk_seek_sg - seek to indicated scatterlist entry
+ * @chunk: the buffer object
+ * @idx: index into scatterlist
+ * @offset: byte offset into that sg entry
+ *
+ * This function sets up the chunk so that subsequent
+ * data is copied to the indicated sg entry, at the given
+ * offset.
+ */
+static inline void
+iscsi_tcp_chunk_seek_sg(struct iscsi_chunk *chunk,
+ unsigned int idx, unsigned int offset)
+{
+ struct scatterlist *sg;
+
+ BUG_ON(chunk->sg == NULL);
+ BUG_ON(chunk->sg_mapped);
+
+ sg = &chunk->sg[idx];
+ chunk->sg_index = idx;
+ chunk->sg_offset = offset;
+ chunk->size = min(sg->length - offset, chunk->total_size);
+
+ if (likely(!PageHighMem(sg->page)))
+ chunk->data = page_address(sg->page) +
+ sg->offset + chunk->sg_offset;
+ else
+ chunk->data = NULL;
+}
+
+/**
+ * iscsi_tcp_chunk_map - map the current S/G page
+ */
+static inline void
+iscsi_tcp_chunk_map(struct iscsi_chunk *chunk)
+{
+ struct scatterlist *sg = &chunk->sg[chunk->sg_index];
+
+ BUG_ON(chunk->sg_mapped);
+ BUG_ON(sg->length == 0);
+ chunk->sg_mapped = kmap_atomic(sg->page, KM_SOFTIRQ0);
+ chunk->data = chunk->sg_mapped + sg->offset + chunk->sg_offset;
+}
+
+/**
+ * iscsi_tcp_chunk_unmap - unmap the current S/G page
+ */
+static inline void
+iscsi_tcp_chunk_unmap(struct iscsi_chunk *chunk)
+{
+ if (unlikely(chunk->sg_mapped)) {
+ kunmap_atomic(chunk->sg_mapped, KM_SOFTIRQ0);
+ chunk->sg_mapped = NULL;
+ chunk->data = NULL;
+ }
+}
+
+/**
+ * iscsi_tcp_chunk_done - check whether the chunk is complete
+ *
+ * Check if we're done receiving this chunk. If the receive
+ * buffer is full but we expect more data, move on to the
+ * next entry in the scatterlist.
+ *
+ * If the amount of data we received isn't a multiple of 4,
+ * we will transparently receive the pad bytes, too.
+ *
+ * This function must be re-entrant.
+ */
+static inline int
+iscsi_tcp_chunk_done(struct iscsi_chunk *chunk)
+{
+ static unsigned char padbuf[ISCSI_PAD_LEN];
+
+ if (likely(chunk->copied < chunk->size)) {
+ /* Map scatterlist entry if needed */
+ if (unlikely(chunk->data == NULL && chunk->sg))
+ iscsi_tcp_chunk_map(chunk);
+ return 0;
+ }
+
+ chunk->total_copied += chunk->copied;
+ chunk->copied = 0;
+ chunk->size = 0;
+
+ /* Unmap the current scatterlist page, if there is one. */
+ iscsi_tcp_chunk_unmap(chunk);
+
+ /* Do we have more scatterlist entries? */
+ if (chunk->total_copied < chunk->total_size) {
+ /* Proceed to the next entry in the scatterlist. */
+ iscsi_tcp_chunk_seek_sg(chunk, chunk->sg_index + 1, 0);
+ BUG_ON(chunk->size == 0);
+ return 0;
+ }
+
+ /* Do we need to handle padding? */
+ if (chunk->total_copied & (ISCSI_PAD_LEN-1)) {
+ unsigned int pad;
+
+ pad = ISCSI_PAD_LEN - (chunk->total_copied & (ISCSI_PAD_LEN-1));
+
+ debug_tcp("consume %d pad bytes\n", pad);
+ chunk->total_size += pad;
+ chunk->size = pad;
+ chunk->data = padbuf;
+ return 0;
+ }
+
+ return 1;
+}
+
+/**
+ * iscsi_tcp_copy_to_chunk - copy data to chunk
+ * @tcp_conn: the iSCSI TCP connection
+ * @chunk: the buffer to copy to
+ * @ptr: data pointer
+ * @len: amount of data available
+ *
+ * This function copies up to @len bytes to the
+ * given buffer, and returns the number of bytes
+ * consumed, which can actually be less than @len.
+ */
+static int
+iscsi_tcp_copy_to_chunk(struct iscsi_tcp_conn *tcp_conn,
+ struct iscsi_chunk *chunk,
+ const void *ptr, size_t len)
+{
+ unsigned int copy, copied = 0;
+
+ while (!iscsi_tcp_chunk_done(chunk) && copied < len) {
+ copy = min(len - copied, chunk->size - chunk->copied);
+
+ memcpy(chunk->data + chunk->copied, ptr + copied, copy);
+ chunk->copied += copy;
+ copied += copy;
+ }
+
+ return copied;
+}
+
+/**
+ * iscsi_tcp_copy_and_crc_to_chunk - copy data to chunk while updating CRC32
+ * @tcp_conn: the iSCSI TCP connection
+ * @chunk: the buffer to copy to
+ * @ptr: data pointer
+ * @len: amount of data available
+ *
+ * This function copies up to @len bytes to the
+ * given buffer, and updates the CRC32 checksum while
+ * doing so. It returns the number of bytes
+ * consumed, which can actually be less than @len.
+ *
+ * Combining these two operations doesn't buy us a lot (yet),
+ * but in the future we could implement combined copy+crc,
+ * just way we do for network layer checksums.
+ */
+static int
+iscsi_tcp_copy_and_crc_to_chunk(struct iscsi_tcp_conn *tcp_conn,
+ struct iscsi_chunk *chunk,
+ const void *ptr, size_t len)
+{
+ struct scatterlist sg;
+ unsigned int copy, copied = 0;
+
+ while (!iscsi_tcp_chunk_done(chunk)) {
+ if (copied == len)
+ goto out;
+
+ copy = min(len - copied, chunk->size - chunk->copied);
+ memcpy(chunk->data + chunk->copied, ptr + copied, copy);
+
+ sg_init_one(&sg, ptr + copied, copy);
+ crypto_hash_update(&tcp_conn->rx_hash, &sg, copy);
+
+ chunk->copied += copy;
+ copied += copy;
+ }
+
+ /* Set us up for receiving the digest */
+ chunk->recv = iscsi_tcp_copy_to_chunk;
+ chunk->data = chunk->rx_digest;
+ chunk->size = 4;
+ chunk->total_size += 4;
+
+out:
+ return copied;
+}
+
+/**
+ * iscsi_tcp_dgst_verify - if CRC32 is used, verify the checksum
+ */
+static inline int
+iscsi_tcp_dgst_verify(struct iscsi_tcp_conn *tcp_conn,
+ struct iscsi_chunk *chunk)
+{
+ unsigned char computed[4];
+
+ if (!chunk->rx_digest_enabled)
+ return 1;
+
+ crypto_hash_final(&tcp_conn->rx_hash, computed);
+ if (memcmp(computed, chunk->rx_digest, sizeof(computed))) {
+ debug_tcp("digest mismatch\n");
+ return 0;
+ }
+
+ return 1;
+}
+
+/*
+ * Helper function to set up chunk buffer
+ */
+static inline struct iscsi_chunk *
+__iscsi_chunk_init(struct iscsi_tcp_conn *tcp_conn, size_t size,
+ iscsi_chunk_done_fn_t *done, int digest)
+{
+ struct iscsi_chunk *chunk = &tcp_conn->in.chunk;
+
+ memset(chunk, 0, sizeof(*chunk));
+ chunk->total_size = size;
+ chunk->done = done;
+
+ if (digest) {
+ crypto_hash_init(&tcp_conn->rx_hash);
+ chunk->rx_digest_enabled = 1;
+ chunk->recv = iscsi_tcp_copy_and_crc_to_chunk;
+ } else {
+ chunk->recv = iscsi_tcp_copy_to_chunk;
+ }
+
+ return chunk;
+}
+
+static inline int
+iscsi_chunk_init_linear(struct iscsi_tcp_conn *tcp_conn,
+ void *data, unsigned int offset, size_t size,
+ iscsi_chunk_done_fn_t *done, int digest)
+{
+ struct iscsi_chunk *chunk;
+
+ chunk = __iscsi_chunk_init(tcp_conn, size, done, digest);
+ chunk->data = data + offset;
+ chunk->size = size;
+ return 0;
+}
+
+static inline int
+iscsi_chunk_init_sg(struct iscsi_tcp_conn *tcp_conn,
+ struct scatterlist *sg, unsigned int sg_count,
+ unsigned int offset, size_t size,
+ iscsi_chunk_done_fn_t *done, int digest)
+{
+ struct iscsi_chunk *chunk;
+ unsigned int i;
+ size_t sg_size = 0;
+
+ /* First, make sure the scatterlist is big enough */
+ for (i = 0; i < sg_count; ++i)
+ sg_size += sg[i].length;
+ if (sg_size < offset + size)
+ goto bad_datalen;
+
+ chunk = __iscsi_chunk_init(tcp_conn, size, done, digest);
+ for (i = 0; i < sg_count; ++i) {
+ if (offset < sg[i].length) {
+ chunk->sg = sg;
+ chunk->sg_count = sg_count;
+ iscsi_tcp_chunk_seek_sg(chunk, i, offset);
+ return 0;
+ }
+ offset -= sg[i].length;
+ }
+
+bad_datalen:
+ return ISCSI_ERR_DATALEN;
+}
+
static inline int
iscsi_hdr_extract(struct iscsi_tcp_conn *tcp_conn)
{
@@ -427,6 +724,141 @@ iscsi_r2t_rsp(struct iscsi_conn *conn, s
return 0;
}
+/**
+ * iscsi_tcp_hdr_dissect - process PDU header
+ * @conn: iSCSI connection
+ * @hdr: PDU header
+ *
+ * This function analyzes the header of the PDU received,
+ * and performs several sanity checks. If the PDU is accompanied
+ * by data, the receive buffer is set up to copy the incoming data
+ * to the correct location.
+ */
+static int
+iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
+{
+ int rc = 0, opcode, ahslen;
+ struct iscsi_session *session = conn->session;
+ struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
+ uint32_t itt;
+
+ /* verify PDU length */
+ tcp_conn->in.datalen = ntoh24(hdr->dlength);
+ if (tcp_conn->in.datalen > conn->max_recv_dlength) {
+ printk(KERN_ERR "iscsi_tcp: datalen %d > %d\n",
+ tcp_conn->in.datalen, conn->max_recv_dlength);
+ return ISCSI_ERR_DATALEN;
+ }
+ tcp_conn->data_copied = 0;
+
+ /* calculate read padding */
+ tcp_conn->in.padding = tcp_conn->in.datalen & (ISCSI_PAD_LEN-1);
+ if (tcp_conn->in.padding) {
+ tcp_conn->in.padding = ISCSI_PAD_LEN - tcp_conn->in.padding;
+ debug_scsi("read padding %d bytes\n", tcp_conn->in.padding);
+ }
+
+ /* Additional header segments. So far, we don't
+ * process additional headers.
+ */
+ ahslen = hdr->hlength << 2;
+
+ opcode = hdr->opcode & ISCSI_OPCODE_MASK;
+ /* verify itt (itt encoding: age+cid+itt) */
+ rc = iscsi_verify_itt(conn, hdr, &itt);
+ if (rc == ISCSI_ERR_NO_SCSI_CMD) {
+ /* XXX: what does this do? */
+ tcp_conn->in.datalen = 0; /* force drop */
+ return 0;
+ } else if (rc)
+ return rc;
+
+ debug_tcp("opcode 0x%x offset %d copy %d ahslen %d datalen %d\n",
+ opcode, tcp_conn->in.offset, tcp_conn->in.copy,
+ ahslen, tcp_conn->in.datalen);
+
+ switch(opcode) {
+ case ISCSI_OP_SCSI_DATA_IN:
+ tcp_conn->in.ctask = session->cmds[itt];
+ rc = iscsi_data_rsp(conn, tcp_conn->in.ctask);
+ if (rc)
+ return rc;
+ if (tcp_conn->in.datalen)
+ return iscsi_tcp_begin_data_in(tcp_conn,
+ tcp_conn->in.ctask);
+
+ /* fall through */
+ case ISCSI_OP_SCSI_CMD_RSP:
+ tcp_conn->in.ctask = session->cmds[itt];
+ if (tcp_conn->in.datalen)
+ return iscsi_tcp_begin_cmd_rsp(tcp_conn, 1);
+
+ /* Why not iscsi_complete_pdu(conn, hdr, NULL, 0)? */
+ spin_lock(&session->lock);
+ rc = __iscsi_complete_pdu(conn, hdr, NULL, 0);
+ spin_unlock(&session->lock);
+ break;
+ case ISCSI_OP_R2T:
+ tcp_conn->in.ctask = session->cmds[itt];
+ if (ahslen)
+ rc = ISCSI_ERR_AHSLEN;
+ else if (tcp_conn->in.ctask->sc->sc_data_direction ==
+ DMA_TO_DEVICE)
+ rc = iscsi_r2t_rsp(conn, tcp_conn->in.ctask);
+ else
+ rc = ISCSI_ERR_PROTO;
+ break;
+ case ISCSI_OP_LOGIN_RSP:
+ case ISCSI_OP_TEXT_RSP:
+ case ISCSI_OP_REJECT:
+ case ISCSI_OP_ASYNC_EVENT:
+ /*
+ * It is possible that we could get a PDU with a buffer larger
+ * than 8K, but there are no targets that currently do this.
+ * For now we fail until we find a vendor that needs it
+ */
+ if (ISCSI_DEF_MAX_RECV_SEG_LEN < tcp_conn->in.datalen) {
+ printk(KERN_ERR "iscsi_tcp: received buffer of len %u "
+ "but conn buffer is only %u (opcode %0x)\n",
+ tcp_conn->in.datalen,
+ ISCSI_DEF_MAX_RECV_SEG_LEN, opcode);
+ rc = ISCSI_ERR_PROTO;
+ break;
+ }
+
+ /* If there's data coming in with the response,
+ * receive it to the connection's buffer.
+ * During the login negotiation, data digests are
+ * turned off even if enabled on the connection.
+ */
+ if (tcp_conn->in.datalen)
+ return iscsi_tcp_begin_cmd_rsp(tcp_conn,
+ opcode != ISCSI_OP_LOGIN_RSP);
+
+ /* fall through */
+ case ISCSI_OP_LOGOUT_RSP:
+ case ISCSI_OP_NOOP_IN:
+ case ISCSI_OP_SCSI_TMFUNC_RSP:
+ rc = iscsi_complete_pdu(conn, hdr, NULL, 0);
+ break;
+ default:
+ rc = ISCSI_ERR_BAD_OPCODE;
+ break;
+ }
+
+ if (rc == 0) {
+ /* Anything that comes with data should have
+ * been handled above. */
+ BUG_ON(tcp_conn->in.datalen);
+ rc = iscsi_tcp_begin_header(tcp_conn);
+ }
+
+ return rc;
+}
+
+/*
+ * Process the reply header
+ */
static int
iscsi_tcp_hdr_recv(struct iscsi_conn *conn)
{
@@ -983,25 +1415,267 @@ again:
return processed;
}
+/**
+ * iscsi_tcp_process_header - process PDU header
+ *
+ * This is the callback invoked when the PDU header has
+ * been received. If the header is followed by additional
+ * header segments, we go back for more data.
+ */
+static int
+iscsi_tcp_process_header(struct iscsi_tcp_conn *tcp_conn,
+ struct iscsi_chunk *chunk)
+{
+ struct iscsi_conn *conn = tcp_conn->iscsi_conn;
+ struct iscsi_hdr *hdr;
+
+ /* Check if there are additional header segments
+ * *prior* to computing the digest, because we
+ * may need to go back to the caller for more.
+ */
+ hdr = &tcp_conn->hdr;
+ if (chunk->copied == sizeof(struct iscsi_hdr) && hdr->hlength) {
+ /* Bump the header length - the caller will
+ * just loop around and get the AHS for us, and
+ * call again. */
+ unsigned int ahslen = hdr->hlength << 2;
+
+ /* For now, we have this icky hdrext buffer
+ * that we can spill into. Yikes! */
+ if (ahslen > sizeof(tcp_conn->hdrext))
+ return ISCSI_ERR_AHSLEN;
+
+ chunk->total_size += ahslen;
+ chunk->size += ahslen;
+ return 0;
+ }
+
+ if (!iscsi_tcp_dgst_verify(tcp_conn, chunk)) {
+ /* If we had Fixed Interval Markers,
+ * we could recover more gracefully. For now,
+ * just punt the connection. */
+ return ISCSI_ERR_HDR_DGST;
+ }
+
+ tcp_conn->in.hdr = hdr;
+
+ return iscsi_tcp_hdr_dissect(conn, hdr);
+}
+
+static int
+iscsi_tcp_begin_header(struct iscsi_tcp_conn *tcp_conn)
+{
+ struct iscsi_conn *conn = tcp_conn->iscsi_conn;
+
+ debug_tcp("iscsi_tcp_begin_header(%p%s)\n", tcp_conn,
+ conn->hdrdgst_en? ", digest enabled" : "");
+ return iscsi_chunk_init_linear(tcp_conn,
+ &tcp_conn->hdr, 0, sizeof(struct iscsi_hdr),
+ iscsi_tcp_process_header,
+ conn->hdrdgst_en);
+}
+
+/*
+ * Handle incoming reply to DataIn command
+ */
+static int
+iscsi_tcp_process_data_in(struct iscsi_tcp_conn *tcp_conn, struct iscsi_chunk *chunk)
+{
+ struct iscsi_conn *conn = tcp_conn->iscsi_conn;
+ int rc;
+
+ if (!iscsi_tcp_dgst_verify(tcp_conn, chunk))
+ return ISCSI_ERR_DATA_DGST;
+
+ /* check for non-exceptional status */
+ if (tcp_conn->in.hdr->flags & ISCSI_FLAG_DATA_STATUS) {
+ debug_scsi("done [sc %lx res %d itt 0x%x flags 0x%x]\n",
+ (long)sc, sc->result, ctask->itt,
+ tcp_conn->in.hdr->flags);
+ rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr, NULL, 0);
+ if (rc)
+ return rc;
+ }
+
+ return iscsi_tcp_begin_header(tcp_conn);
+}
+
+static int
+iscsi_tcp_begin_data_in(struct iscsi_tcp_conn *tcp_conn,
+ struct iscsi_cmd_task *ctask)
+{
+ struct iscsi_conn *conn = tcp_conn->iscsi_conn;
+ struct iscsi_tcp_cmd_task *tcp_ctask = ctask->dd_data;
+ struct scsi_cmnd *sc = ctask->sc;
+ int rc = 0;
+
+ debug_tcp("iscsi_tcp_begin_data_in(%p, offset=%d, datalen=%d)\n",
+ tcp_conn, tcp_ctask->data_offset, tcp_conn->in.datalen);
+ BUG_ON((void*)ctask != sc->SCp.ptr);
+
+ /*
+ * Setup copy of Data-In into the Scsi_Cmnd
+ */
+ if (!sc->use_sg) {
+ /* Check all of these values separately to avoid
+ * overflows. */
+ if (tcp_conn->in.datalen > sc->request_bufflen ||
+ tcp_ctask->data_offset > sc->request_bufflen ||
+ tcp_ctask->data_offset + tcp_conn->in.datalen >
+ sc->request_bufflen)
+ return ISCSI_ERR_DATALEN;
+
+ rc = iscsi_chunk_init_linear(tcp_conn, sc->request_buffer,
+ tcp_ctask->data_offset,
+ tcp_conn->in.datalen,
+ iscsi_tcp_process_data_in,
+ conn->datadgst_en);
+ } else {
+ /* Scatterlist case:
+ * We set up the iscsi_chunk to point to the next
+ * scatterlist entry to copy to. As we go along,
+ * we move on to the next scatterlist entry and
+ * update the digest per-entry.
+ */
+ rc = iscsi_chunk_init_sg(tcp_conn,
+ (struct scatterlist *) sc->request_buffer,
+ sc->use_sg,
+ tcp_ctask->data_offset,
+ tcp_conn->in.datalen,
+ iscsi_tcp_process_data_in,
+ conn->datadgst_en);
+ }
+
+ return rc;
+}
+
+/*
+ * Handle incoming reply to any other type of command
+ */
+static int
+iscsi_tcp_process_cmd_rsp(struct iscsi_tcp_conn *tcp_conn,
+ struct iscsi_chunk *chunk)
+{
+ struct iscsi_conn *conn = tcp_conn->iscsi_conn;
+ int rc = 0;
+
+ if (!iscsi_tcp_dgst_verify(tcp_conn, chunk))
+ return ISCSI_ERR_DATA_DGST;
+
+ rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr,
+ conn->data, tcp_conn->in.datalen);
+ if (rc)
+ return rc;
+
+ return iscsi_tcp_begin_header(tcp_conn);
+}
+
+static int
+iscsi_tcp_begin_cmd_rsp(struct iscsi_tcp_conn *tcp_conn, int digest_ok)
+{
+ struct iscsi_conn *conn = tcp_conn->iscsi_conn;
+
+ return iscsi_chunk_init_linear(tcp_conn,
+ conn->data, 0, tcp_conn->in.datalen,
+ iscsi_tcp_process_cmd_rsp,
+ conn->datadgst_en && digest_ok);
+}
+
+/**
+ * iscsi_rcv - TCP receive in sendfile fashion
+ * @rd_desc: read descriptor
+ * @skb: socket buffer
+ * @offset: offset in skb
+ * @len: skb->len - offset
+ **/
+static int
+iscsi_rcv(read_descriptor_t *rd_desc, struct sk_buff *skb,
+ unsigned int offset, size_t len)
+{
+ struct iscsi_conn *conn = rd_desc->arg.data;
+ struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
+ struct iscsi_chunk *chunk = &tcp_conn->in.chunk;
+ struct skb_seq_state seq;
+ unsigned int consumed = 0;
+ int rc = 0;
+
+ /*
+ * Save current SKB and its offset in the corresponding
+ * connection context.
+ * Old-style interface - will go away
+ */
+ tcp_conn->in.copy = skb->len - offset;
+ tcp_conn->in.offset = offset;
+ tcp_conn->in.skb = skb;
+ tcp_conn->in.len = tcp_conn->in.copy;
+ BUG_ON(tcp_conn->in.copy <= 0);
+ debug_tcp("in %d bytes\n", skb->len - offset);
+
+ if (unlikely(conn->suspend_rx)) {
+ debug_tcp("conn %d Rx suspended!\n", conn->id);
+ return 0;
+ }
+
+ skb_prepare_seq_read(skb, offset, skb->len, &seq);
+ while (1) {
+ unsigned int avail;
+ const u8 *ptr;
+
+ avail = skb_seq_read(consumed, &ptr, &seq);
+ if (avail == 0)
+ break;
+ BUG_ON(chunk->copied >= chunk->size);
+
+ debug_tcp("skb %p ptr=%p avail=%u\n", skb, ptr, avail);
+ rc = chunk->recv(tcp_conn, chunk, ptr, avail);
+ BUG_ON(rc == 0);
+ consumed += rc;
+
+ if (chunk->copied >= chunk->size) {
+ rc = chunk->done(tcp_conn, chunk);
+ if (rc != 0)
+ goto error;
+
+ /* The done() functions sets up the
+ * next chunk. */
+ }
+ }
+
+ conn->rxdata_octets += consumed;
+ return consumed;
+
+error:
+ /* Receive error. We could initiate error recovery
+ * here. */
+ debug_tcp("Error receiving PDU, errno=%d\n", rc);
+ iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED);
+ return 0;
+}
+
static void
iscsi_tcp_data_ready(struct sock *sk, int flag)
{
struct iscsi_conn *conn = sk->sk_user_data;
+ struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
read_descriptor_t rd_desc;
read_lock(&sk->sk_callback_lock);
/*
- * Use rd_desc to pass 'conn' to iscsi_tcp_data_recv.
+ * Use rd_desc to pass 'conn' to iscsi_rcv.
* We set count to 1 because we want the network layer to
- * hand us all the skbs that are available. iscsi_tcp_data_recv
+ * hand us all the skbs that are available. iscsi_rcv
* handled pdus that cross buffers or pdus that still need data.
*/
rd_desc.arg.data = conn;
rd_desc.count = 1;
- tcp_read_sock(sk, &rd_desc, iscsi_tcp_data_recv);
+ tcp_read_sock(sk, &rd_desc, iscsi_rcv);
read_unlock(&sk->sk_callback_lock);
+
+ /* If we had to (atomically) map a highmem page,
+ * unmap it now. */
+ iscsi_tcp_chunk_unmap(&tcp_conn->in.chunk);
}
static void
@@ -1931,6 +2605,7 @@ iscsi_tcp_conn_bind(struct iscsi_cls_ses
* set receive state machine into initial state
*/
tcp_conn->in_progress = IN_PROGRESS_WAIT_HEADER;
+ iscsi_tcp_begin_header(tcp_conn);
return 0;
}
@@ -2023,6 +2698,20 @@ iscsi_conn_set_param(struct iscsi_cls_co
tcp_conn->hdr_size = sizeof(struct iscsi_hdr);
if (conn->hdrdgst_en)
tcp_conn->hdr_size += sizeof(__u32);
+ /* Here's a teeny little hack - if we're expecting
+ * a header next, make sure we got the digest
+ * setting right. */
+ if (tcp_conn->in.chunk.done == iscsi_tcp_process_header) {
+ if (tcp_conn->in.chunk.total_copied) {
+ printk(KERN_ERR
+ "iSCSI: Changed HeaderDigest setting "
+ "while receiving a header!\n");
+ } else {
+ /* Restart header reception with the
+ * updated HeaderDigest setting. */
+ iscsi_tcp_begin_header(tcp_conn);
+ }
+ }
break;
case ISCSI_PARAM_DATADGST_EN:
iscsi_set_param(cls_conn, param, buf, buflen);
Index: iscsi-2.6/drivers/scsi/iscsi_tcp.h
===================================================================
--- iscsi-2.6.orig/drivers/scsi/iscsi_tcp.h
+++ iscsi-2.6/drivers/scsi/iscsi_tcp.h
@@ -53,6 +53,34 @@
struct crypto_hash;
struct socket;
+struct iscsi_tcp_conn;
+struct iscsi_chunk;
+
+typedef int iscsi_chunk_recv_fn_t(struct iscsi_tcp_conn *,
+ struct iscsi_chunk *,
+ const void *, size_t);
+typedef int iscsi_chunk_done_fn_t(struct iscsi_tcp_conn *,
+ struct iscsi_chunk *);
+
+struct iscsi_chunk {
+ unsigned char * data;
+ unsigned int size;
+ unsigned int copied;
+ unsigned int total_size;
+ unsigned int total_copied;
+
+ unsigned int rx_digest_enabled : 1;
+ unsigned char rx_digest[4];
+
+ struct scatterlist * sg;
+ void * sg_mapped;
+ unsigned int sg_offset;
+ unsigned int sg_index;
+ unsigned int sg_count;
+
+ iscsi_chunk_recv_fn_t *recv;
+ iscsi_chunk_done_fn_t *done;
+};
/* Socket connection recieve helper */
struct iscsi_tcp_recv {
@@ -66,6 +94,8 @@ struct iscsi_tcp_recv {
int padding;
struct iscsi_cmd_task *ctask; /* current cmd in progress */
+ struct iscsi_chunk chunk;
+
/* copied and flipped values */
int datalen;
int datadgst;
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
ok...@lst.de | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax
Olaf Kirch
open-iscsi code was that the frame processing is heavily intertwined
with the interpretation of the data; in particular, the digest handling
crops up in lots of places (and I'm not even sure it's corrrect - in
several places it looks to me like we first tell the SCSI midlayer that
the command is done, and *then* we go check whether the CRC is good).
Also, there's still some code in there that seems to make assumptions
about how TCP segments data - for instance, we will currently fail a
reply if there are additional header segments that extend past the end
of the current sk_buff. The current code also seems to make it hard to
introduce things like fixed interval markers.
The following set of patches introduce a new recv handling,
and delete quite a bit of unused code. I tested this code against
iscsitarget, with and without header/data digests. Any additional
testing you may want to give this code is appreciated.
The patch set is against the linux-2.6-iscsi.git tree
Olaf
Mike Christie
> One of the things that struck me while trying to understand the
> open-iscsi code was that the frame processing is heavily intertwined
> with the interpretation of the data; in particular, the digest handling
> crops up in lots of places (and I'm not even sure it's corrrect - in
> several places it looks to me like we first tell the SCSI midlayer that
> the command is done, and *then* we go check whether the CRC is good).
> Also, there's still some code in there that seems to make assumptions
> about how TCP segments data - for instance, we will currently fail a
> reply if there are additional header segments that extend past the end
> of the current sk_buff. The current code also seems to make it hard to
> introduce things like fixed interval markers.
>
> The following set of patches introduce a new recv handling,
> and delete quite a bit of unused code. I tested this code against
> iscsitarget, with and without header/data digests. Any additional
> testing you may want to give this code is appreciated.
>
> The patch set is against the linux-2.6-iscsi.git tree
>
I broke these patches, but I would like to get them in to get some
testing soon, so I will rediff for you. Do not bother unless you have
some update or like rediffing patches :)
Mike Christie
Oh yeah, Boaz, I know I told you I would only do minor bug fixes until
the bidi stuff was done but I think this patch should help you in the
end since it adds proper support for things like ahs handling.
I know you are sort of sore about getting shoved around on linux-scsi so
I just wanted to give you some heads up that your patches are probably
going to get broken again :) Or maybe the bidi stuff will straighten out
really quickly and it will be me that has the fun rediffing and merging
fun :) I can also try to help on your rediffing so let me know how much
breakage there is and if you need any help.
Boaz Harrosh
No problems Mike! Thanks for the offer
I am so used to doing this by now that it takes me 10 minutes.
I have not looked at latest code, but it sounds very good.
ahs handling is a good part of my old patch. I have done
some codeing to move that to libiscsi.c, but have not finished.
If you already done that than it's grate. I'll only have to add the
varlen-cdb stuff.
Once bidi settles in kernel I will send the patches to iSCSI. Meanwhile
I am happy with out-of-tree patches over the iSCSI in kernel.
It looks like the last mail from James Bottomley makes things go
forward.
From what I've seen so far. All the changes are good for me and bidi
will be a smaller patch than before. Thank you for the grate work.
Sorry for not able to help testing new code more. You know two of the
three flavors of pNFS, over Objects, and over block devices, relay on
open-iscsi as implementation. The stability of this subsystem will be key
in positive adoption of pNFS. (Not to make you scared:-)
Boaz
Boaz Harrosh
> the bidi stuff was done but I think this patch should help you in the
> end since it adds proper support for things like ahs handling.
>
> I know you are sort of sore about getting shoved around on linux-scsi so
> I just wanted to give you some heads up that your patches are probably
> going to get broken again :) Or maybe the bidi stuff will straighten out
> really quickly and it will be me that has the fun rediffing and merging
> fun :) I can also try to help on your rediffing so let me know how much
> breakage there is and if you need any help.
>
Dir Mike.
What is the best place to get the latest most bleeding edge
development of open-iscsi kernel code? Is it still:
git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
iscsi branch or do you have another one. Also it looks like SVN kernel
work has some stuff that the iscsi branch does not have, but I might
be wrong about that.
Thanks for everything I have some time now and I want to go back to
the iscsi latest code and implement bidi+varlen the proper way, on top
of your changes.
Boaz
Mike Christie
> On 5/7/07, Mike Christie <mich...@cs.wisc.edu> wrote:
>> Mike Christie wrote:
>>
>> Oh yeah, Boaz, I know I told you I would only do minor bug fixes until
>> the bidi stuff was done but I think this patch should help you in the
>> end since it adds proper support for things like ahs handling.
>>
>> I know you are sort of sore about getting shoved around on linux-scsi so
>> I just wanted to give you some heads up that your patches are probably
>> going to get broken again :) Or maybe the bidi stuff will straighten out
>> really quickly and it will be me that has the fun rediffing and merging
>> fun :) I can also try to help on your rediffing so let me know how much
>> breakage there is and if you need any help.
>>
>
> Dir Mike.
> What is the best place to get the latest most bleeding edge
> development of open-iscsi kernel code? Is it still:
> git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
Yep, this is the place.
> iscsi branch or do you have another one. Also it looks like SVN kernel
> work has some stuff that the iscsi branch does not have, but I might
> be wrong about that.
They should be pretty much up to date. The only differences are
1. There were some extra whitespaces in svn. For the git tree I cleaned
them up so JamesB could git-am them.
2. The git tree was rebased against scsi-misc which has some netlink API
changes. SVN has not been updated for those changes yet since I have not
had time to make compat patches.
> Thanks for everything I have some time now and I want to go back to
> the iscsi latest code and implement bidi+varlen the proper way, on top
> of your changes.
>
I have not had a chance to rediff these patches. I had just started
testing them over the weekend. If you want you can rediff them for me or
wait for this weekend for me to do it.
Mike Christie
> Boaz Harrosh wrote:
>> On 5/7/07, Mike Christie <mich...@cs.wisc.edu> wrote:
>>> Mike Christie wrote:
>>>
>>> Oh yeah, Boaz, I know I told you I would only do minor bug fixes until
>>> the bidi stuff was done but I think this patch should help you in the
>>> end since it adds proper support for things like ahs handling.
>>>
>>> I know you are sort of sore about getting shoved around on linux-scsi so
>>> I just wanted to give you some heads up that your patches are probably
>>> going to get broken again :) Or maybe the bidi stuff will straighten out
>>> really quickly and it will be me that has the fun rediffing and merging
>>> fun :) I can also try to help on your rediffing so let me know how much
>>> breakage there is and if you need any help.
>>>
>> Dir Mike.
>> What is the best place to get the latest most bleeding edge
>> development of open-iscsi kernel code? Is it still:
>> git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
>
> Yep, this is the place.
>
Oh yeah make sure you get the iscsi branch in that tree.
# git checkout iscsi
Boaz Harrosh
week. I did the checkout iscsi and had a hard look at the code. I like
it better now. I want to more-less start from scratch (with reference
to old code) it should be more simple now, I think. We'll see how it
goes.
Boaz
Mike Christie
Hey Boaz, did you rediff this patchset and review it and test it? If you
rediffed it let me know. I have some time and will do it now. Also do
you know if you will be having any more bidi prep patches (will they be
large or smallish)?
Boaz Harrosh
libiscsi this time. So it is now supported at the generic level. The
rest in iscsi_tcp.c is Just to use the right bidi API for use in IN or
OUT code paths. Over all it should be a much easier job to maintain
the new patchset over time. (I have also broken iscsi bidi support to
a patchset) I will only release all this for review end of next week
together with SCSI-ml bidi support. (Not that anyone is waiting for
it)
I have only 2 more cleanups on top of iscsi branch. They will follow
in a mail of their own at the end of today.
1. Is a cleanup of Duplicate code at the error handling path.
2. Is the cleanup of !cmd->use_sg case. I know this can be a part of
Tomo's cleanup tree. But even than they are suppose to go threw
maintainers tree or ACK. And also iscsi is a special case, Because it
will not use the accessors that Tomo uses. The moment bidi support
goes into kernel and accessors are switched to new API iscsi will be
converted to bidi as first user of the new API. So we can go head and
just do the !use_sg case right now.
Thanks for your help
Boaz
Mike Christie
> This patch rewrites the TCP receive handling in open-iscsi.
>
> The basic idea of this patch is to replace the existing struct
> iscsi_tcp_recv with a different object that layers things a little
> better. The new struct has two callbacks - a recv callback for receiving
> data, and a done() callback that is invoked when data is complete and
> can be processed. The done() callback should not do any processing
> on the raw data, or mess with buffers and such - it gets called
> when all the data has arrived, and the CRC is fine.
>
> The main routine is iscsi_rcv, which just processes an incoming
> sk_buff (using the skb_seq_read interface), and repeatedly calls
> the current recv() routine, and when the chunk is complete, it calls
> done().
>
> There are two different recv handlers - one for plain copy, and one
> for copy+crc. Both transparently take care of padding, and walking
> a scatterlist (if that is the destination of the copy operation).
>
> There are three different sets of handlers for receiving data:
>
> - for the header
> - for data in replies
> - for any other command responses
>
> This draft patch does not remove any of the existing code, in order
> to keep the patch readable. A follow-up patch will delete any dead and
> unused code/variables.
>
Oh yeah Tomo and Boaz, I will also make any changes that are needed to
remove the use_sg=0 case.
Mike Christie
> One of the things that struck me while trying to understand the
> open-iscsi code was that the frame processing is heavily intertwined
> with the interpretation of the data; in particular, the digest handling
> crops up in lots of places (and I'm not even sure it's corrrect - in
> several places it looks to me like we first tell the SCSI midlayer that
> the command is done, and *then* we go check whether the CRC is good).
> Also, there's still some code in there that seems to make assumptions
> about how TCP segments data - for instance, we will currently fail a
> reply if there are additional header segments that extend past the end
> of the current sk_buff. The current code also seems to make it hard to
> introduce things like fixed interval markers.
>
> The following set of patches introduce a new recv handling,
> and delete quite a bit of unused code. I tested this code against
> iscsitarget, with and without header/data digests. Any additional
> testing you may want to give this code is appreciated.
>
> The patch set is against the linux-2.6-iscsi.git tree
>
> Olaf
I put this in the iscsi git tree so we could test it. I updated it for
the new sg accessors and did some other minor cleanup. It has a
compilation warning though:
drivers/scsi/iscsi_tcp.c: In function ‘iscsi_tcp_copy_to_chunk’:
drivers/scsi/iscsi_tcp.c:264: warning: comparison of distinct pointer
types lacks a cast
Thanks again for the work on this and sorry again for it taking so long
to get merged up.
Mike Christie
Oh yeah, I goofed and put these patches and the accessor ones in master
instead of iscsi on the kernel.org git tree.
Boaz Harrosh
I guess below fix is a leftover from the Merges
(I'm using gmail GUI I hope patch goes in well)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 3e4edab..e244ce6 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -1760,9 +1760,6 @@ iscsi_tcp_ctask_xmit(struct iscsi_conn *conn,
struct iscsi_cmd_task *ctask)
return rc;
rc = iscsi_send_sol_pdu(conn, ctask);
- if (rc)
- return rc;
-
return rc;
}
Mike Christie
>
> I guess below fix is a leftover from the Merges
> (I'm using gmail GUI I hope patch goes in well)
>
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 3e4edab..e244ce6 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -1760,9 +1760,6 @@ iscsi_tcp_ctask_xmit(struct iscsi_conn *conn,
> struct iscsi_cmd_task *ctask)
> return rc;
>
> rc = iscsi_send_sol_pdu(conn, ctask);
> - if (rc)
> - return rc;
> -
> return rc;
> }
Thanks. I just changed this to a
return scsi_send_sol_pdu(conn, ctask);
koshua
> This patch rewrites the TCP receive handling in open-iscsi.
Would this likely fix the hang I'm seeing during login whilst talking
to recent builds of opensolaris (e.g. nv_65)?
(An examination with wireshark indicates that the solaris iscsi target
is now sending padding in a separate segment; it wasn't in nv_59)
/k
Boaz Harrosh
Boaz
Mike Christie
I fixed the tree, so they should be the iscsi branch again.
Boaz Harrosh
Boaz Harrosh
Mike some small fixes to last patches maybe you can rework the
relevant patches instead of applying this one.
- min_t stuff should go into Olf's patches.
- scsi_bufflen into Tomo's cleanup
- and #undef DEBUG_TCP is ?
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index a23fc6f..90af469 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -48,7 +48,7 @@ MODULE_AUTHOR("Dmitry Yusupov <dmitr...@yahoo.com>, "
"Alex Aizman <itn...@yahoo.com>");
MODULE_DESCRIPTION("iSCSI/TCP data-path");
MODULE_LICENSE("GPL");
-#undef DEBUG_TCP
+
#define DEBUG_ASSERT
#ifdef DEBUG_TCP
@@ -261,7 +261,8 @@ iscsi_tcp_copy_to_chunk(struct iscsi_tcp_conn *tcp_conn,
unsigned int copy, copied = 0;
while (!iscsi_tcp_chunk_done(chunk) && copied < len) {
- copy = min(len - copied, chunk->size - chunk->copied);
+ copy = min_t(unsigned, len - copied,
+ chunk->size - chunk->copied);
memcpy(chunk->data + chunk->copied, ptr + copied, copy);
chunk->copied += copy;
@@ -299,7 +300,8 @@ iscsi_tcp_copy_and_crc_to_chunk(struct
iscsi_tcp_conn *tcp_conn,
if (copied == len)
goto out;
- copy = min(len - copied, chunk->size - chunk->copied);
+ copy = min_t(unsigned, len - copied,
+ chunk->size - chunk->copied);
memcpy(chunk->data + chunk->copied, ptr + copied, copy);
sg_init_one(&sg, ptr + copied, copy);
@@ -469,7 +471,7 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct
iscsi_cmd_task *ctask)
if (tcp_ctask->data_offset + tcp_conn->in.datalen > scsi_bufflen(sc)) {
debug_tcp("%s: data_offset(%d) + data_len(%d) > total_length_in(%d)\n",
__FUNCTION__, tcp_ctask->data_offset,
- tcp_conn->in.datalen, scsi_buffle(sc));
+ tcp_conn->in.datalen, scsi_bufflen(sc));
return ISCSI_ERR_DATA_OFFSET;
}
Olaf Kirch
> - and #undef DEBUG_TCP is ?
I just put it in there as a place-holder; it helps to
remind people that you can turn on debugging by changing
it to "#define DEBUG_TCP". Feel free to replace with
a comment saying "define DEBUG_TCP to turn on debugging
at the connection level" or some such.
FUJITA Tomonori
Subject: Re: [PATCH 0/3] Rewrite iSCSI TCP receive handling
Date: Tue, 12 Jun 2007 13:41:02 +0300
> @@ -469,7 +471,7 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct
> iscsi_cmd_task *ctask)
> if (tcp_ctask->data_offset + tcp_conn->in.datalen > scsi_bufflen(sc)) {
> debug_tcp("%s: data_offset(%d) + data_len(%d) > total_length_in(%d)\n",
> __FUNCTION__, tcp_ctask->data_offset,
> - tcp_conn->in.datalen, scsi_buffle(sc));
> + tcp_conn->in.datalen, scsi_bufflen(sc));
> return ISCSI_ERR_DATA_OFFSET;
Oops, I should have tested the patch with the debug option
enabled. Thanks.
Mike, are you ready to ack this patch? Then I'll send an updated patch
to scsi-ml.
Boaz Harrosh
Please review my posted patch to drivers/scsi/Makefile to enable debug
prints. I'm not sure it is the right way. But now that we are all
using git, we need something.
Thanks
Boaz
Olaf Kirch
> Please review my posted patch to drivers/scsi/Makefile to enable debug
> prints. I'm not sure it is the right way. But now that we are all
> using git, we need something.
Well, using a #define is the more common way to
do this (grep the kernel source for #undef.*DEBUG)
but I'm not religious about this. Your approach is fine
as well
Mike Christie
Yeah, I ran it through the regression script against iet and netapp and
it looks ok, so send it on. Feel free to add my signed off on it or ack
line or whatever.