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

2017-07-10 Thread 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
>> c9065db85540
>> [  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: 8807de4f5800 task.stack: c900047dc000
>> [  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
>> [  240.167377] RSP: 0018:c900047dfc68 EFLAGS: 00010202
>> [  240.172621] RAX: c9065db85540 RBX: 8807f798 RCX:
>> 0010
>> [  240.179771] RDX: 0010 RSI: 8807de574fe0 RDI:
>> c9065db85540
>> [  240.186930] RBP: c900047dfd30 R08: 8807de41b000 R09:
>> 
>> [  240.194088] R10: 0040 R11: 8807e9b726f0 R12:
>> 0006565726b0
>> [  240.201246] R13: c90007612ea0 R14: 00065657d540 R15:
>> 
>> [  240.208397] FS:  () GS:88081fa0()
>> knlGS:
>> [  240.216510] CS:  0010 DS:  ES:  CR0: 80050033
>> [  240.80] CR2: c9065db85540 CR3: 01c0f000 CR4:
>> 001406f0
>> [  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  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: c900047dfc68
>> [  240.328838] CR2: c9065db85540
>> [  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

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

2017-07-10 Thread 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
> c9065db85540
> [  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: 8807de4f5800 task.stack: c900047dc000
> [  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
> [  240.167377] RSP: 0018:c900047dfc68 EFLAGS: 00010202
> [  240.172621] RAX: c9065db85540 RBX: 8807f798 RCX:
> 0010
> [  240.179771] RDX: 0010 RSI: 8807de574fe0 RDI:
> c9065db85540
> [  240.186930] RBP: c900047dfd30 R08: 8807de41b000 R09:
> 
> [  240.194088] R10: 0040 R11: 8807e9b726f0 R12:
> 0006565726b0
> [  240.201246] R13: c90007612ea0 R14: 00065657d540 R15:
> 
> [  240.208397] FS:  () GS:88081fa0()
> knlGS:
> [  240.216510] CS:  0010 DS:  ES:  CR0: 80050033
> [  240.80] CR2: c9065db85540 CR3: 01c0f000 CR4:
> 001406f0
> [  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  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: c900047dfc68
> [  240.328838] CR2: c9065db85540
> [  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

2017-07-09 Thread 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
c9065db85540
[  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: 8807de4f5800 task.stack: c900047dc000
[  240.163270] RIP: 0010:memcpy_erms+0x6/0x10
[  240.167377] RSP: 0018:c900047dfc68 EFLAGS: 00010202
[  240.172621] RAX: c9065db85540 RBX: 8807f798 RCX:
0010
[  240.179771] RDX: 0010 RSI: 8807de574fe0 RDI:
c9065db85540
[  240.186930] RBP: c900047dfd30 R08: 8807de41b000 R09:

[  240.194088] R10: 0040 R11: 8807e9b726f0 R12:
0006565726b0
[  240.201246] R13: c90007612ea0 R14: 00065657d540 R15:

[  240.208397] FS:  () GS:88081fa0()
knlGS:
[  240.216510] CS:  0010 DS:  ES:  CR0: 80050033
[  240.80] CR2: c9065db85540 CR3: 01c0f000 CR4:
001406f0
[  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  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: c900047dfc68
[  240.328838] CR2: c9065db85540
[  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

2017-07-07 Thread 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 
>>> ---
>>>  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 
> 
> 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

2017-07-07 Thread 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

2017-07-07 Thread 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 
> > >>> ---
> > >>>  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 
> > > 
> > > 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

2017-07-07 Thread 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 
> >>> ---
> >>>  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 
> > 
> > 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

2017-07-06 Thread 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 
>>> ---
>>>  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 
> 
> 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

2017-07-06 Thread 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 
> ---
>  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

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

2017-07-06 Thread 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 
> > ---
> >  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 

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

2017-06-28 Thread 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 
>> ---
>>  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 

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

2017-06-28 Thread 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 
> ---
>  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

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

2017-06-28 Thread 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 
---
 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