[PATCH 0/5] target: Zoned block device support and bug fixes

[PATCH 0/5] target: Zoned block device support and bug fixes

[Damien Le Moal]

This series introduce zoned block device support for the pscsi backstore and
also fixes several problems with sense data handling for failed requests.

The first patch is only a cleanup, so not really necessary but nice to have I
think.

Patch 2 and 3 introduce support for host managed zoned block device type (14h)
in the SCSI passthrough backstore and fixes sense data handling for commands
failed by the backstore device. With these fixes, a host zoned block device
exported with the iscsi or loopback transport pass libzbc ZBC specification
conformance tests.

Finally, patch 4 and 5 fix sense data hadling with the user backstore code. A
prototype ZBC emulation tcmu-runner handler was used to test these fixes and
result in the emulated handler passing libzbc ZBC specification conformance
tests.

(Note: the ZBC emulation tcmu-runner handler will be submitted to the
tcmu-runner project on github)

Please consider these patches for inclusion with kernel 4.13.

Damien Le Moal (5):
  target: Use macro for WRITE_VERIFY_xx operation codes
  target: pscsi: Introduce TYPE_ZBC support
  target: pscsi: Fix sense data handling
  target: user: Fix sense data handling
  target: core: Fix failed command sense data handling

 drivers/target/target_core_device.c    |  4 ++--
 drivers/target/target_core_pscsi.c     | 20 +++++++++++++-------
 drivers/target/target_core_transport.c |  5 +++--
 drivers/target/target_core_user.c      |  4 +++-
 include/scsi/scsi_proto.h              |  1 +
 5 files changed, 22 insertions(+), 12 deletions(-)

-- 
2.9.4

[PATCH 1/5] target: Use macro for WRITE_VERIFY_xx operation codes

[Damien Le Moal]

Add WRITE_VERIFY_32 definition to scsi prototypes and use this macro
definition isntead of the hard coded value. Same for the already defined
WRITE_VERIFY_16 command code.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/target/target_core_device.c | 4 ++--
 include/scsi/scsi_proto.h           | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 8add07f38..15dcbb9 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -1126,7 +1126,7 @@ passthrough_parse_cdb(struct se_cmd *cmd,
 	case WRITE_16:
 	case WRITE_VERIFY:
 	case WRITE_VERIFY_12:
-	case 0x8e: /* WRITE_VERIFY_16 */
+	case WRITE_VERIFY_16:
 	case COMPARE_AND_WRITE:
 	case XDWRITEREAD_10:
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
@@ -1135,7 +1135,7 @@ passthrough_parse_cdb(struct se_cmd *cmd,
 		switch (get_unaligned_be16(&cdb[8])) {
 		case READ_32:
 		case WRITE_32:
-		case 0x0c: /* WRITE_VERIFY_32 */
+		case WRITE_VERIFY_32:
 		case XDWRITEREAD_32:
 			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 			break;
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index ce78ec8..5e3fe03 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -161,6 +161,7 @@
 #define READ_32		      0x09
 #define VERIFY_32	      0x0a
 #define WRITE_32	      0x0b
+#define WRITE_VERIFY_32	      0x0c
 #define WRITE_SAME_32	      0x0d
 
 /* Values for T10/04-262r7 */
-- 
2.9.4

Re: [PATCH 1/5] target: Use macro for WRITE_VERIFY_xx operation codes

[Nicholas A. Bellinger]

Hi Damien,

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> Add WRITE_VERIFY_32 definition to scsi prototypes and use this macro
> definition isntead of the hard coded value. Same for the already defined
> WRITE_VERIFY_16 command code.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_device.c | 4 ++--
>  include/scsi/scsi_proto.h           | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)

Note the WRITE_VERIFY_16 macro usage has already been changed by
commit 500073bb23 in target-pending/for-next.

So applied to target-pending/for-next for the WRITE_VERIFY_32 specific
bits.

[PATCH 2/5] target: pscsi: Introduce TYPE_ZBC support

[Damien Le Moal]

TYPE_ZBC host managed zoned block devices are also block devices
despite the non-standard device type (14h). Handle them similarly to
regular TYPE_DISK devices.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/target/target_core_pscsi.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 3e4abb1..97d0318 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -384,7 +384,7 @@ static int pscsi_create_type_disk(struct se_device *dev, struct scsi_device *sd)
 	spin_unlock_irq(sh->host_lock);
 	/*
 	 * Claim exclusive struct block_device access to struct scsi_device
-	 * for TYPE_DISK using supplied udev_path
+	 * for TYPE_DISK and TYPE_ZBC using supplied udev_path
 	 */
 	bd = blkdev_get_by_path(dev->udev_path,
 				FMODE_WRITE|FMODE_READ|FMODE_EXCL, pdv);
@@ -402,8 +402,9 @@ static int pscsi_create_type_disk(struct se_device *dev, struct scsi_device *sd)
 		return ret;
 	}
 
-	pr_debug("CORE_PSCSI[%d] - Added TYPE_DISK for %d:%d:%d:%llu\n",
-		phv->phv_host_id, sh->host_no, sd->channel, sd->id, sd->lun);
+	pr_debug("CORE_PSCSI[%d] - Added TYPE_%s for %d:%d:%d:%llu\n",
+		phv->phv_host_id, sd->type == TYPE_DISK ? "DISK" : "ZBC",
+		sh->host_no, sd->channel, sd->id, sd->lun);
 	return 0;
 }
 
@@ -522,6 +523,7 @@ static int pscsi_configure_device(struct se_device *dev)
 		 */
 		switch (sd->type) {
 		case TYPE_DISK:
+		case TYPE_ZBC:
 			ret = pscsi_create_type_disk(dev, sd);
 			break;
 		default:
@@ -573,9 +575,11 @@ static void pscsi_free_device(struct se_device *dev)
 	if (sd) {
 		/*
 		 * Release exclusive pSCSI internal struct block_device claim for
-		 * struct scsi_device with TYPE_DISK from pscsi_create_type_disk()
+		 * struct scsi_device with TYPE_DISK or TYPE_ZBC
+		 * from pscsi_create_type_disk()
 		 */
-		if ((sd->type == TYPE_DISK) && pdv->pdv_bd) {
+		if ((sd->type == TYPE_DISK || sd->type == TYPE_ZBC) &&
+		    pdv->pdv_bd) {
 			blkdev_put(pdv->pdv_bd,
 				   FMODE_WRITE|FMODE_READ|FMODE_EXCL);
 			pdv->pdv_bd = NULL;
@@ -1004,7 +1008,8 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 	req->end_io_data = cmd;
 	scsi_req(req)->cmd_len = scsi_command_size(pt->pscsi_cdb);
 	scsi_req(req)->cmd = &pt->pscsi_cdb[0];
-	if (pdv->pdv_sd->type == TYPE_DISK)
+	if (pdv->pdv_sd->type == TYPE_DISK ||
+	    pdv->pdv_sd->type == TYPE_ZBC)
 		req->timeout = PS_TIMEOUT_DISK;
 	else
 		req->timeout = PS_TIMEOUT_OTHER;
-- 
2.9.4

Re: [PATCH 2/5] target: pscsi: Introduce TYPE_ZBC support

[Nicholas A. Bellinger]

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> TYPE_ZBC host managed zoned block devices are also block devices
> despite the non-standard device type (14h). Handle them similarly to
> regular TYPE_DISK devices.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_pscsi.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Applied to target-pending/for-next.

Thanks.

[PATCH 3/5] target: pscsi: Fix sense data handling

[Damien Le Moal]

On completion of a request sent to the target backstore device,
pscsi_req_done() calls target_complete_cmd() which in turn will execute
pscsi_transport_complete(). In case of a failed request, this last
function will copy the target request sense data to the initiator side
request sense data. However, in pscsi_req_done(), the sense data is
retreived from the request after calling target_complete_cmd(), which
result in the sense data always conatining zeroes. Simply fix this by
copying the sense data before calling target_complete_cmd().

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/target/target_core_pscsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 97d0318..959d9f6 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1065,6 +1065,8 @@ static void pscsi_req_done(struct request *req, int uptodate)
 			pt->pscsi_result);
 	}
 
+	memcpy(pt->pscsi_sense, scsi_req(req)->sense, TRANSPORT_SENSE_BUFFER);
+
 	switch (host_byte(pt->pscsi_result)) {
 	case DID_OK:
 		target_complete_cmd(cmd, cmd->scsi_status);
@@ -1077,7 +1079,6 @@ static void pscsi_req_done(struct request *req, int uptodate)
 		break;
 	}
 
-	memcpy(pt->pscsi_sense, scsi_req(req)->sense, TRANSPORT_SENSE_BUFFER);
 	__blk_put_request(req->q, req);
 	kfree(pt);
 }
-- 
2.9.4

Re: [PATCH 3/5] target: pscsi: Fix sense data handling

[Nicholas A. Bellinger]

(Adding MNC CC')

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> On completion of a request sent to the target backstore device,
> pscsi_req_done() calls target_complete_cmd() which in turn will execute
> pscsi_transport_complete(). In case of a failed request, this last
> function will copy the target request sense data to the initiator side
> request sense data. However, in pscsi_req_done(), the sense data is
> retreived from the request after calling target_complete_cmd(), which
> result in the sense data always conatining zeroes. Simply fix this by
> copying the sense data before calling target_complete_cmd().
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_pscsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

As per MNC, skipping this patch as the following has been applied that
makes pscsi_complete_cmd() use the new transport_copy_sense_to_cmd()
helper.

https://www.spinics.net/lists/target-devel/msg15433.html

Please let me know if you run into any issues.

[PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

If the user request handler completed the request with a CHECK CONDITION
status, tcmu_handle_completion() copies the command entry sense data
into the session request structure sense data. However, the sense data
length indicated by the field scsi_sense_length is not set and equal to
0, resulting in the copy being a no-op and failure to propagate the
sense data back to the initiator. To fix this, properly set the session
command sense data length and also set the session command
SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
sense data.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/target/target_core_user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index beb5f09..7426b4c 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
 		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
 	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
 		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
-			       se_cmd->scsi_sense_length);
+		       TRANSPORT_SENSE_BUFFER);
+		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
+		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
 	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
 		/* Get Data-In buffer before clean up */
 		gather_data_area(udev, cmd, true);
-- 
2.9.4

Re: [PATCH 4/5] target: user: Fix sense data handling

[Mike Christie]

On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> If the user request handler completed the request with a CHECK CONDITION
> status, tcmu_handle_completion() copies the command entry sense data
> into the session request structure sense data. However, the sense data
> length indicated by the field scsi_sense_length is not set and equal to
> 0, resulting in the copy being a no-op and failure to propagate the
> sense data back to the initiator. To fix this, properly set the session
> command sense data length and also set the session command
> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> sense data.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_user.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index beb5f09..7426b4c 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
>  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
>  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
>  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> -			       se_cmd->scsi_sense_length);
> +		       TRANSPORT_SENSE_BUFFER);
> +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
>  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
>  		/* Get Data-In buffer before clean up */
>  		gather_data_area(udev, cmd, true);
> 

I have a patch similar to this and 5/5 in my set:

https://www.spinics.net/lists/target-devel/msg15430.html

If yours gets merged first then I will build my set over them, so patch
looks ok to me.

Reviewed-by: Mike Christie <mchristi@redhat.com>

Re: [PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

On 6/29/17 02:44, Mike Christie wrote:
> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
>> If the user request handler completed the request with a CHECK CONDITION
>> status, tcmu_handle_completion() copies the command entry sense data
>> into the session request structure sense data. However, the sense data
>> length indicated by the field scsi_sense_length is not set and equal to
>> 0, resulting in the copy being a no-op and failure to propagate the
>> sense data back to the initiator. To fix this, properly set the session
>> command sense data length and also set the session command
>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
>> sense data.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  drivers/target/target_core_user.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
>> index beb5f09..7426b4c 100644
>> --- a/drivers/target/target_core_user.c
>> +++ b/drivers/target/target_core_user.c
>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
>>  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
>>  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
>>  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
>> -			       se_cmd->scsi_sense_length);
>> +		       TRANSPORT_SENSE_BUFFER);
>> +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
>> +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
>>  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>  		/* Get Data-In buffer before clean up */
>>  		gather_data_area(udev, cmd, true);
>>
> 
> I have a patch similar to this and 5/5 in my set:
> 
> https://www.spinics.net/lists/target-devel/msg15430.html
> 
> If yours gets merged first then I will build my set over them, so patch
> looks ok to me.
> 
> Reviewed-by: Mike Christie <mchristi@redhat.com>

Mike,

Thank you for the review. I checked your series and it looks like it
addressing all the sense data handling problems I detected. I will
retest with your patch set applied.

Thanks !

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

Hey MNC & Co,

On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> > If the user request handler completed the request with a CHECK CONDITION
> > status, tcmu_handle_completion() copies the command entry sense data
> > into the session request structure sense data. However, the sense data
> > length indicated by the field scsi_sense_length is not set and equal to
> > 0, resulting in the copy being a no-op and failure to propagate the
> > sense data back to the initiator. To fix this, properly set the session
> > command sense data length and also set the session command
> > SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> > sense data.
> > 
> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > ---
> >  drivers/target/target_core_user.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> > index beb5f09..7426b4c 100644
> > --- a/drivers/target/target_core_user.c
> > +++ b/drivers/target/target_core_user.c
> > @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
> >  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> >  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> >  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> > -			       se_cmd->scsi_sense_length);
> > +		       TRANSPORT_SENSE_BUFFER);
> > +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> > +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> >  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> >  		/* Get Data-In buffer before clean up */
> >  		gather_data_area(udev, cmd, true);
> > 
> 
> I have a patch similar to this and 5/5 in my set:
> 
> https://www.spinics.net/lists/target-devel/msg15430.html
> 
> If yours gets merged first then I will build my set over them, so patch
> looks ok to me.
> 
> Reviewed-by: Mike Christie <mchristi@redhat.com>

The RFC patches above from May 31st weren't merged because I thought you
where going to send out a second series..

https://www.spinics.net/lists/target-devel/msg15505.html

Since that hasn't been the case, I'll go ahead and merge the bugfixes in
patches #1-6 for v4.13 now.  :)

Please resend patches #7-13 as post v4.13 items at your earliest
convenience.

Apologies for the confusion.

Re: [PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

Nicholas,

On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> Hey MNC & Co,
> 
> On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
>> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
>>> If the user request handler completed the request with a CHECK CONDITION
>>> status, tcmu_handle_completion() copies the command entry sense data
>>> into the session request structure sense data. However, the sense data
>>> length indicated by the field scsi_sense_length is not set and equal to
>>> 0, resulting in the copy being a no-op and failure to propagate the
>>> sense data back to the initiator. To fix this, properly set the session
>>> command sense data length and also set the session command
>>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
>>> sense data.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>> ---
>>>  drivers/target/target_core_user.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
>>> index beb5f09..7426b4c 100644
>>> --- a/drivers/target/target_core_user.c
>>> +++ b/drivers/target/target_core_user.c
>>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
>>>  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
>>>  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
>>>  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
>>> -			       se_cmd->scsi_sense_length);
>>> +		       TRANSPORT_SENSE_BUFFER);
>>> +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
>>> +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
>>>  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>>  		/* Get Data-In buffer before clean up */
>>>  		gather_data_area(udev, cmd, true);
>>>
>>
>> I have a patch similar to this and 5/5 in my set:
>>
>> https://www.spinics.net/lists/target-devel/msg15430.html
>>
>> If yours gets merged first then I will build my set over them, so patch
>> looks ok to me.
>>
>> Reviewed-by: Mike Christie <mchristi@redhat.com>
> 
> The RFC patches above from May 31st weren't merged because I thought you
> where going to send out a second series..

My apologies. I have been busy with other things and could not get to that.

> 
> https://www.spinics.net/lists/target-devel/msg15505.html
> 
> Since that hasn't been the case, I'll go ahead and merge the bugfixes in
> patches #1-6 for v4.13 now.  :)
> 
> Please resend patches #7-13 as post v4.13 items at your earliest
> convenience.

I will retest first thing Monday the merge with Mikes series and send
incremental fixes if any is needed.

Thank you.

Best regards.


-- 
Damien Le Moal,
Western Digital

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

On Fri, 2017-07-07 at 14:14 +0900, Damien Le Moal wrote:
> Nicholas,
> 
> On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> > Hey MNC & Co,
> > 
> > On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> >> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> >>> If the user request handler completed the request with a CHECK CONDITION
> >>> status, tcmu_handle_completion() copies the command entry sense data
> >>> into the session request structure sense data. However, the sense data
> >>> length indicated by the field scsi_sense_length is not set and equal to
> >>> 0, resulting in the copy being a no-op and failure to propagate the
> >>> sense data back to the initiator. To fix this, properly set the session
> >>> command sense data length and also set the session command
> >>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> >>> sense data.
> >>>
> >>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> >>> ---
> >>>  drivers/target/target_core_user.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> >>> index beb5f09..7426b4c 100644
> >>> --- a/drivers/target/target_core_user.c
> >>> +++ b/drivers/target/target_core_user.c
> >>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
> >>>  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> >>>  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> >>>  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> >>> -			       se_cmd->scsi_sense_length);
> >>> +		       TRANSPORT_SENSE_BUFFER);
> >>> +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> >>> +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> >>>  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> >>>  		/* Get Data-In buffer before clean up */
> >>>  		gather_data_area(udev, cmd, true);
> >>>
> >>
> >> I have a patch similar to this and 5/5 in my set:
> >>
> >> https://www.spinics.net/lists/target-devel/msg15430.html
> >>
> >> If yours gets merged first then I will build my set over them, so patch
> >> looks ok to me.
> >>
> >> Reviewed-by: Mike Christie <mchristi@redhat.com>
> > 
> > The RFC patches above from May 31st weren't merged because I thought you
> > where going to send out a second series..
> 
> My apologies. I have been busy with other things and could not get to that.
> 
> > 
> > https://www.spinics.net/lists/target-devel/msg15505.html
> > 
> > Since that hasn't been the case, I'll go ahead and merge the bugfixes in
> > patches #1-6 for v4.13 now.  :)
> > 
> > Please resend patches #7-13 as post v4.13 items at your earliest
> > convenience.
> 
> I will retest first thing Monday the merge with Mikes series and send
> incremental fixes if any is needed.
> 

Great, thanks.

Everything including MNC's #1-6 and your #1-2 be pushed to
target-pending/for-next shortly.

Please use this as your base for testing.  :)

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

On Thu, 2017-07-06 at 23:05 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-07-07 at 14:14 +0900, Damien Le Moal wrote:
> > Nicholas,
> > 
> > On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> > > Hey MNC & Co,
> > > 
> > > On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> > >> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> > >>> If the user request handler completed the request with a CHECK CONDITION
> > >>> status, tcmu_handle_completion() copies the command entry sense data
> > >>> into the session request structure sense data. However, the sense data
> > >>> length indicated by the field scsi_sense_length is not set and equal to
> > >>> 0, resulting in the copy being a no-op and failure to propagate the
> > >>> sense data back to the initiator. To fix this, properly set the session
> > >>> command sense data length and also set the session command
> > >>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> > >>> sense data.
> > >>>
> > >>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > >>> ---
> > >>>  drivers/target/target_core_user.c | 4 +++-
> > >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> > >>> index beb5f09..7426b4c 100644
> > >>> --- a/drivers/target/target_core_user.c
> > >>> +++ b/drivers/target/target_core_user.c
> > >>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
> > >>>  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> > >>>  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> > >>>  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> > >>> -			       se_cmd->scsi_sense_length);
> > >>> +		       TRANSPORT_SENSE_BUFFER);
> > >>> +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> > >>> +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> > >>>  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> > >>>  		/* Get Data-In buffer before clean up */
> > >>>  		gather_data_area(udev, cmd, true);
> > >>>
> > >>
> > >> I have a patch similar to this and 5/5 in my set:
> > >>
> > >> https://www.spinics.net/lists/target-devel/msg15430.html
> > >>
> > >> If yours gets merged first then I will build my set over them, so patch
> > >> looks ok to me.
> > >>
> > >> Reviewed-by: Mike Christie <mchristi@redhat.com>
> > > 
> > > The RFC patches above from May 31st weren't merged because I thought you
> > > where going to send out a second series..
> > 
> > My apologies. I have been busy with other things and could not get to that.
> > 

Oh btw, I was talking to MNC wrt to his original patch series, and not
yours.  :)

Re: [PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

On 7/7/17 15:06, Nicholas A. Bellinger wrote:
>>> My apologies. I have been busy with other things and could not get to that.
>>>
> 
> Oh btw, I was talking to MNC wrt to his original patch series, and not
> yours.  :)

Yes, I realized that after sending. Tired Friday afternoon here in
Japan. My brain is slowing down for the weekend :)

Cheers.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

Nicholas, Mike,

On 7/7/17 15:05, Nicholas A. Bellinger wrote:
> Everything including MNC's #1-6 and your #1-2 be pushed to
> target-pending/for-next shortly.
> 
> Please use this as your base for testing.  :)

I ran tests this morning with the latest target-pending/for-next branch.
I ran libzbc test suite on top of 4 different configurations:

1) ZBC drive + pscsi + loopback -> OK, no problems.
2) ZBC drive + pscsi + iscsi -> OK, no problems.
3) ZBC emulation tcmu-runner handler + loopback -> OK, no problems.
4) ZBC emulation tcmu-runner handler + iscsi -> Crash !

Here is the oops for case (4):

[  169.545459] scsi host7: iSCSI Initiator over TCP/IP
[  169.559013] scsi 7:0:0:0: Direct-Access-ZBC LIO-ORG  TCMU ZBC device
0002 PQ: 0 ANSI: 5
[  169.576920] sd 7:0:0:0: Attached scsi generic sg9 type 20
[  169.577209] sd 7:0:0:0: [sdi] Host-managed zoned block device
[  169.577794] sd 7:0:0:0: [sdi] 20971520 512-byte logical blocks: (10.7
GB/10.0 GiB)
[  169.577796] sd 7:0:0:0: [sdi] 40 zones of 524288 logical blocks
[  169.577980] sd 7:0:0:0: [sdi] Write Protect is off
[  169.578329] sd 7:0:0:0: [sdi] Write cache: enabled, read cache:
enabled, doesn't support DPO or FUA
[  169.590379] sd 7:0:0:0: [sdi] Attached SCSI disk
[  240.071464] BUG: unable to handle kernel paging request at
ffffc9065db85540
[  240.078460] IP: memcpy_erms+0x6/0x10
[  240.082044] PGD 7ff0ba067
[  240.082045] P4D 7ff0ba067
[  240.084766] PUD 0
[  240.087486]
[  240.091006] Oops: 0002 [#1] PREEMPT SMP
[  240.094855] Modules linked in: ip6table_filter ip6_tables
rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache
iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi sunrpc
snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec
snd_hwdep snd_hda_core snd_seq snd_seq_device x86_pkg_temp_thermal
coretemp snd_pcm crc32_pclmul snd_timer iTCO_wdt snd i2c_i801
iTCO_vendor_support soundcore i915 iosf_mbi i2c_algo_bit drm_kms_helper
syscopyarea sysfillrect sysimgblt fb_sys_fops drm e1000e r8169 mpt3sas
mii i2c_core raid_class video
[  240.143969] CPU: 0 PID: 1285 Comm: iscsi_trx Not tainted 4.12.0-rc1+ #3
[  240.150607] Hardware name: ASUS All Series/H87-PRO, BIOS 2104 10/28/2014
[  240.157331] task: ffff8807de4f5800 task.stack: ffffc900047dc000
[  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
[  240.167377] RSP: 0018:ffffc900047dfc68 EFLAGS: 00010202
[  240.172621] RAX: ffffc9065db85540 RBX: ffff8807f7980000 RCX:
0000000000000010
[  240.179771] RDX: 0000000000000010 RSI: ffff8807de574fe0 RDI:
ffffc9065db85540
[  240.186930] RBP: ffffc900047dfd30 R08: ffff8807de41b000 R09:
0000000000000000
[  240.194088] R10: 0000000000000040 R11: ffff8807e9b726f0 R12:
00000006565726b0
[  240.201246] R13: ffffc90007612ea0 R14: 000000065657d540 R15:
0000000000000000
[  240.208397] FS:  0000000000000000(0000) GS:ffff88081fa00000(0000)
knlGS:0000000000000000
[  240.216510] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  240.222280] CR2: ffffc9065db85540 CR3: 0000000001c0f000 CR4:
00000000001406f0
[  240.229430] Call Trace:
[  240.231887]  ? tcmu_queue_cmd+0x83c/0xa80
[  240.235916]  ? target_check_reservation+0xcd/0x6f0
[  240.240725]  __target_execute_cmd+0x27/0xa0
[  240.244918]  target_execute_cmd+0x232/0x2c0
[  240.249124]  ? __local_bh_enable_ip+0x64/0xa0
[  240.253499]  iscsit_execute_cmd+0x20d/0x270
[  240.257693]  iscsit_sequence_cmd+0x110/0x190
[  240.261985]  iscsit_get_rx_pdu+0x360/0xc80
[  240.267565]  ? iscsi_target_rx_thread+0x54/0xd0
[  240.273571]  iscsi_target_rx_thread+0x9a/0xd0
[  240.279413]  kthread+0x113/0x150
[  240.284120]  ? iscsi_target_tx_thread+0x1e0/0x1e0
[  240.290297]  ? kthread_create_on_node+0x40/0x40
[  240.296297]  ret_from_fork+0x2e/0x40
[  240.301332] Code: 90 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48
c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48
89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38
[  240.321751] RIP: memcpy_erms+0x6/0x10 RSP: ffffc900047dfc68
[  240.328838] CR2: ffffc9065db85540
[  240.333667] ---[ end trace b7e5354cfb54d08b ]---

I went back to running my initial 5 patch series on top of the current
4.12 kernel and everything is fine, including case (4).

A diff of the 2 versions of drivers/target/target_core_user.c did not
reveal anything obvious that could result in this... It does look like a
race condition on the session command or some memory corruption/bad
pointer. Any idea ?

Thanks you.

Best regards.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 4/5] target: user: Fix sense data handling

[Mike Christie]

On 07/10/2017 12:36 AM, Damien Le Moal wrote:
> Nicholas, Mike,
> 
> On 7/7/17 15:05, Nicholas A. Bellinger wrote:
>> Everything including MNC's #1-6 and your #1-2 be pushed to
>> target-pending/for-next shortly.
>>
>> Please use this as your base for testing.  :)
> 
> I ran tests this morning with the latest target-pending/for-next branch.
> I ran libzbc test suite on top of 4 different configurations:
> 
> 1) ZBC drive + pscsi + loopback -> OK, no problems.
> 2) ZBC drive + pscsi + iscsi -> OK, no problems.
> 3) ZBC emulation tcmu-runner handler + loopback -> OK, no problems.
> 4) ZBC emulation tcmu-runner handler + iscsi -> Crash !
> 
> Here is the oops for case (4):
> 
> [  169.545459] scsi host7: iSCSI Initiator over TCP/IP
> [  169.559013] scsi 7:0:0:0: Direct-Access-ZBC LIO-ORG  TCMU ZBC device
> 0002 PQ: 0 ANSI: 5
> [  169.576920] sd 7:0:0:0: Attached scsi generic sg9 type 20
> [  169.577209] sd 7:0:0:0: [sdi] Host-managed zoned block device
> [  169.577794] sd 7:0:0:0: [sdi] 20971520 512-byte logical blocks: (10.7
> GB/10.0 GiB)
> [  169.577796] sd 7:0:0:0: [sdi] 40 zones of 524288 logical blocks
> [  169.577980] sd 7:0:0:0: [sdi] Write Protect is off
> [  169.578329] sd 7:0:0:0: [sdi] Write cache: enabled, read cache:
> enabled, doesn't support DPO or FUA
> [  169.590379] sd 7:0:0:0: [sdi] Attached SCSI disk
> [  240.071464] BUG: unable to handle kernel paging request at
> ffffc9065db85540
> [  240.078460] IP: memcpy_erms+0x6/0x10
> [  240.082044] PGD 7ff0ba067
> [  240.082045] P4D 7ff0ba067
> [  240.084766] PUD 0
> [  240.087486]
> [  240.091006] Oops: 0002 [#1] PREEMPT SMP
> [  240.094855] Modules linked in: ip6table_filter ip6_tables
> rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache
> iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi sunrpc
> snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec
> snd_hwdep snd_hda_core snd_seq snd_seq_device x86_pkg_temp_thermal
> coretemp snd_pcm crc32_pclmul snd_timer iTCO_wdt snd i2c_i801
> iTCO_vendor_support soundcore i915 iosf_mbi i2c_algo_bit drm_kms_helper
> syscopyarea sysfillrect sysimgblt fb_sys_fops drm e1000e r8169 mpt3sas
> mii i2c_core raid_class video
> [  240.143969] CPU: 0 PID: 1285 Comm: iscsi_trx Not tainted 4.12.0-rc1+ #3
> [  240.150607] Hardware name: ASUS All Series/H87-PRO, BIOS 2104 10/28/2014
> [  240.157331] task: ffff8807de4f5800 task.stack: ffffc900047dc000
> [  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
> [  240.167377] RSP: 0018:ffffc900047dfc68 EFLAGS: 00010202
> [  240.172621] RAX: ffffc9065db85540 RBX: ffff8807f7980000 RCX:
> 0000000000000010
> [  240.179771] RDX: 0000000000000010 RSI: ffff8807de574fe0 RDI:
> ffffc9065db85540
> [  240.186930] RBP: ffffc900047dfd30 R08: ffff8807de41b000 R09:
> 0000000000000000
> [  240.194088] R10: 0000000000000040 R11: ffff8807e9b726f0 R12:
> 00000006565726b0
> [  240.201246] R13: ffffc90007612ea0 R14: 000000065657d540 R15:
> 0000000000000000
> [  240.208397] FS:  0000000000000000(0000) GS:ffff88081fa00000(0000)
> knlGS:0000000000000000
> [  240.216510] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  240.222280] CR2: ffffc9065db85540 CR3: 0000000001c0f000 CR4:
> 00000000001406f0
> [  240.229430] Call Trace:
> [  240.231887]  ? tcmu_queue_cmd+0x83c/0xa80
> [  240.235916]  ? target_check_reservation+0xcd/0x6f0
> [  240.240725]  __target_execute_cmd+0x27/0xa0
> [  240.244918]  target_execute_cmd+0x232/0x2c0
> [  240.249124]  ? __local_bh_enable_ip+0x64/0xa0
> [  240.253499]  iscsit_execute_cmd+0x20d/0x270
> [  240.257693]  iscsit_sequence_cmd+0x110/0x190
> [  240.261985]  iscsit_get_rx_pdu+0x360/0xc80
> [  240.267565]  ? iscsi_target_rx_thread+0x54/0xd0
> [  240.273571]  iscsi_target_rx_thread+0x9a/0xd0
> [  240.279413]  kthread+0x113/0x150
> [  240.284120]  ? iscsi_target_tx_thread+0x1e0/0x1e0
> [  240.290297]  ? kthread_create_on_node+0x40/0x40
> [  240.296297]  ret_from_fork+0x2e/0x40
> [  240.301332] Code: 90 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48
> c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48
> 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38
> [  240.321751] RIP: memcpy_erms+0x6/0x10 RSP: ffffc900047dfc68
> [  240.328838] CR2: ffffc9065db85540
> [  240.333667] ---[ end trace b7e5354cfb54d08b ]---
> 
> I went back to running my initial 5 patch series on top of the current
> 4.12 kernel and everything is fine, including case (4).
> 
> A diff of the 2 versions of drivers/target/target_core_user.c did not
> reveal anything obvious that could result in this... It does look like a
> race condition on the session command or some memory corruption/bad
> pointer. Any idea ?
> 

I have not seen this crash before. You are running these tests:

https://github.com/hgst/libzbc/tree/master/test

right?

What test was it? If you need a device that supports zone to run the
test, do you know what scsi command it crashed on? If not can you send a
tcmpdump trace and/or enable lio kernel debugging?

Re: [PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

Mike,

On 7/11/17 02:26, Mike Christie wrote:
> On 07/10/2017 12:36 AM, Damien Le Moal wrote:
>> Nicholas, Mike,
>>
>> On 7/7/17 15:05, Nicholas A. Bellinger wrote:
>>> Everything including MNC's #1-6 and your #1-2 be pushed to
>>> target-pending/for-next shortly.
>>>
>>> Please use this as your base for testing.  :)
>>
>> I ran tests this morning with the latest target-pending/for-next branch.
>> I ran libzbc test suite on top of 4 different configurations:
>>
>> 1) ZBC drive + pscsi + loopback -> OK, no problems.
>> 2) ZBC drive + pscsi + iscsi -> OK, no problems.
>> 3) ZBC emulation tcmu-runner handler + loopback -> OK, no problems.
>> 4) ZBC emulation tcmu-runner handler + iscsi -> Crash !
>>
>> Here is the oops for case (4):
>>
>> [  169.545459] scsi host7: iSCSI Initiator over TCP/IP
>> [  169.559013] scsi 7:0:0:0: Direct-Access-ZBC LIO-ORG  TCMU ZBC device
>> 0002 PQ: 0 ANSI: 5
>> [  169.576920] sd 7:0:0:0: Attached scsi generic sg9 type 20
>> [  169.577209] sd 7:0:0:0: [sdi] Host-managed zoned block device
>> [  169.577794] sd 7:0:0:0: [sdi] 20971520 512-byte logical blocks: (10.7
>> GB/10.0 GiB)
>> [  169.577796] sd 7:0:0:0: [sdi] 40 zones of 524288 logical blocks
>> [  169.577980] sd 7:0:0:0: [sdi] Write Protect is off
>> [  169.578329] sd 7:0:0:0: [sdi] Write cache: enabled, read cache:
>> enabled, doesn't support DPO or FUA
>> [  169.590379] sd 7:0:0:0: [sdi] Attached SCSI disk
>> [  240.071464] BUG: unable to handle kernel paging request at
>> ffffc9065db85540
>> [  240.078460] IP: memcpy_erms+0x6/0x10
>> [  240.082044] PGD 7ff0ba067
>> [  240.082045] P4D 7ff0ba067
>> [  240.084766] PUD 0
>> [  240.087486]
>> [  240.091006] Oops: 0002 [#1] PREEMPT SMP
>> [  240.094855] Modules linked in: ip6table_filter ip6_tables
>> rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache
>> iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi sunrpc
>> snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec
>> snd_hwdep snd_hda_core snd_seq snd_seq_device x86_pkg_temp_thermal
>> coretemp snd_pcm crc32_pclmul snd_timer iTCO_wdt snd i2c_i801
>> iTCO_vendor_support soundcore i915 iosf_mbi i2c_algo_bit drm_kms_helper
>> syscopyarea sysfillrect sysimgblt fb_sys_fops drm e1000e r8169 mpt3sas
>> mii i2c_core raid_class video
>> [  240.143969] CPU: 0 PID: 1285 Comm: iscsi_trx Not tainted 4.12.0-rc1+ #3
>> [  240.150607] Hardware name: ASUS All Series/H87-PRO, BIOS 2104 10/28/2014
>> [  240.157331] task: ffff8807de4f5800 task.stack: ffffc900047dc000
>> [  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
>> [  240.167377] RSP: 0018:ffffc900047dfc68 EFLAGS: 00010202
>> [  240.172621] RAX: ffffc9065db85540 RBX: ffff8807f7980000 RCX:
>> 0000000000000010
>> [  240.179771] RDX: 0000000000000010 RSI: ffff8807de574fe0 RDI:
>> ffffc9065db85540
>> [  240.186930] RBP: ffffc900047dfd30 R08: ffff8807de41b000 R09:
>> 0000000000000000
>> [  240.194088] R10: 0000000000000040 R11: ffff8807e9b726f0 R12:
>> 00000006565726b0
>> [  240.201246] R13: ffffc90007612ea0 R14: 000000065657d540 R15:
>> 0000000000000000
>> [  240.208397] FS:  0000000000000000(0000) GS:ffff88081fa00000(0000)
>> knlGS:0000000000000000
>> [  240.216510] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  240.222280] CR2: ffffc9065db85540 CR3: 0000000001c0f000 CR4:
>> 00000000001406f0
>> [  240.229430] Call Trace:
>> [  240.231887]  ? tcmu_queue_cmd+0x83c/0xa80
>> [  240.235916]  ? target_check_reservation+0xcd/0x6f0
>> [  240.240725]  __target_execute_cmd+0x27/0xa0
>> [  240.244918]  target_execute_cmd+0x232/0x2c0
>> [  240.249124]  ? __local_bh_enable_ip+0x64/0xa0
>> [  240.253499]  iscsit_execute_cmd+0x20d/0x270
>> [  240.257693]  iscsit_sequence_cmd+0x110/0x190
>> [  240.261985]  iscsit_get_rx_pdu+0x360/0xc80
>> [  240.267565]  ? iscsi_target_rx_thread+0x54/0xd0
>> [  240.273571]  iscsi_target_rx_thread+0x9a/0xd0
>> [  240.279413]  kthread+0x113/0x150
>> [  240.284120]  ? iscsi_target_tx_thread+0x1e0/0x1e0
>> [  240.290297]  ? kthread_create_on_node+0x40/0x40
>> [  240.296297]  ret_from_fork+0x2e/0x40
>> [  240.301332] Code: 90 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48
>> c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48
>> 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38
>> [  240.321751] RIP: memcpy_erms+0x6/0x10 RSP: ffffc900047dfc68
>> [  240.328838] CR2: ffffc9065db85540
>> [  240.333667] ---[ end trace b7e5354cfb54d08b ]---
>>
>> I went back to running my initial 5 patch series on top of the current
>> 4.12 kernel and everything is fine, including case (4).
>>
>> A diff of the 2 versions of drivers/target/target_core_user.c did not
>> reveal anything obvious that could result in this... It does look like a
>> race condition on the session command or some memory corruption/bad
>> pointer. Any idea ?
>>
> 
> I have not seen this crash before. You are running these tests:
> 
> https://github.com/hgst/libzbc/tree/master/test
> 
> right?

Yes.

> What test was it? If you need a device that supports zone to run the
> test, do you know what scsi command it crashed on? If not can you send a
> tcmpdump trace and/or enable lio kernel debugging?

It is always crashing on a 4KB write command in test 01.071 (WRITE
sequential zone boundary violation). This test verifies that the drive
fails a write command spanning zones and return correct sense key &
codes for the error. The write crashing the kernel is however not the
last one that should fail, but one in the middle of the zone (filling up
the zone to reach the end and generate the zone spanning write).
So this is not related to ZBC specific commands.

I will rerun with LIO debug and report back (anything in particular you
want or should I just enable everything ?)

I will also try with the regular tcmu-runner file handler to see if the
same problem exists there too.

Best regards.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 4/5] target: user: Fix sense data handling

[Mike Christie]

On 07/06/2017 11:50 PM, Nicholas A. Bellinger wrote:
> Hey MNC & Co,
> 
> On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
>> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
>>> If the user request handler completed the request with a CHECK CONDITION
>>> status, tcmu_handle_completion() copies the command entry sense data
>>> into the session request structure sense data. However, the sense data
>>> length indicated by the field scsi_sense_length is not set and equal to
>>> 0, resulting in the copy being a no-op and failure to propagate the
>>> sense data back to the initiator. To fix this, properly set the session
>>> command sense data length and also set the session command
>>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
>>> sense data.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>> ---
>>>  drivers/target/target_core_user.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
>>> index beb5f09..7426b4c 100644
>>> --- a/drivers/target/target_core_user.c
>>> +++ b/drivers/target/target_core_user.c
>>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, struct tcmu_cmd_entry *
>>>  		entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
>>>  	} else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
>>>  		memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
>>> -			       se_cmd->scsi_sense_length);
>>> +		       TRANSPORT_SENSE_BUFFER);
>>> +		se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
>>> +		se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
>>>  	} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>>  		/* Get Data-In buffer before clean up */
>>>  		gather_data_area(udev, cmd, true);
>>>
>>
>> I have a patch similar to this and 5/5 in my set:
>>
>> https://www.spinics.net/lists/target-devel/msg15430.html
>>
>> If yours gets merged first then I will build my set over them, so patch
>> looks ok to me.
>>
>> Reviewed-by: Mike Christie <mchristi@redhat.com>
> 
> The RFC patches above from May 31st weren't merged because I thought you
> where going to send out a second series..

I am/was. Sorry for the confusion. Above, I meant if Daemien's patches
gets merged before I can repost. I have been having troubles testing the
usb patch and was not sure how long it would take me.

I should have just broken out 1 - 6 like you just did for me below.

> 
> https://www.spinics.net/lists/target-devel/msg15505.html
> 
> Since that hasn't been the case, I'll go ahead and merge the bugfixes in
> patches #1-6 for v4.13 now.  :)

Thanks.

> 
> Please resend patches #7-13 as post v4.13 items at your earliest
> convenience.
> 
> Apologies for the confusion.
> 

It was my fault :)

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

(Adding MNC CC')

On Wed, 2017-06-28 at 14:58 +0900, Damien Le Moal wrote:
> If the user request handler completed the request with a CHECK CONDITION
> status, tcmu_handle_completion() copies the command entry sense data
> into the session request structure sense data. However, the sense data
> length indicated by the field scsi_sense_length is not set and equal to
> 0, resulting in the copy being a no-op and failure to propagate the
> sense data back to the initiator. To fix this, properly set the session
> command sense data length and also set the session command
> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> sense data.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_user.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Per MNC, skipping this patch in favor of tcmu_handle_completion() using
the new transport_copy_sense_to_cmd() helper.

https://www.spinics.net/lists/target-devel/msg15435.html

[PATCH 5/5] target: core: Fix failed command sense data handling

[Damien Le Moal]

For a target device without a transport->transport_complete method
defined (e.g. target_core_user), target_complete_cmd() will always
result in a failed command completion being processed through target
failure completion work even when the command failure comes from the
target processing and has valid sense data (and hence does not require
sense data emulation as done in the failure work processing). To ensure
that the failed command sense data is propagated as indicated by the
target, make sure that the normal "ok" work completion path is used by
moving the command SCF_TRANSPORT_TASK_SENSE flag test out of the
transport_complete defined conditional.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/target/target_core_transport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f1b3a46..a18e4db 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -719,10 +719,11 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 		dev->transport->transport_complete(cmd,
 				cmd->t_data_sg,
 				transport_get_sense_buffer(cmd));
-		if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
-			success = 1;
 	}
 
+	if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
+		success = 1;
+
 	/*
 	 * Check for case where an explicit ABORT_TASK has been received
 	 * and transport_wait_for_tasks() will be waiting for completion..
-- 
2.9.4

Re: [PATCH 5/5] target: core: Fix failed command sense data handling

[Mike Christie]

On 06/28/2017 12:59 AM, Damien Le Moal wrote:
> For a target device without a transport->transport_complete method
> defined (e.g. target_core_user), target_complete_cmd() will always
> result in a failed command completion being processed through target
> failure completion work even when the command failure comes from the
> target processing and has valid sense data (and hence does not require
> sense data emulation as done in the failure work processing). To ensure
> that the failed command sense data is propagated as indicated by the
> target, make sure that the normal "ok" work completion path is used by
> moving the command SCF_TRANSPORT_TASK_SENSE flag test out of the
> transport_complete defined conditional.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_transport.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index f1b3a46..a18e4db 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -719,10 +719,11 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
>  		dev->transport->transport_complete(cmd,
>  				cmd->t_data_sg,
>  				transport_get_sense_buffer(cmd));
> -		if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
> -			success = 1;
>  	}
>  
> +	if (cmd->se_cmd_flags & SCF_TRANSPORT_TASK_SENSE)
> +		success = 1;
> +
>  	/*
>  	 * Check for case where an explicit ABORT_TASK has been received
>  	 * and transport_wait_for_tasks() will be waiting for completion..
> 

Reviewed-by: Mike Christie <mchristi@redhat.com>

Re: [PATCH 5/5] target: core: Fix failed command sense data handling

[Nicholas A. Bellinger]

(Adding MNC CC')

On Wed, 2017-06-28 at 14:59 +0900, Damien Le Moal wrote:
> For a target device without a transport->transport_complete method
> defined (e.g. target_core_user), target_complete_cmd() will always
> result in a failed command completion being processed through target
> failure completion work even when the command failure comes from the
> target processing and has valid sense data (and hence does not require
> sense data emulation as done in the failure work processing). To ensure
> that the failed command sense data is propagated as indicated by the
> target, make sure that the normal "ok" work completion path is used by
> moving the command SCF_TRANSPORT_TASK_SENSE flag test out of the
> transport_complete defined conditional.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/target/target_core_transport.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Per MNC, skipping this patch in favor of target_complete_cmd() checking
se_cmd->scsi_status:

https://www.spinics.net/lists/target-devel/msg15436.html

Re: [PATCH 0/5] target: Zoned block device support and bug fixes

[Bryant G. Ly]

> This series introduce zoned block device support for the pscsi backstore and
> also fixes several problems with sense data handling for failed requests.
>
> The first patch is only a cleanup, so not really necessary but nice to have I
> think.
>
> Patch 2 and 3 introduce support for host managed zoned block device type (14h)
> in the SCSI passthrough backstore and fixes sense data handling for commands
> failed by the backstore device. With these fixes, a host zoned block device
> exported with the iscsi or loopback transport pass libzbc ZBC specification
> conformance tests.
>
> Finally, patch 4 and 5 fix sense data hadling with the user backstore code. A
> prototype ZBC emulation tcmu-runner handler was used to test these fixes and
> result in the emulated handler passing libzbc ZBC specification conformance
> tests.
>
> (Note: the ZBC emulation tcmu-runner handler will be submitted to the
> tcmu-runner project on github)
>
> Please consider these patches for inclusion with kernel 4.13.
>
> Damien Le Moal (5):
>    target: Use macro for WRITE_VERIFY_xx operation codes
>    target: pscsi: Introduce TYPE_ZBC support
>    target: pscsi: Fix sense data handling
>    target: user: Fix sense data handling
>    target: core: Fix failed command sense data handling
>
>   drivers/target/target_core_device.c    |  4 ++--
>   drivers/target/target_core_pscsi.c     | 20 +++++++++++++-------
>   drivers/target/target_core_transport.c |  5 +++--
>   drivers/target/target_core_user.c      |  4 +++-
>   include/scsi/scsi_proto.h              |  1 +
>   5 files changed, 22 insertions(+), 12 deletions(-)

Hi Damien,

You should take a look at the first two patches in this series to address your sense data handling.

https://www.spinics.net/lists/target-devel/msg15430.html

-Bryant

Re: [PATCH 0/5] target: Zoned block device support and bug fixes

[Damien Le Moal]

On 6/28/17 23:34, Bryant G. Ly wrote:
> 
>> This series introduce zoned block device support for the pscsi backstore and
>> also fixes several problems with sense data handling for failed requests.
>>
>> The first patch is only a cleanup, so not really necessary but nice to have I
>> think.
>>
>> Patch 2 and 3 introduce support for host managed zoned block device type (14h)
>> in the SCSI passthrough backstore and fixes sense data handling for commands
>> failed by the backstore device. With these fixes, a host zoned block device
>> exported with the iscsi or loopback transport pass libzbc ZBC specification
>> conformance tests.
>>
>> Finally, patch 4 and 5 fix sense data hadling with the user backstore code. A
>> prototype ZBC emulation tcmu-runner handler was used to test these fixes and
>> result in the emulated handler passing libzbc ZBC specification conformance
>> tests.
>>
>> (Note: the ZBC emulation tcmu-runner handler will be submitted to the
>> tcmu-runner project on github)
>>
>> Please consider these patches for inclusion with kernel 4.13.
>>
>> Damien Le Moal (5):
>>    target: Use macro for WRITE_VERIFY_xx operation codes
>>    target: pscsi: Introduce TYPE_ZBC support
>>    target: pscsi: Fix sense data handling
>>    target: user: Fix sense data handling
>>    target: core: Fix failed command sense data handling
>>
>>   drivers/target/target_core_device.c    |  4 ++--
>>   drivers/target/target_core_pscsi.c     | 20 +++++++++++++-------
>>   drivers/target/target_core_transport.c |  5 +++--
>>   drivers/target/target_core_user.c      |  4 +++-
>>   include/scsi/scsi_proto.h              |  1 +
>>   5 files changed, 22 insertions(+), 12 deletions(-)
> 
> Hi Damien,
> 
> You should take a look at the first two patches in this series to address your sense data handling.
> 
> https://www.spinics.net/lists/target-devel/msg15430.html

Bryant,

Thank you for the pointer. I missed that.

-- 
Damien Le Moal,
Western Digital