Re: [PATCH v2] target: transport should allow st ILI reads

2018-05-10 Thread Lee Duncan 
On 05/10/2018 12:19 PM, Mike Christie wrote:
> On 05/10/2018 01:51 PM, Mike Christie wrote:
>> On 05/09/2018 03:59 PM, Lee Duncan wrote:
>>> When a tape drive is exported via LIO using the
>>> pscsi module, a read that requests more bytes per block
>>> than the tape can supply returns an empty buffer. This
>>> is because the pscsi pass-through target module sees
>>> the "ILI" illegal length bit set and thinks there
>>> is no reason to return the data.
>>>
>>> This is a long-standing transport issue, since it
>>> assume that no data need be returned under a check
>>> condition, which isn't always the case for tape.
>>>
>>> Add in a check for tape reads with the the ILI, EOM,
>>> or FM bits set, with a sense code of NO_SENSE,
>>> treating such cases as if there is no sense data
>>> and the read succeeded. The layered tape driver then
>>> "does the right thing" when it gets such a response.
>>>
>>> Changes from RFC:
>>>  - Moved ugly code from transport to pscsi module
>>>  - Added checking EOM and FM bits, as well as ILI
>>>  - fixed malformed patch
>>>  - Clarified description a bit
>>>
>>> Signed-off-by: Lee Duncan 
>>> ---
>>>  drivers/target/target_core_pscsi.c | 22 +-
>>>  drivers/target/target_core_transport.c |  6 ++
>>>  include/target/target_core_base.h  |  1 +
>>>  3 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> ...
>>
>>
>> Do you want to return both the data and sense or just one or the other?
>> Both right? In this code path we would return both the data and sense
>> for drivers like iscsi.

Yes, both the sense data and the IO data get returned with this patch.

>>
>> If the queue_data_in call a little below this line returns
>> -ENOMEM/-EAGAIN then I think the queue full handling is going to end up
>> only returning the sense like in your original bug. You would need a
>> similar change to transport_complete_qf so it returns the data.

Good point. I will update the patch.

>>
> 
> Oh yeah, one other question/comment. The above code is bypassing the
> normal sense sending code so SCF_SENT_CHECK_CONDITION is not going to be
> set. For iscsi could you end up where 2 paths complete the same command
> because a reassign could race with one of these completions?
> 

Good question.

I believe it will not be an issue. I believe that if the IO completes on
multiple paths it will be treated the same on each path, which means
that the SCF_SENT_CHECK_CONDITION will not be set on either path. Which
is how it is handled now for a normal IO.
-- 
Lee

Re: [PATCH v2] target: transport should allow st ILI reads

2018-05-10 Thread Mike Christie 
On 05/10/2018 01:51 PM, Mike Christie wrote:
> On 05/09/2018 03:59 PM, Lee Duncan wrote:
>> When a tape drive is exported via LIO using the
>> pscsi module, a read that requests more bytes per block
>> than the tape can supply returns an empty buffer. This
>> is because the pscsi pass-through target module sees
>> the "ILI" illegal length bit set and thinks there
>> is no reason to return the data.
>>
>> This is a long-standing transport issue, since it
>> assume that no data need be returned under a check
>> condition, which isn't always the case for tape.
>>
>> Add in a check for tape reads with the the ILI, EOM,
>> or FM bits set, with a sense code of NO_SENSE,
>> treating such cases as if there is no sense data
>> and the read succeeded. The layered tape driver then
>> "does the right thing" when it gets such a response.
>>
>> Changes from RFC:
>>  - Moved ugly code from transport to pscsi module
>>  - Added checking EOM and FM bits, as well as ILI
>>  - fixed malformed patch
>>  - Clarified description a bit
>>
>> Signed-off-by: Lee Duncan 
>> ---
>>  drivers/target/target_core_pscsi.c | 22 +-
>>  drivers/target/target_core_transport.c |  6 ++
>>  include/target/target_core_base.h  |  1 +
>>  3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_pscsi.c 
>> b/drivers/target/target_core_pscsi.c
>> index 0d99b242e82e..b237104af81c 100644
>> --- a/drivers/target/target_core_pscsi.c
>> +++ b/drivers/target/target_core_pscsi.c
>> @@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 
>> scsi_status,
>>  }
>>  after_mode_select:
>>  
>> -if (scsi_status == SAM_STAT_CHECK_CONDITION)
>> +if (scsi_status == SAM_STAT_CHECK_CONDITION) {
>>  transport_copy_sense_to_cmd(cmd, req_sense);
>> +
>> +/*
>> + * hack to check for TAPE device reads with
>> + * FM/EOM/ILI set, so that we can get data
>> + * back despite framework assumption that a
>> + * check condition means there is no data
>> + */
>> +if ((sd->type == TYPE_TAPE) &&
>> +(cmd->data_direction == DMA_FROM_DEVICE)) {
>> +/*
>> + * is sense data valid, fixed format,
>> + * and have FM, EOM, or ILI set?
>> + */
>> +if ((req_sense[0] == 0xf0) &&   /* valid, fixed format 
>> */
>> +(req_sense[2] & 0xe0) &&/* FM, EOM, or ILI */
>> +((req_sense[2] & 0xf) == 0)) { /* key==NO_SENSE */
>> +cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
>> +}
>> +}
>> +}
>>  }
>>  
>>  enum {
>> diff --git a/drivers/target/target_core_transport.c 
>> b/drivers/target/target_core_transport.c
>> index 74b646f165d4..56661a824266 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct 
>> work_struct *work)
>>  if (atomic_read(>se_dev->dev_qf_count) != 0)
>>  schedule_work(>se_dev->qf_work_queue);
>>  
>> +if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
>> +pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
>> +goto treat_as_normal_read;
>> +}
>> +
>>  /*
>>   * Check if we need to send a sense buffer from
>>   * the struct se_cmd in question.
>> @@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct 
>> *work)
>>  if (cmd->scsi_status)
>>  goto queue_status;
>>  
>> +treat_as_normal_read:
>>  atomic_long_add(cmd->data_length,
>>  >se_lun->lun_stats.tx_data_octets);
> 
> 
> Do you want to return both the data and sense or just one or the other?
> Both right? In this code path we would return both the data and sense
> for drivers like iscsi.
> 
> If the queue_data_in call a little below this line returns
> -ENOMEM/-EAGAIN then I think the queue full handling is going to end up
> only returning the sense like in your original bug. You would need a
> similar change to transport_complete_qf so it returns the data.
> 

Oh yeah, one other question/comment. The above code is bypassing the
normal sense sending code so SCF_SENT_CHECK_CONDITION is not going to be
set. For iscsi could you end up where 2 paths complete the same command
because a reassign could race with one of these completions?

Re: [PATCH v2] target: transport should allow st ILI reads

2018-05-10 Thread Mike Christie 
On 05/09/2018 03:59 PM, Lee Duncan wrote:
> When a tape drive is exported via LIO using the
> pscsi module, a read that requests more bytes per block
> than the tape can supply returns an empty buffer. This
> is because the pscsi pass-through target module sees
> the "ILI" illegal length bit set and thinks there
> is no reason to return the data.
> 
> This is a long-standing transport issue, since it
> assume that no data need be returned under a check
> condition, which isn't always the case for tape.
> 
> Add in a check for tape reads with the the ILI, EOM,
> or FM bits set, with a sense code of NO_SENSE,
> treating such cases as if there is no sense data
> and the read succeeded. The layered tape driver then
> "does the right thing" when it gets such a response.
> 
> Changes from RFC:
>  - Moved ugly code from transport to pscsi module
>  - Added checking EOM and FM bits, as well as ILI
>  - fixed malformed patch
>  - Clarified description a bit
> 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/target/target_core_pscsi.c | 22 +-
>  drivers/target/target_core_transport.c |  6 ++
>  include/target/target_core_base.h  |  1 +
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_pscsi.c 
> b/drivers/target/target_core_pscsi.c
> index 0d99b242e82e..b237104af81c 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 
> scsi_status,
>   }
>  after_mode_select:
>  
> - if (scsi_status == SAM_STAT_CHECK_CONDITION)
> + if (scsi_status == SAM_STAT_CHECK_CONDITION) {
>   transport_copy_sense_to_cmd(cmd, req_sense);
> +
> + /*
> +  * hack to check for TAPE device reads with
> +  * FM/EOM/ILI set, so that we can get data
> +  * back despite framework assumption that a
> +  * check condition means there is no data
> +  */
> + if ((sd->type == TYPE_TAPE) &&
> + (cmd->data_direction == DMA_FROM_DEVICE)) {
> + /*
> +  * is sense data valid, fixed format,
> +  * and have FM, EOM, or ILI set?
> +  */
> + if ((req_sense[0] == 0xf0) &&   /* valid, fixed format 
> */
> + (req_sense[2] & 0xe0) &&/* FM, EOM, or ILI */
> + ((req_sense[2] & 0xf) == 0)) { /* key==NO_SENSE */
> + cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
> + }
> + }
> + }
>  }
>  
>  enum {
> diff --git a/drivers/target/target_core_transport.c 
> b/drivers/target/target_core_transport.c
> index 74b646f165d4..56661a824266 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>   if (atomic_read(>se_dev->dev_qf_count) != 0)
>   schedule_work(>se_dev->qf_work_queue);
>  
> + if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
> + pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
> + goto treat_as_normal_read;
> + }
> +
>   /*
>* Check if we need to send a sense buffer from
>* the struct se_cmd in question.
> @@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>   if (cmd->scsi_status)
>   goto queue_status;
>  
> +treat_as_normal_read:
>   atomic_long_add(cmd->data_length,
>   >se_lun->lun_stats.tx_data_octets);


Do you want to return both the data and sense or just one or the other?
Both right? In this code path we would return both the data and sense
for drivers like iscsi.

If the queue_data_in call a little below this line returns
-ENOMEM/-EAGAIN then I think the queue full handling is going to end up
only returning the sense like in your original bug. You would need a
similar change to transport_complete_qf so it returns the data.

Re: [PATCH-next] scsi: libsas: dynamically allocate and free ata host

2018-05-10 Thread Tejun Heo 
On Thu, May 10, 2018 at 11:05:16AM +0800, Jason Yan wrote:
> Commit 2623c7a5f2 ("libata: add refcounting to ata_host") v4.17+
> introduced refcounting to ata_host and will increase or decrease the
> refcount when adding or deleting transport ATA port.

libata side looks good to me.

Acked-by: Tejun Heo 

Thanks.

-- 
tejun

Re: [RESEND] tcmu: fix error resetting qfull_time_out to default

2018-05-10 Thread Mike Christie 
On 05/10/2018 08:42 AM, Prasanna Kumar Kalever wrote:
> Problem:
> ---
> $ cat /sys/kernel/config/target/core/user_0/block/attrib/qfull_time_out
> -1
> 
> $ echo "-1" > 
> /sys/kernel/config/target/core/user_0/block/attrib/qfull_time_out
> -bash: echo: write error: Invalid argument
> 
> Fix:
> ---
> This patch will help reset qfull_time_out to its default i.e. 
> qfull_time_out=-1
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  drivers/target/target_core_user.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 4ad89ea71a70..4f26bdc3d1dc 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -2121,6 +2121,8 @@ static ssize_t tcmu_qfull_time_out_store(struct 
> config_item *item,
>  
>   if (val >= 0) {
>   udev->qfull_time_out = val * MSEC_PER_SEC;
> + } else if (val == -1) {
> + udev->qfull_time_out = val;
>   } else {
>   printk(KERN_ERR "Invalid qfull timeout value %d\n", val);
>   return -EINVAL;
> 

Look ok.

Acked-by: Mike Christie

[RESEND] tcmu: fix error resetting qfull_time_out to default

2018-05-10 Thread Prasanna Kumar Kalever 
Problem:
---
$ cat /sys/kernel/config/target/core/user_0/block/attrib/qfull_time_out
-1

$ echo "-1" > /sys/kernel/config/target/core/user_0/block/attrib/qfull_time_out
-bash: echo: write error: Invalid argument

Fix:
---
This patch will help reset qfull_time_out to its default i.e. qfull_time_out=-1

Signed-off-by: Prasanna Kumar Kalever 
---
 drivers/target/target_core_user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 4ad89ea71a70..4f26bdc3d1dc 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2121,6 +2121,8 @@ static ssize_t tcmu_qfull_time_out_store(struct 
config_item *item,
 
if (val >= 0) {
udev->qfull_time_out = val * MSEC_PER_SEC;
+   } else if (val == -1) {
+   udev->qfull_time_out = val;
} else {
printk(KERN_ERR "Invalid qfull timeout value %d\n", val);
return -EINVAL;
-- 
2.14.3

Re: [PATCH] scsi: qlogicpti: Fix an error handling path in 'qpti_sbus_probe()'

2018-05-10 Thread Dan Carpenter 
On Thu, May 10, 2018 at 01:45:58PM +0200, Christophe JAILLET wrote:
> The 'free_irq()' call is not at the right place in the error handling path.
> The changed order has been introduced in commit 3d4253d9afab
> ("[SCSI] qlogicpti: Convert to new SBUS device framework.")
> 
> Fixes: 3d4253d9afab ("[SCSI] qlogicpti: Convert to new SBUS device 
> framework.")
> Signed-off-by: Christophe JAILLET 
> ---
> Please review carefully.  This patch is proposed because it triggers one of
> my coccinelle scripts. I'm not 100% sure if correct.

Looks good to me.

Reviewed-by: Dan Carpenter 

regards,
dan carpenter

[PATCH] scsi: qlogicpti: Fix an error handling path in 'qpti_sbus_probe()'

2018-05-10 Thread Christophe JAILLET 
The 'free_irq()' call is not at the right place in the error handling path.
The changed order has been introduced in commit 3d4253d9afab
("[SCSI] qlogicpti: Convert to new SBUS device framework.")

Fixes: 3d4253d9afab ("[SCSI] qlogicpti: Convert to new SBUS device framework.")
Signed-off-by: Christophe JAILLET 
---
Please review carefully.  This patch is proposed because it triggers one of
my coccinelle scripts. I'm not 100% sure if correct.

The script tries to spot wrongly ordered error handling path. It is:
@@
identifier l1, l2;
@@

  if (...) {
 ...
* goto l1;
  }
  ...
  if (...) {
 ...
* goto l2;
  }
  ...
*l1:
  ...
*l2:
  ...
---
 drivers/scsi/qlogicpti.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qlogicpti.c b/drivers/scsi/qlogicpti.c
index cec9a14982e6..8578e566ab41 100644
--- a/drivers/scsi/qlogicpti.c
+++ b/drivers/scsi/qlogicpti.c
@@ -1385,6 +1385,9 @@ static int qpti_sbus_probe(struct platform_device *op)
  qpti->req_cpu, qpti->req_dvma);
 #undef QSIZE
 
+fail_free_irq:
+   free_irq(qpti->irq, qpti);
+
 fail_unmap_regs:
of_iounmap(>resource[0], qpti->qregs,
   resource_size(>resource[0]));
@@ -1392,9 +1395,6 @@ static int qpti_sbus_probe(struct platform_device *op)
of_iounmap(>resource[0], qpti->sreg,
   sizeof(unsigned char));
 
-fail_free_irq:
-   free_irq(qpti->irq, qpti);
-
 fail_unlink:
scsi_host_put(host);
 
-- 
2.17.0

Re: [PATCH v2] target: transport should allow st ILI reads

2018-05-10 Thread Christoph Hellwig 
On Wed, May 09, 2018 at 01:59:21PM -0700, Lee Duncan wrote:
> When a tape drive is exported via LIO using the
> pscsi module, a read that requests more bytes per block
> than the tape can supply returns an empty buffer. This
> is because the pscsi pass-through target module sees
> the "ILI" illegal length bit set and thinks there
> is no reason to return the data.

Please use the full line length available for commit messages..

> and the read succeeded. The layered tape driver then
> "does the right thing" when it gets such a response.
>  - Moved ugly code from transport to pscsi module

probably wants and uptdate of the subject line as well.

> +  * hack to check for TAPE device reads with
> +  * FM/EOM/ILI set, so that we can get data
> +  * back despite framework assumption that a
> +  * check condition means there is no data
> +  */
> + if ((sd->type == TYPE_TAPE) &&
> + (cmd->data_direction == DMA_FROM_DEVICE)) {

No need for the inner braces.

> + if ((req_sense[0] == 0xf0) &&   /* valid, fixed format 
> */
> + (req_sense[2] & 0xe0) &&/* FM, EOM, or ILI */
> + ((req_sense[2] & 0xf) == 0)) { /* key==NO_SENSE */
> + cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;

Same here.

>  enum {
> diff --git a/drivers/target/target_core_transport.c 
> b/drivers/target/target_core_transport.c
> index 74b646f165d4..56661a824266 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>   if (atomic_read(>se_dev->dev_qf_count) != 0)
>   schedule_work(>se_dev->qf_work_queue);
>  
> + if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
> + pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
> + goto treat_as_normal_read;
> + }
> +
>   /*
>* Check if we need to send a sense buffer from
>* the struct se_cmd in question.
> @@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>   if (cmd->scsi_status)
>   goto queue_status;
>  
> +treat_as_normal_read:
>   atomic_long_add(cmd->data_length,
>   >se_lun->lun_stats.tx_data_octets);
>   /*

The goto looks at least a little odd as it skips many things that
don't look realted.  As far as I can tell what you want to skip is
mostly the SCF_TRANSPORT_TASK_SENSE check, which can be done with
an additional conditional.  Do we also need to skip the scsi_status
check above?  If yes it needs another conditional, and a good comment.