All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>,
	target-devel@vger.kernel.org, Hannes Reinecke <hare@suse.com>,
	Christoph Hellwig <hch@lst.de>, Andy Grover <agrover@redhat.com>,
	David Disseldorp <ddiss@suse.de>,
	stable@vger.kernel.org
Subject: Re: [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands
Date: Fri, 5 May 2017 11:42:50 +0200	[thread overview]
Message-ID: <20170505094250.GA6138@lst.de> (raw)
In-Reply-To: <20170504225102.8931-7-bart.vanassche@sandisk.com>

Hi Bart,

what do you think about the variant below instead which avoids
overloading the size variable?

---
>From 206696ec37cf4f6efe093964c2bdc96100de1f62 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 5 May 2017 11:40:38 +0200
Subject: target: fix and cleanup size calculation in sbc_parse_cdb

Calculate the data buffer size individually for each command instead of
trying to generalize it.  This fixes the size calculation for VERIFY
and WRITE_VERIFY, while making the code a lot easier to understand.

Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/target_core_sbc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5807123214e5..0bd879b9ce38 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -899,12 +899,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 	switch (cdb[0]) {
 	case READ_6:
 		sectors = transport_get_sectors_6(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case READ_10:
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -919,6 +921,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case READ_12:
 		sectors = transport_get_sectors_12(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -933,6 +936,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case READ_16:
 		sectors = transport_get_sectors_16(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -947,12 +951,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case WRITE_6:
 		sectors = transport_get_sectors_6(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_10:
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -974,6 +980,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		goto check_lba;
 	case WRITE_12:
 		sectors = transport_get_sectors_12(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -988,6 +995,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case WRITE_16:
 		sectors = transport_get_sectors_16(cdb);
+		size = sbc_get_size(cmd, sectors);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
@@ -1005,6 +1013,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		    !(cmd->se_cmd_flags & SCF_BIDI))
 			return TCM_INVALID_CDB_FIELD;
 		sectors = transport_get_sectors_10(cdb);
+		size = sbc_get_size(cmd, sectors);
 
 		if (sbc_check_dpofua(dev, cmd, cdb))
 			return TCM_INVALID_CDB_FIELD;
@@ -1024,6 +1033,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		switch (service_action) {
 		case XDWRITEREAD_32:
 			sectors = transport_get_sectors_32(cdb);
+			size = sbc_get_size(cmd, sectors);
 
 			if (sbc_check_dpofua(dev, cmd, cdb))
 				return TCM_INVALID_CDB_FIELD;
@@ -1116,7 +1126,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			sectors = transport_get_sectors_16(cdb);
 			cmd->t_task_lba = transport_lba_64(cdb);
 		}
+	
+		/*
+		 * XXX: why treat sectors / size check differently for
+		 * the offload  / non-offload cases?
+		 */
 		if (ops->execute_sync_cache) {
+			size = sbc_get_size(cmd, sectors);
 			cmd->execute_cmd = ops->execute_sync_cache;
 			goto check_lba;
 		}
@@ -1205,9 +1221,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 				end_lba, cmd->t_task_lba, sectors);
 			return TCM_ADDRESS_OUT_OF_RANGE;
 		}
-
-		if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
-			size = sbc_get_size(cmd, sectors);
 	}
 
 	return target_cmd_size_check(cmd, size);
-- 
2.11.0

  reply	other threads:[~2017-05-05  9:42 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20170504225102.8931-1-bart.vanassche@sandisk.com>
2017-05-04 22:50 ` [PATCH 03/19] target: Avoid that aborting a command sporadically hangs Bart Van Assche
2017-05-05  6:12   ` Hannes Reinecke
2017-05-05  8:53   ` Christoph Hellwig
2017-05-05 15:00     ` Bart Van Assche
2017-05-11  0:23     ` Bart Van Assche
2017-05-07 22:20   ` Nicholas A. Bellinger
2017-05-08 21:25     ` Bart Van Assche
2017-05-10  4:48       ` Nicholas A. Bellinger
2017-05-04 22:50 ` [PATCH 04/19] target/fileio: Avoid that zero-length READ and WRITE commands hang Bart Van Assche
2017-05-05  6:14   ` Hannes Reinecke
2017-05-05  8:54   ` Christoph Hellwig
2017-05-07 22:28   ` Nicholas A. Bellinger
2017-05-04 22:50 ` [PATCH 05/19] target: Allocate sg-list correctly Bart Van Assche
2017-05-05  6:15   ` Hannes Reinecke
2017-05-05  9:06   ` Christoph Hellwig
2017-05-05 15:49     ` Bart Van Assche
2017-05-07 22:45   ` Nicholas A. Bellinger
2017-05-08 17:46     ` Bart Van Assche
2017-05-10  4:03       ` Nicholas A. Bellinger
2017-05-10  6:12         ` Nicholas A. Bellinger
2017-05-10 20:31         ` Bart Van Assche
2017-05-11  5:28           ` Nicholas A. Bellinger
2017-05-04 22:50 ` [PATCH 06/19] target: Fix data buffer size for VERIFY and WRITE AND VERIFY commands Bart Van Assche
2017-05-05  9:42   ` Christoph Hellwig [this message]
2017-05-05 15:51     ` Bart Van Assche
2017-05-07 22:49   ` Nicholas A. Bellinger
2017-05-08 18:07     ` Bart Van Assche
2017-05-10  4:28       ` Nicholas A. Bellinger
2017-05-10 15:16         ` Bart Van Assche
2017-05-11  5:09           ` Nicholas A. Bellinger
2017-05-04 22:51 ` [PATCH 17/19] target/iscsi: Simplify timer manipulation code Bart Van Assche
2017-05-05 11:24   ` Hannes Reinecke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170505094250.GA6138@lst.de \
    --to=hch@lst.de \
    --cc=agrover@redhat.com \
    --cc=bart.vanassche@sandisk.com \
    --cc=ddiss@suse.de \
    --cc=hare@suse.com \
    --cc=nab@linux-iscsi.org \
    --cc=stable@vger.kernel.org \
    --cc=target-devel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.