[PATCH 0/6] target: Reenable buffered FILEIO + add iscsi-target MXDSL logic

[PATCH 0/6] target: Reenable buffered FILEIO + add iscsi-target MXDSL logic

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi folks,

This following series is destined for v3.7-rc1 code, and re-adds support for
buffered FILEIO + emulate_write_cache=1 emulation removed in v3.5, as well as
fixing up a long standing issue wrt to MaxRecvDataSegmentLength handling within
iscsi-target code.

The latter bit is the bulk of the series that involves adding a proper
configurable MaxXmitDataSegmentLength TPG parameter configfs attr, and converts
iscsi-target login logic, incoming PDU payload checks, TMR TASK_REASSIGN ops, and
non contigious sequence+pdu operation to now honor MXDSL.

Open-iSCSI + Linux-iSCSI folks please review the MXDSL bits, and please let us
know if you have any concerns.

Thank you!

--nab

Nicholas Bellinger (6):
  target/file: Re-enable optional fd_buffered_io=1 operation
  iscsi-target: Add base MaxXmitDataSegmentLength code
  iscsi-target: Enable MaxXmitDataSegmentLength operation in login path
  iscsi-target: Convert incoming PDU payload checks to
    MaxXmitDataSegmentLength
  iscsi-target: Add MaxXmitDataSegmentLength connection recovery check
  iscsi-target: Change iscsi_target_seq_pdu_list.c to honor
    MaxXmitDataSegmentLength

 drivers/target/iscsi/iscsi_target.c              |   24 ++++----
 drivers/target/iscsi/iscsi_target_configfs.c     |    4 +
 drivers/target/iscsi/iscsi_target_core.h         |    2 +
 drivers/target/iscsi/iscsi_target_erl2.c         |    1 +
 drivers/target/iscsi/iscsi_target_nego.c         |    4 +-
 drivers/target/iscsi/iscsi_target_parameters.c   |   71 +++++++++++++++++++---
 drivers/target/iscsi/iscsi_target_parameters.h   |    7 ++-
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c |   61 ++++++++++---------
 drivers/target/iscsi/iscsi_target_tmr.c          |    9 +++-
 drivers/target/target_core_file.c                |   36 ++++++++++-
 drivers/target/target_core_file.h                |    1 +
 11 files changed, 162 insertions(+), 58 deletions(-)

-- 
1.7.2.5

[PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch re-adds the ability to optionally run in buffered FILEIO mode
(eg: w/o O_DSYNC) for device backends in order to once again use the
Linux buffered cache as a write-back storage mechanism.

This logic was originally dropped with mainline v3.5-rc commit:

commit a4dff3043c231d57f982af635c9d2192ee40e5ae
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date:   Wed May 30 16:25:41 2012 -0700

    target/file: Use O_DSYNC by default for FILEIO backends

This difference with this patch is that fd_create_virtdevice() now
forces the explicit setting of emulate_write_cache=1 when buffered FILEIO
operation has been enabled.

Reported-by: Ferry <iscsitmp@bananateam.nl>
Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_file.c |   36 +++++++++++++++++++++++++++++++++---
 drivers/target/target_core_file.h |    1 +
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 1111925..e79b7b9 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -125,6 +125,14 @@ static struct se_device *fd_create_virtdevice(
 	 * of pure timestamp updates.
 	 */
 	flags = O_RDWR | O_CREAT | O_LARGEFILE | O_DSYNC;
+	 /*
+	 * Optionally allow fd_buffered_io=1 to be enabled for people
+	 * who know what they are doing w/o O_DSYNC.
+	 */
+	if (fd_dev->fbd_flags & FDBD_USE_BUFFERED_IO) {
+		pr_debug("FILEIO: Disabling O_DSYNC, using buffered FILEIO\n");
+		flags &= ~O_DSYNC;
+	}
 
 	file = filp_open(fd_dev->fd_dev_name, flags, 0600);
 	if (IS_ERR(file)) {
@@ -188,6 +196,12 @@ static struct se_device *fd_create_virtdevice(
 	if (!dev)
 		goto fail;
 
+	if (fd_dev->fbd_flags & FDBD_USE_BUFFERED_IO) {
+		pr_debug("FILEIO: Forcing setting of emulate_write_cache=1"
+			" with FDBD_USE_BUFFERED_IO\n");
+		dev->se_sub_dev->se_dev_attrib.emulate_write_cache = 1;
+	}
+
 	fd_dev->fd_dev_id = fd_host->fd_host_dev_id_count++;
 	fd_dev->fd_queue_depth = dev->queue_depth;
 
@@ -407,6 +421,7 @@ enum {
 static match_table_t tokens = {
 	{Opt_fd_dev_name, "fd_dev_name=%s"},
 	{Opt_fd_dev_size, "fd_dev_size=%s"},
+	{Opt_fd_buffered_io, "fd_buffered_io=%d"},
 	{Opt_err, NULL}
 };
 
@@ -418,7 +433,7 @@ static ssize_t fd_set_configfs_dev_params(
 	struct fd_dev *fd_dev = se_dev->se_dev_su_ptr;
 	char *orig, *ptr, *arg_p, *opts;
 	substring_t args[MAX_OPT_ARGS];
-	int ret = 0, token;
+	int ret = 0, arg, token;
 
 	opts = kstrdup(page, GFP_KERNEL);
 	if (!opts)
@@ -459,6 +474,19 @@ static ssize_t fd_set_configfs_dev_params(
 					" bytes\n", fd_dev->fd_dev_size);
 			fd_dev->fbd_flags |= FBDF_HAS_SIZE;
 			break;
+		case Opt_fd_buffered_io:
+			match_int(args, &arg);
+			if (arg != 1) {
+				pr_err("bogus fd_buffered_io=%d value\n", arg);
+				ret = -EINVAL;
+				goto out;
+			}
+
+			pr_debug("FILEIO: Using buffered I/O"
+				" operations for struct fd_dev\n");
+
+			fd_dev->fbd_flags |= FDBD_USE_BUFFERED_IO;
+			break;
 		default:
 			break;
 		}
@@ -490,8 +518,10 @@ static ssize_t fd_show_configfs_dev_params(
 	ssize_t bl = 0;
 
 	bl = sprintf(b + bl, "TCM FILEIO ID: %u", fd_dev->fd_dev_id);
-	bl += sprintf(b + bl, "        File: %s  Size: %llu  Mode: O_DSYNC\n",
-		fd_dev->fd_dev_name, fd_dev->fd_dev_size);
+	bl += sprintf(b + bl, "        File: %s  Size: %llu  Mode: %s\n",
+		fd_dev->fd_dev_name, fd_dev->fd_dev_size,
+		(fd_dev->fbd_flags & FDBD_USE_BUFFERED_IO) ?
+		"Buffered" : "O_DSYNC");
 	return bl;
 }
 
diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h
index 70ce7fd..fbd59ef 100644
--- a/drivers/target/target_core_file.h
+++ b/drivers/target/target_core_file.h
@@ -14,6 +14,7 @@
 
 #define FBDF_HAS_PATH		0x01
 #define FBDF_HAS_SIZE		0x02
+#define FDBD_USE_BUFFERED_IO	0x04
 
 struct fd_dev {
 	u32		fbd_flags;
-- 
1.7.2.5

[PATCH 2/6] iscsi-target: Add base MaxXmitDataSegmentLength code

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch introduces a new per connection MaxXmitDataSegmentLength
parameter value used to represent the outgoing MaxRecvDataSegmentLength
that is actually sent over the wire during iSCSI login response back
to the initiator side.

It also adds a new MaxXmitDataSegmentLength configfs attribute to
represent this value within the existing TPG parameter group under
/sys/kernel/config/target/iscsi/$TARGETNAME/$TPGT/param/

Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Andy Grover <agrover@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_configfs.c   |    4 ++++
 drivers/target/iscsi/iscsi_target_core.h       |    1 +
 drivers/target/iscsi/iscsi_target_parameters.c |   21 +++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_parameters.h |    5 +++++
 4 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index f86833f..f491f96 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1034,6 +1034,9 @@ TPG_PARAM_ATTR(ImmediateData, S_IRUGO | S_IWUSR);
 DEF_TPG_PARAM(MaxRecvDataSegmentLength);
 TPG_PARAM_ATTR(MaxRecvDataSegmentLength, S_IRUGO | S_IWUSR);
 
+DEF_TPG_PARAM(MaxXmitDataSegmentLength);
+TPG_PARAM_ATTR(MaxXmitDataSegmentLength, S_IRUGO | S_IWUSR);
+
 DEF_TPG_PARAM(MaxBurstLength);
 TPG_PARAM_ATTR(MaxBurstLength, S_IRUGO | S_IWUSR);
 
@@ -1079,6 +1082,7 @@ static struct configfs_attribute *lio_target_tpg_param_attrs[] = {
 	&iscsi_tpg_param_InitialR2T.attr,
 	&iscsi_tpg_param_ImmediateData.attr,
 	&iscsi_tpg_param_MaxRecvDataSegmentLength.attr,
+	&iscsi_tpg_param_MaxXmitDataSegmentLength.attr,
 	&iscsi_tpg_param_MaxBurstLength.attr,
 	&iscsi_tpg_param_FirstBurstLength.attr,
 	&iscsi_tpg_param_DefaultTime2Wait.attr,
diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 8a908b2..b26611a 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -239,6 +239,7 @@ struct iscsi_conn_ops {
 	u8	HeaderDigest;			/* [0,1] == [None,CRC32C] */
 	u8	DataDigest;			/* [0,1] == [None,CRC32C] */
 	u32	MaxRecvDataSegmentLength;	/* [512..2**24-1] */
+	u32	MaxXmitDataSegmentLength;	/* [512..2**24-1] */
 	u8	OFMarker;			/* [0,1] == [No,Yes] */
 	u8	IFMarker;			/* [0,1] == [No,Yes] */
 	u32	OFMarkInt;			/* [1..65535] */
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 0c4760f..40864ee 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -334,6 +334,13 @@ int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 	if (!param)
 		goto out;
 
+	param = iscsi_set_default_param(pl, MAXXMITDATASEGMENTLENGTH,
+			INITIAL_MAXXMITDATASEGMENTLENGTH,
+			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
+			TYPERANGE_512_TO_16777215, USE_ALL);
+	if (!param)
+		goto out;
+
 	param = iscsi_set_default_param(pl, MAXRECVDATASEGMENTLENGTH,
 			INITIAL_MAXRECVDATASEGMENTLENGTH,
 			PHASE_OPERATIONAL, SCOPE_CONNECTION_ONLY, SENDER_BOTH,
@@ -467,6 +474,8 @@ int iscsi_set_keys_to_negotiate(
 			SET_PSTATE_NEGOTIATE(param);
 		} else if (!strcmp(param->name, MAXRECVDATASEGMENTLENGTH)) {
 			SET_PSTATE_NEGOTIATE(param);
+		} else if (!strcmp(param->name, MAXXMITDATASEGMENTLENGTH)) {
+			continue;
 		} else if (!strcmp(param->name, MAXBURSTLENGTH)) {
 			SET_PSTATE_NEGOTIATE(param);
 		} else if (!strcmp(param->name, FIRSTBURSTLENGTH)) {
@@ -1720,6 +1729,18 @@ void iscsi_set_connection_parameters(
 	pr_debug("---------------------------------------------------"
 			"---------------\n");
 	list_for_each_entry(param, &param_list->param_list, p_list) {
+		/*
+		 * Special case to set MAXXMITDATASEGMENTLENGTH from the
+		 * target requested MaxRecvDataSegmentLength, even though
+		 * this key is not sent over the wire.
+		 */
+		if (!strcmp(param->name, MAXXMITDATASEGMENTLENGTH)) {
+			ops->MaxXmitDataSegmentLength =
+				simple_strtoul(param->value, &tmpptr, 0);
+			pr_debug("MaxXmitDataSegmentLength:     %s\n",
+				param->value);
+		}
+
 		if (!IS_PSTATE_ACCEPTOR(param) && !IS_PSTATE_PROPOSER(param))
 			continue;
 		if (!strcmp(param->name, AUTHMETHOD)) {
diff --git a/drivers/target/iscsi/iscsi_target_parameters.h b/drivers/target/iscsi/iscsi_target_parameters.h
index 6a37fd6..77a28b5 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.h
+++ b/drivers/target/iscsi/iscsi_target_parameters.h
@@ -70,6 +70,7 @@ extern void iscsi_set_session_parameters(struct iscsi_sess_ops *,
 #define INITIALR2T			"InitialR2T"
 #define IMMEDIATEDATA			"ImmediateData"
 #define MAXRECVDATASEGMENTLENGTH	"MaxRecvDataSegmentLength"
+#define MAXXMITDATASEGMENTLENGTH	"MaxXmitDataSegmentLength"
 #define MAXBURSTLENGTH			"MaxBurstLength"
 #define FIRSTBURSTLENGTH		"FirstBurstLength"
 #define DEFAULTTIME2WAIT		"DefaultTime2Wait"
@@ -113,6 +114,10 @@ extern void iscsi_set_session_parameters(struct iscsi_sess_ops *,
 #define INITIAL_INITIALR2T			YES
 #define INITIAL_IMMEDIATEDATA			YES
 #define INITIAL_MAXRECVDATASEGMENTLENGTH	"8192"
+/*
+ * Match outgoing MXDSL default to incoming Open-iSCSI default
+ */
+#define INITIAL_MAXXMITDATASEGMENTLENGTH	"262144"
 #define INITIAL_MAXBURSTLENGTH			"262144"
 #define INITIAL_FIRSTBURSTLENGTH		"65536"
 #define INITIAL_DEFAULTTIME2WAIT		"2"
-- 
1.7.2.5

[PATCH 3/6] iscsi-target: Enable MaxXmitDataSegmentLength operation in login path

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch activates MaxXmitDataSegmentLength usage that performs the
following sequence of events:

- Once the incoming initiator's MAXRECVDATASEGMENTLENGTH key is detected
  within iscsi_check_acceptor_state(), save the requested MRDSL into
  conn->conn_ops->MaxRecvDataSegmentLength

- Next change the outgoing target's MaxRecvDataSegmenthLength key=value
  based upon the local TPG's MaxXmitDataSegmentLength attribute value.

- Change iscsi_set_connection_parameters() to skip the assignment of
  conn->conn_ops->MaxRecvDataSegmentLength, now setup within
  iscsi_check_acceptor_state()

Also update iscsi_decode_text_input() -> iscsi_check_acceptor_state()
code-path to accept struct iscsi_conn *.

Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Andy Grover <agrover@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_nego.c       |    4 +-
 drivers/target/iscsi/iscsi_target_parameters.c |   50 +++++++++++++++++++----
 drivers/target/iscsi/iscsi_target_parameters.h |    2 +-
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index a9a73ac..d08373f 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -550,7 +550,7 @@ static int iscsi_target_handle_csg_zero(
 			SENDER_INITIATOR|SENDER_RECEIVER,
 			login->req_buf,
 			payload_length,
-			conn->param_list);
+			conn);
 	if (ret < 0)
 		return -1;
 
@@ -627,7 +627,7 @@ static int iscsi_target_handle_csg_one(struct iscsi_conn *conn, struct iscsi_log
 			SENDER_INITIATOR|SENDER_RECEIVER,
 			login->req_buf,
 			payload_length,
-			conn->param_list);
+			conn);
 	if (ret < 0)
 		return -1;
 
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 40864ee..3678ff2 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -1065,7 +1065,8 @@ out:
 	return proposer_values;
 }
 
-static int iscsi_check_acceptor_state(struct iscsi_param *param, char *value)
+static int iscsi_check_acceptor_state(struct iscsi_param *param, char *value,
+				struct iscsi_conn *conn)
 {
 	u8 acceptor_boolean_value = 0, proposer_boolean_value = 0;
 	char *negoitated_value = NULL;
@@ -1140,8 +1141,35 @@ static int iscsi_check_acceptor_state(struct iscsi_param *param, char *value)
 				return -1;
 		}
 
-		if (!strcmp(param->name, MAXRECVDATASEGMENTLENGTH))
-			SET_PSTATE_REPLY_OPTIONAL(param);
+		if (!strcmp(param->name, MAXRECVDATASEGMENTLENGTH)) {
+			struct iscsi_param *param_mxdsl;
+			unsigned long long tmp;
+			int rc;
+
+			rc = strict_strtoull(param->value, 0, &tmp);
+			if (rc < 0)
+				return -1;
+
+			conn->conn_ops->MaxRecvDataSegmentLength = tmp;
+			pr_debug("Saving op->MaxRecvDataSegmentLength from"
+				" original initiator received value: %u\n",
+				conn->conn_ops->MaxRecvDataSegmentLength);
+
+			param_mxdsl = iscsi_find_param_from_key(
+						MAXXMITDATASEGMENTLENGTH,
+						conn->param_list);
+			if (!param_mxdsl)
+				return -1;
+
+			rc = iscsi_update_param_value(param,
+						param_mxdsl->value);
+			if (rc < 0)
+				return -1;
+
+			pr_debug("Updated %s to target MXDSL value: %s\n",
+					param->name, param->value);
+		}
+
 	} else if (IS_TYPE_NUMBER_RANGE(param)) {
 		negoitated_value = iscsi_get_value_from_number_range(
 					param, value);
@@ -1535,8 +1563,9 @@ int iscsi_decode_text_input(
 	u8 sender,
 	char *textbuf,
 	u32 length,
-	struct iscsi_param_list *param_list)
+	struct iscsi_conn *conn)
 {
+	struct iscsi_param_list *param_list = conn->param_list;
 	char *tmpbuf, *start = NULL, *end = NULL;
 
 	tmpbuf = kzalloc(length + 1, GFP_KERNEL);
@@ -1594,7 +1623,7 @@ int iscsi_decode_text_input(
 			}
 			SET_PSTATE_RESPONSE_GOT(param);
 		} else {
-			if (iscsi_check_acceptor_state(param, value) < 0) {
+			if (iscsi_check_acceptor_state(param, value, conn) < 0) {
 				kfree(tmpbuf);
 				return -1;
 			}
@@ -1755,10 +1784,13 @@ void iscsi_set_connection_parameters(
 			pr_debug("DataDigest:                   %s\n",
 				param->value);
 		} else if (!strcmp(param->name, MAXRECVDATASEGMENTLENGTH)) {
-			ops->MaxRecvDataSegmentLength =
-				simple_strtoul(param->value, &tmpptr, 0);
-			pr_debug("MaxRecvDataSegmentLength:     %s\n",
-				param->value);
+			/*
+			 * At this point iscsi_check_acceptor_state() will have
+			 * set ops->MaxRecvDataSegmentLength from the original
+			 * initiator provided value.
+			 */
+			pr_debug("MaxRecvDataSegmentLength:     %u\n",
+				ops->MaxRecvDataSegmentLength);
 		} else if (!strcmp(param->name, OFMARKER)) {
 			ops->OFMarker = !strcmp(param->value, YES);
 			pr_debug("OFMarker:                     %s\n",
diff --git a/drivers/target/iscsi/iscsi_target_parameters.h b/drivers/target/iscsi/iscsi_target_parameters.h
index 77a28b5..1e1b750 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.h
+++ b/drivers/target/iscsi/iscsi_target_parameters.h
@@ -36,7 +36,7 @@ extern void iscsi_release_param_list(struct iscsi_param_list *);
 extern struct iscsi_param *iscsi_find_param_from_key(char *, struct iscsi_param_list *);
 extern int iscsi_extract_key_value(char *, char **, char **);
 extern int iscsi_update_param_value(struct iscsi_param *, char *);
-extern int iscsi_decode_text_input(u8, u8, char *, u32, struct iscsi_param_list *);
+extern int iscsi_decode_text_input(u8, u8, char *, u32, struct iscsi_conn *);
 extern int iscsi_encode_text_output(u8, u8, char *, u32 *,
 			struct iscsi_param_list *);
 extern int iscsi_check_negotiated_keys(struct iscsi_param_list *);
-- 
1.7.2.5

[PATCH 4/6] iscsi-target: Convert incoming PDU payload checks to MaxXmitDataSegmentLength

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Now that iscsi-target supports a local configurable MaxXmitDataSegmentLength,
go ahead and make ISCSI_OP_SCSI_CMD, ISCSI_OP_SCSI_DATA_OUT, ISCSI_OP_NOOP_OUT
and ISCSI_OP_TEXT PDU payload checks honor conn_ops->MaxXmitDataSegmentLength.

Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Andy Grover <agrover@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 30842e1..632a5ae 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -869,10 +869,10 @@ done:
 				buf, conn);
 	}
 
-	if (payload_length > conn->conn_ops->MaxRecvDataSegmentLength) {
+	if (payload_length > conn->conn_ops->MaxXmitDataSegmentLength) {
 		pr_err("DataSegmentLength: %u is greater than"
-			" MaxRecvDataSegmentLength: %u, protocol error.\n",
-			payload_length, conn->conn_ops->MaxRecvDataSegmentLength);
+			" MaxXmitDataSegmentLength: %u, protocol error.\n",
+			payload_length, conn->conn_ops->MaxXmitDataSegmentLength);
 		return iscsit_add_reject(ISCSI_REASON_PROTOCOL_ERROR, 1,
 				buf, conn);
 	}
@@ -1216,10 +1216,10 @@ static int iscsit_handle_data_out(struct iscsi_conn *conn, unsigned char *buf)
 	}
 	spin_unlock_bh(&conn->sess->session_stats_lock);
 
-	if (payload_length > conn->conn_ops->MaxRecvDataSegmentLength) {
+	if (payload_length > conn->conn_ops->MaxXmitDataSegmentLength) {
 		pr_err("DataSegmentLength: %u is greater than"
-			" MaxRecvDataSegmentLength: %u\n", payload_length,
-			conn->conn_ops->MaxRecvDataSegmentLength);
+			" MaxXmitDataSegmentLength: %u\n", payload_length,
+			conn->conn_ops->MaxXmitDataSegmentLength);
 		return iscsit_add_reject(ISCSI_REASON_PROTOCOL_ERROR, 1,
 					buf, conn);
 	}
@@ -1437,11 +1437,11 @@ static int iscsit_handle_nop_out(
 					buf, conn);
 	}
 
-	if (payload_length > conn->conn_ops->MaxRecvDataSegmentLength) {
+	if (payload_length > conn->conn_ops->MaxXmitDataSegmentLength) {
 		pr_err("NOPOUT Ping Data DataSegmentLength: %u is"
-			" greater than MaxRecvDataSegmentLength: %u, protocol"
+			" greater than MaxXmitDataSegmentLength: %u, protocol"
 			" error.\n", payload_length,
-			conn->conn_ops->MaxRecvDataSegmentLength);
+			conn->conn_ops->MaxXmitDataSegmentLength);
 		return iscsit_add_reject(ISCSI_REASON_PROTOCOL_ERROR, 1,
 					buf, conn);
 	}
@@ -1874,10 +1874,10 @@ static int iscsit_handle_text_cmd(
 	hdr->cmdsn		= be32_to_cpu(hdr->cmdsn);
 	hdr->exp_statsn		= be32_to_cpu(hdr->exp_statsn);
 
-	if (payload_length > conn->conn_ops->MaxRecvDataSegmentLength) {
+	if (payload_length > conn->conn_ops->MaxXmitDataSegmentLength) {
 		pr_err("Unable to accept text parameter length: %u"
-			"greater than MaxRecvDataSegmentLength %u.\n",
-		       payload_length, conn->conn_ops->MaxRecvDataSegmentLength);
+			"greater than MaxXmitDataSegmentLength %u.\n",
+		       payload_length, conn->conn_ops->MaxXmitDataSegmentLength);
 		return iscsit_add_reject(ISCSI_REASON_PROTOCOL_ERROR, 1,
 					buf, conn);
 	}
-- 
1.7.2.5

[PATCH 5/6] iscsi-target: Add MaxXmitDataSegmentLength connection recovery check

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

The iSCSI TMR TASK_REASSIGN completion logic in iscsi_tmr_task_reassign()
does an explict check for MRDSL across task reassignment, so go ahead and
add an explict MaxXmitDataSegmentLength check here as well to be safe.

Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Andy Grover <agrover@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_core.h |    1 +
 drivers/target/iscsi/iscsi_target_erl2.c |    1 +
 drivers/target/iscsi/iscsi_target_tmr.c  |    9 ++++++++-
 3 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index b26611a..e6a8305 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -579,6 +579,7 @@ struct iscsi_conn_recovery {
 	u16			cid;
 	u32			cmd_count;
 	u32			maxrecvdatasegmentlength;
+	u32			maxxmitdatasegmentlength;
 	int			ready_for_reallegiance;
 	struct list_head	conn_recovery_cmd_list;
 	spinlock_t		conn_recovery_cmd_lock;
diff --git a/drivers/target/iscsi/iscsi_target_erl2.c b/drivers/target/iscsi/iscsi_target_erl2.c
index 65aac14..667c19c 100644
--- a/drivers/target/iscsi/iscsi_target_erl2.c
+++ b/drivers/target/iscsi/iscsi_target_erl2.c
@@ -421,6 +421,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn *conn)
 	cr->cid = conn->cid;
 	cr->cmd_count = cmd_count;
 	cr->maxrecvdatasegmentlength = conn->conn_ops->MaxRecvDataSegmentLength;
+	cr->maxxmitdatasegmentlength = conn->conn_ops->MaxXmitDataSegmentLength;
 	cr->sess = conn->sess;
 
 	iscsit_attach_inactive_connection_recovery_entry(conn->sess, cr);
diff --git a/drivers/target/iscsi/iscsi_target_tmr.c b/drivers/target/iscsi/iscsi_target_tmr.c
index f62fe12..05d2e64 100644
--- a/drivers/target/iscsi/iscsi_target_tmr.c
+++ b/drivers/target/iscsi/iscsi_target_tmr.c
@@ -146,7 +146,7 @@ u8 iscsit_tmr_task_reassign(
 	}
 	/*
 	 * Temporary check to prevent connection recovery for
-	 * connections with a differing MaxRecvDataSegmentLength.
+	 * connections with a differing Max*DataSegmentLength.
 	 */
 	if (cr->maxrecvdatasegmentlength !=
 	    conn->conn_ops->MaxRecvDataSegmentLength) {
@@ -155,6 +155,13 @@ u8 iscsit_tmr_task_reassign(
 			" TMR TASK_REASSIGN.\n");
 		return ISCSI_TMF_RSP_REJECTED;
 	}
+	if (cr->maxxmitdatasegmentlength !=
+	    conn->conn_ops->MaxXmitDataSegmentLength) {
+		pr_err("Unable to perform connection recovery for"
+			" differing MaxXmitDataSegmentLength, rejecting"
+			" TMR TASK_REASSIGN.\n");
+		return ISCSI_TMF_RSP_REJECTED;
+	}
 
 	ref_lun = scsilun_to_int(&hdr->lun);
 	if (ref_lun != ref_cmd->se_cmd.orig_fe_lun) {
-- 
1.7.2.5

[PATCH 6/6] iscsi-target: Change iscsi_target_seq_pdu_list.c to honor MaxXmitDataSegmentLength

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts iscsi_target_seq_pdu_list.c code for DataSequenceInOrder=No +
DataPDUInOrder=No operation to honor the MaxXmitDataSegmentLength value
for iscsi_cmd->se_cmd.data_direction == DMA_TO_DEVICE cases.

Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Andy Grover <agrover@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c |   61 +++++++++++-----------
 1 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_seq_pdu_list.c b/drivers/target/iscsi/iscsi_target_seq_pdu_list.c
index 85a306e..edb592a 100644
--- a/drivers/target/iscsi/iscsi_target_seq_pdu_list.c
+++ b/drivers/target/iscsi/iscsi_target_seq_pdu_list.c
@@ -219,8 +219,14 @@ static void iscsit_determine_counts_for_list(
 	int check_immediate = 0;
 	u32 burstlength = 0, offset = 0;
 	u32 unsolicited_data_length = 0;
+	u32 mdsl;
 	struct iscsi_conn *conn = cmd->conn;
 
+	if (cmd->se_cmd.data_direction == DMA_TO_DEVICE)
+		mdsl = cmd->conn->conn_ops->MaxXmitDataSegmentLength;
+	else
+		mdsl = cmd->conn->conn_ops->MaxRecvDataSegmentLength;
+
 	if ((bl->type == PDULIST_IMMEDIATE) ||
 	    (bl->type == PDULIST_IMMEDIATE_AND_UNSOLICITED))
 		check_immediate = 1;
@@ -243,14 +249,13 @@ static void iscsit_determine_counts_for_list(
 			continue;
 		}
 		if (unsolicited_data_length > 0) {
-			if ((offset + conn->conn_ops->MaxRecvDataSegmentLength)
-					>= cmd->se_cmd.data_length) {
+			if ((offset + mdsl) >= cmd->se_cmd.data_length) {
 				unsolicited_data_length -=
 					(cmd->se_cmd.data_length - offset);
 				offset += (cmd->se_cmd.data_length - offset);
 				continue;
 			}
-			if ((offset + conn->conn_ops->MaxRecvDataSegmentLength)
+			if ((offset + mdsl)
 					>= conn->sess->sess_ops->FirstBurstLength) {
 				unsolicited_data_length -=
 					(conn->sess->sess_ops->FirstBurstLength -
@@ -262,17 +267,15 @@ static void iscsit_determine_counts_for_list(
 				continue;
 			}
 
-			offset += conn->conn_ops->MaxRecvDataSegmentLength;
-			unsolicited_data_length -=
-				conn->conn_ops->MaxRecvDataSegmentLength;
+			offset += mdsl;
+			unsolicited_data_length -= mdsl;
 			continue;
 		}
-		if ((offset + conn->conn_ops->MaxRecvDataSegmentLength) >=
-		     cmd->se_cmd.data_length) {
+		if ((offset + mdsl) >= cmd->se_cmd.data_length) {
 			offset += (cmd->se_cmd.data_length - offset);
 			continue;
 		}
-		if ((burstlength + conn->conn_ops->MaxRecvDataSegmentLength) >=
+		if ((burstlength + mdsl) >=
 		     conn->sess->sess_ops->MaxBurstLength) {
 			offset += (conn->sess->sess_ops->MaxBurstLength -
 					burstlength);
@@ -281,8 +284,8 @@ static void iscsit_determine_counts_for_list(
 			continue;
 		}
 
-		burstlength += conn->conn_ops->MaxRecvDataSegmentLength;
-		offset += conn->conn_ops->MaxRecvDataSegmentLength;
+		burstlength += mdsl;
+		offset += mdsl;
 	}
 }
 
@@ -296,12 +299,17 @@ static int iscsit_do_build_pdu_and_seq_lists(
 	struct iscsi_build_list *bl)
 {
 	int check_immediate = 0, datapduinorder, datasequenceinorder;
-	u32 burstlength = 0, offset = 0, i = 0;
+	u32 burstlength = 0, offset = 0, i = 0, mdsl;
 	u32 pdu_count = 0, seq_no = 0, unsolicited_data_length = 0;
 	struct iscsi_conn *conn = cmd->conn;
 	struct iscsi_pdu *pdu = cmd->pdu_list;
 	struct iscsi_seq *seq = cmd->seq_list;
 
+	if (cmd->se_cmd.data_direction == DMA_TO_DEVICE)
+		mdsl = cmd->conn->conn_ops->MaxXmitDataSegmentLength;
+	else
+		mdsl = cmd->conn->conn_ops->MaxRecvDataSegmentLength;
+
 	datapduinorder = conn->sess->sess_ops->DataPDUInOrder;
 	datasequenceinorder = conn->sess->sess_ops->DataSequenceInOrder;
 
@@ -348,9 +356,7 @@ static int iscsit_do_build_pdu_and_seq_lists(
 			continue;
 		}
 		if (unsolicited_data_length > 0) {
-			if ((offset +
-			     conn->conn_ops->MaxRecvDataSegmentLength) >=
-			     cmd->se_cmd.data_length) {
+			if ((offset + mdsl) >= cmd->se_cmd.data_length) {
 				if (!datapduinorder) {
 					pdu[i].type = PDUTYPE_UNSOLICITED;
 					pdu[i].length =
@@ -367,8 +373,7 @@ static int iscsit_do_build_pdu_and_seq_lists(
 				offset += (cmd->se_cmd.data_length - offset);
 				continue;
 			}
-			if ((offset +
-			     conn->conn_ops->MaxRecvDataSegmentLength) >=
+			if ((offset + mdsl) >=
 					conn->sess->sess_ops->FirstBurstLength) {
 				if (!datapduinorder) {
 					pdu[i].type = PDUTYPE_UNSOLICITED;
@@ -396,17 +401,14 @@ static int iscsit_do_build_pdu_and_seq_lists(
 
 			if (!datapduinorder) {
 				pdu[i].type = PDUTYPE_UNSOLICITED;
-				pdu[i++].length =
-				     conn->conn_ops->MaxRecvDataSegmentLength;
+				pdu[i++].length = mdsl;
 			}
-			burstlength += conn->conn_ops->MaxRecvDataSegmentLength;
-			offset += conn->conn_ops->MaxRecvDataSegmentLength;
-			unsolicited_data_length -=
-				conn->conn_ops->MaxRecvDataSegmentLength;
+			burstlength += mdsl;
+			offset += mdsl;
+			unsolicited_data_length -= mdsl;
 			continue;
 		}
-		if ((offset + conn->conn_ops->MaxRecvDataSegmentLength) >=
-		     cmd->se_cmd.data_length) {
+		if ((offset + mdsl) >= cmd->se_cmd.data_length) {
 			if (!datapduinorder) {
 				pdu[i].type = PDUTYPE_NORMAL;
 				pdu[i].length = (cmd->se_cmd.data_length - offset);
@@ -420,7 +422,7 @@ static int iscsit_do_build_pdu_and_seq_lists(
 			offset += (cmd->se_cmd.data_length - offset);
 			continue;
 		}
-		if ((burstlength + conn->conn_ops->MaxRecvDataSegmentLength) >=
+		if ((burstlength + mdsl) >=
 		     conn->sess->sess_ops->MaxBurstLength) {
 			if (!datapduinorder) {
 				pdu[i].type = PDUTYPE_NORMAL;
@@ -445,11 +447,10 @@ static int iscsit_do_build_pdu_and_seq_lists(
 
 		if (!datapduinorder) {
 			pdu[i].type = PDUTYPE_NORMAL;
-			pdu[i++].length =
-				conn->conn_ops->MaxRecvDataSegmentLength;
+			pdu[i++].length = mdsl;
 		}
-		burstlength += conn->conn_ops->MaxRecvDataSegmentLength;
-		offset += conn->conn_ops->MaxRecvDataSegmentLength;
+		burstlength += mdsl;
+		offset += mdsl;
 	}
 
 	if (!datasequenceinorder) {
-- 
1.7.2.5

Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation

[Christoph Hellwig]

On Sun, Sep 30, 2012 at 05:58:11AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch re-adds the ability to optionally run in buffered FILEIO mode
> (eg: w/o O_DSYNC) for device backends in order to once again use the
> Linux buffered cache as a write-back storage mechanism.
> 
> This difference with this patch is that fd_create_virtdevice() now
> forces the explicit setting of emulate_write_cache=1 when buffered FILEIO
> operation has been enabled.

What this lacks is a clear reason why you would enable this inherently
unsafe mode.  While there is some clear precedence to allow people doing
stupid thing I'd least like a rationale for it, and it being documented
as unsafe.

> +	 /*

Indention error.

> +	 * Optionally allow fd_buffered_io=1 to be enabled for people
> +	 * who know what they are doing w/o O_DSYNC.
> +	 */

This comment doesn't explain at all what's going on here.  It should be
something like

	/*
	 * Unsafe mode allows disabling data integrity by not forcing
	 * data out to disk in writethrough cache mode.  Only to be used
	 * for benchmark cheating or similar purposes.
	 */

>  #define FBDF_HAS_PATH		0x01
>  #define FBDF_HAS_SIZE		0x02
> +#define FDBD_USE_BUFFERED_IO	0x04

This should be named BDBD_UNSAFE

Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation

[Vladislav Bolkhovitin]

Christoph Hellwig, on 10/01/2012 04:46 AM wrote:
> On Sun, Sep 30, 2012 at 05:58:11AM +0000, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger<nab@linux-iscsi.org>
>>
>> This patch re-adds the ability to optionally run in buffered FILEIO mode
>> (eg: w/o O_DSYNC) for device backends in order to once again use the
>> Linux buffered cache as a write-back storage mechanism.
>>
>> This difference with this patch is that fd_create_virtdevice() now
>> forces the explicit setting of emulate_write_cache=1 when buffered FILEIO
>> operation has been enabled.
>
> What this lacks is a clear reason why you would enable this inherently
> unsafe mode.  While there is some clear precedence to allow people doing
> stupid thing I'd least like a rationale for it, and it being documented
> as unsafe.

Nowadays nearly all serious applications are transactional, and know how to flush 
storage cache between transactions. That means that write back caching is 
absolutely safe for them. No data can't be lost in any circumstances.

Welcome to the 21 century, Christoph!

Vlad

Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation

[Nicholas A. Bellinger]

On Mon, 2012-10-01 at 04:46 -0400, Christoph Hellwig wrote:
> On Sun, Sep 30, 2012 at 05:58:11AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch re-adds the ability to optionally run in buffered FILEIO mode
> > (eg: w/o O_DSYNC) for device backends in order to once again use the
> > Linux buffered cache as a write-back storage mechanism.
> > 
> > This difference with this patch is that fd_create_virtdevice() now
> > forces the explicit setting of emulate_write_cache=1 when buffered FILEIO
> > operation has been enabled.
> 
> What this lacks is a clear reason why you would enable this inherently
> unsafe mode.  While there is some clear precedence to allow people doing
> stupid thing I'd least like a rationale for it, and it being documented
> as unsafe.
> 
> > +	 /*
> 
> Indention error.
> 

Fixed

> > +	 * Optionally allow fd_buffered_io=1 to be enabled for people
> > +	 * who know what they are doing w/o O_DSYNC.
> > +	 */
> 
> This comment doesn't explain at all what's going on here.  It should be
> something like
> 
> 	/*
> 	 * Unsafe mode allows disabling data integrity by not forcing
> 	 * data out to disk in writethrough cache mode.  Only to be used
> 	 * for benchmark cheating or similar purposes.
> 	 */
> 

How about the following instead..?

        /*
         * Optionally allow fd_buffered_io=1 to be enabled for people
         * who want use the fs buffer cache as an WriteCache mechanism.
         *
         * This means that in event of a hard failure, there is a risk
         * of silent data-loss if the SCSI client has *not* performed a
         * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE
         * to write-out the entire device cache.
         */


> >  #define FBDF_HAS_PATH		0x01
> >  #define FBDF_HAS_SIZE		0x02
> > +#define FDBD_USE_BUFFERED_IO	0x04
> 
> This should be named BDBD_UNSAFE
> 

As we are keeping the same fd_buffered_io key=value usage that rtslib
user-space already knows about, how using FDBD_HAS_BUFFERED_IO_WCE
instead for consistency..?

Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation

[Christoph Hellwig]

On Tue, Oct 02, 2012 at 01:16:44PM -0700, Nicholas A. Bellinger wrote:
>          * Optionally allow fd_buffered_io=1 to be enabled for people
>          * who want use the fs buffer cache as an WriteCache mechanism.
>          *
>          * This means that in event of a hard failure, there is a risk
>          * of silent data-loss if the SCSI client has *not* performed a
>          * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE
>          * to write-out the entire device cache.
>          */

Oh, I get Vlads flame.  This doesn't simply disable O_DSYNC now but also
sets WCE=1.  In this case I don't really get the point of the patch, why
can't we simply set it from configfs?

Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation

[Nicholas A. Bellinger]

On Wed, 2012-10-03 at 07:47 -0400, Christoph Hellwig wrote:
> On Tue, Oct 02, 2012 at 01:16:44PM -0700, Nicholas A. Bellinger wrote:
> >          * Optionally allow fd_buffered_io=1 to be enabled for people
> >          * who want use the fs buffer cache as an WriteCache mechanism.
> >          *
> >          * This means that in event of a hard failure, there is a risk
> >          * of silent data-loss if the SCSI client has *not* performed a
> >          * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE
> >          * to write-out the entire device cache.
> >          */
> 
> Oh, I get Vlads flame.  This doesn't simply disable O_DSYNC now but also
> sets WCE=1.  In this case I don't really get the point of the patch, why
> can't we simply set it from configfs?
> 

The patch prevents existing user-space code from using fd_buffered_io=1
operation incorrectly regardless of what is being set for
emulate_write_cache device attribute.

Using FILEIO w/o O_DSYNC + emulate_write_cache=0 is certainly a big
no-no regardless of context, so I'd rather enforce the right thing to do
here from kernel code for this special case instead of allowing an
unsafe mode to be enabled by default + set attributes from user-space.

--nab