Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[chenxiang (M)]

Hi,
在 2018/12/6 20:04, John Garry 写道:

On 06/12/2018 04:17, Martin K. Petersen wrote:

+



Bart,


Had you considered to use lower_32_bits() instead of "0x"?
That would to avoid that reviewers have to count the 'f'-s to verify
correctness of t10_pi_ref_tag().


I hadn't. I guess I tend to think of lower_32_bits() as something you do
to pointers, not to block numbers.



Xiang Chen tested the patch successfully. I'll let him provide 
tested-by tag.


I have tested the patch with running fio on 3008 (on our ARM64 board 
with 4 DIF disks and 8 non-DIF disks), and now it works well.

Tested-by: Xiang Chen 



Thanks,
John


.

Performance improvements & regressions in storage IO tests in Linux Kernel 4.19

[Rajender M]

t #'s across iterations. 
- The above performance data is from local SSD as backend device, in which we 
see latency improvements of up to 60% and CPU cost regressions of up to 23%
- For FC SAN as backend device, the depth is slightly varying - with latency 
improvements of up to 40% and CPU cost regressions of up to  13%. 

4. Conclusions
The results indicate very significant latency improvements at the cost of 
incremental CPU consumption and this behavior is specifically visible for large 
sequential reads/write operations (e.g. 60% latency improvement combined with 
23% increase in CPU consumption for 64k sequential reads.) This can be seen as 
expected behavior because the change enables more parallelism in the storage 
stack which will inherently allow for more CPU cycles to be consumed. Assuming 
that in most deployments (be it bare metal or virtual) administrators or 
automated resource management tools will strive to keep CPU utilization at 70% 
or below, this change should be seen as a significant improvement for most 
customers. An exception could be in non-customer controlled Cloud environments 
where the additional cycles might not be available and as such the latency 
improvements might not be achievable.

Rajender M
Performance Engineering
VMware, Inc.

Re: Grant

[Maratovich M. Fridman]

--
I Mikhail Fridman. has selected you specially as one of my beneficiaries
for my Charitable Donation, Just as I have declared on May 23, 2016 to give
my fortune as charity.

Check the link below for confirmation:

http://www.ibtimes.co.uk/russias-second-wealthiest-man-mikhail-fridman-plans-leaving-14-2bn-fortune-charity-1561604

Reply as soon as possible with further directives.

Best Regards,
Mikhail Fridman.

Re: [PATCH] scsi: ata: don't reset three times if device is offline for SAS host

[chenxiang (M)]

Hi Tejun,

在 2018/2/28 2:19, Tejun Heo 写道:

Hello,

On Mon, Feb 26, 2018 at 07:45:37PM +0800, chenxiang (M) wrote:

So, if there are real consequences, we can definitely add a way to
short-circuit the recovery logic but let's do that by adding proper
signaling rathr than testing for driver type.

I am not familiar with ata recovery logic, and do you have idea
about how to add a way to
short-circuit the recovery logic by adding proper signaling?

e.g. define a return value from reset function which indicates no
retry or introduce a port flag.  Basically, something which doens't
add special casing logic to the core logic.


If we can introduce a port flags such as ATA_LFLAG_DISABLED or 
ATA_EHI_NO_RECOVERY before ata_eh_recover,
it will skip recovery. But we only get device status NODEV from 
ata_do_reset which is after ata_eh_recover, i don't

think it is used to introduce a port flags at that time.
From function ata_eh_reset, it seems there are two situations that end 
the recovery logic in current code:

1. Retry 3 times;
2. Reset function return value -ERESTART, but this return value seems be 
specal situation for ATA;





Thanks.

Re: [bug report] Don't enter SCSI error handler on kernel 4.16-rc1

[chenxiang (M)]

在 2018/2/27 22:57, Bart Van Assche 写道:

On Tue, 2018-02-27 at 15:09 +0800, chenxiang (M) wrote:

在 2018/2/26 23:25, Bart Van Assche 写道:

On Mon, 2018-02-26 at 17:37 +0800, chenxiang (M) wrote:

When i have a test on kernel 4.16-rc1, find a issue: running IO on SATA disk, 
then disable the disk through
sysfs interface(echo 0 > /sys/class/sas_phy/phy-1:0:0/enable), IO will hang and 
never enter SCSI EH. The issue
appears every time.

I add some prints on code and find that  those IOs will be timeout after 30s, 
and they all enter
function scsi_eh_scmd_add, but only some of them can enter function 
scsi_eh_inc_host_failed. So it will never
enter SCSI EH.  I suspect it is related to the patch ("commit 3bd6f43f5cb371" 
scsi: core: Ensure that the
SCSI error handler gets woken up ). Please have a check.

Hello chenxiang,

Had you already noticed patch "[PATCH v2] Avoid that ATA error handling can
trigger a kernel hang or oops"? If not, can you apply that patch to your
kernel and verify whether it fixes this behavior? See also
https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg71189.html or
https://patchwork.kernel.org/patch/10236213/.

After applied your patch, the issue i reported seems be solved.

Thanks for having testing that patch!


But when i have long time test(disable/enable disk when running IO) on
the testcase, Null pointer occurs.
It seems not related to current issue but i am not sure.
I ran the testcase for long time before in kernel 4.15-rc5, and it was okay.

Part of log is as follows, and i add attachment of log in the email :

[  485.716578] pc : blk_abort_request+0x14/0x68

(+Tejun)

Hello chenxiang,

Please check whether the following patch fixes the kernel crash you ran into:

https://marc.info/?l=linux-block=151895951207014


It seems the patch is for block mq, but the issue i encount is under 
block legacy as CONFIG_SCSI_MQ_DEFAULT is not enabled.




Thanks,

Bart.

Re: [bug report] Don't enter SCSI error handler on kernel 4.16-rc1

[chenxiang (M)]

在 2018/2/26 23:25, Bart Van Assche 写道:

On Mon, 2018-02-26 at 17:37 +0800, chenxiang (M) wrote:

When i have a test on kernel 4.16-rc1, find a issue: running IO on SATA disk, 
then disable the disk through
sysfs interface(echo 0 > /sys/class/sas_phy/phy-1:0:0/enable), IO will hang and 
never enter SCSI EH. The issue
appears every time.

I add some prints on code and find that  those IOs will be timeout after 30s, 
and they all enter
function scsi_eh_scmd_add, but only some of them can enter function 
scsi_eh_inc_host_failed. So it will never
enter SCSI EH.  I suspect it is related to the patch ("commit 3bd6f43f5cb371" 
scsi: core: Ensure that the
SCSI error handler gets woken up ). Please have a check.

Hello chenxiang,

Had you already noticed patch "[PATCH v2] Avoid that ATA error handling can
trigger a kernel hang or oops"? If not, can you apply that patch to your
kernel and verify whether it fixes this behavior? See also
https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg71189.html or
https://patchwork.kernel.org/patch/10236213/.


Ok， i will verify the patch.


Thanks,

Bart.

Re: [PATCH] scsi: ata: don't reset three times if device is offline for SAS host

[chenxiang (M)]

Hi Tejun,
Sorry for my late reply as i have a vacation last week.

在 2018/2/13 22:27, Tejun Heo 写道:

Hello,

On Tue, Feb 13, 2018 at 09:44:53AM +0800, chenxiang (M) wrote:

For those drivers using libsas,  i think they have the same issue.
It takes about 1 minute to
recover but actually device is gone, so this recover is useless for
this scenario (when enter EH,
all normal IOs are blocked actually, so it will cause normal IOs are
blocked one more minute which
user doesn't want to).

Right, it'd block other devices sharing the port.  Doesn't sas map
each ata device to its own port tho?


Yes, for ata devices connected with expander, all the devices share the 
same port.





Actually in sas_ata_hard_reset, there are two situations returned
-ENODEV which represent device is gone:
- LLDD directly returns -ENODEV through lldd_I_T_nexus_reset;
- It sends SMP DISCOVER to check local phy in smp_ata_check_ready,
and find it is gone;

So, if there are real consequences, we can definitely add a way to
short-circuit the recovery logic but let's do that by adding proper
signaling rathr than testing for driver type.


I am not familiar with ata recovery logic, and do you have idea about 
how to add a way to

short-circuit the recovery logic by adding proper signaling?



Thanks.

Re: [PATCH] scsi: ata: don't reset three times if device is offline for SAS host

[chenxiang (M)]

Hi Tejun,

在 2018/2/13 0:51, Tejun Heo 写道:

Hello,

On Wed, Jan 24, 2018 at 09:20:25PM +0800, chenxiang wrote:

In ata_eh_reset, it will reset three times at most for sata disk. For
some drivers through libsas, it calls sas_ata_hard_reset at last. When
device is gone, function sas_ata_hard_reset will return -ENODEV. But
it will still try to reset three times for offline device. This process
lasts a long time:

[11248.344323] ata13.00: status: { ERR }
[11248.344324] ata13.00: error: { ABRT }
[11248.344327] ata13: hard resetting link
[11248.503557] sas: ata: ex 500e004aaa1f phy02:U:A 
attached: (no device)
[11249.359524] sas: ata13: end_device-1:0:2: reset failed 
(errno=-19)[eta03d:19h:35m:17s]
[11249.365692] ata13: reset failed (errno=-19), retrying in 9 secs
[11258.451402] ata13: hard resetting linkKB/0KB/0KB /s] [0/0/0 iops][eta 
03d:22h:10m:48s]
[11259.411508] sas: ata13: end_device-1:0:2: reset failed (errno=-19)[eta 
03d:22h:28m:05s]
[11259.417683] ata13: reset failed (errno=-19), retrying in 10 secs
[11268.695401] ata13: hard resetting linkKB/0KB/0KB /s] [0/0/0 iops] [eta 
04d:01h:03m:37s]
[11269.699513] sas: ata13: end_device-1:0:2: reset failed (errno=-19)[eta 
04d:01h:20m:54s]
[11269.705689] ata13: reset failed (errno=-19), retrying in 34 secs
[11304.275393] ata13: hard resetting linkKB/0KB/0KB /s] [0/0/0 iops] [eta 
04d:11h:25m:43s]
[11305.283516] sas: ata13: end_device-1:0:2: reset failed (errno=-19)[eta 
04d:11h:43m:00s]
[11305.289692] ata13: reset failed, giving up
[11305.293785] ata13.00: disabled

Actually it is no need to reset three times for this scenario. So add
a check to avoid it.

I'm a bit reluctant in changing this per-driver.  Does this actually
hurt something?


For those drivers using libsas,  i think they have the same issue. It 
takes about 1 minute to
recover but actually device is gone, so this recover is useless for this 
scenario (when enter EH,
all normal IOs are blocked actually, so it will cause normal IOs are 
blocked one more minute which

user doesn't want to).
Actually in sas_ata_hard_reset, there are two situations returned 
-ENODEV which represent device is gone:

- LLDD directly returns -ENODEV through lldd_I_T_nexus_reset;
- It sends SMP DISCOVER to check local phy in smp_ata_check_ready, and 
find it is gone;



Thanks.

Re: [PATCH V2 8/8] scsi: hpsa: use blk_mq to solve irq affinity issue

[chenxiang (M)]

在 2018/2/5 23:20, Ming Lei 写道:

This patch uses .force_blk_mq to drive HPSA via SCSI_MQ, meantime maps
each reply queue to blk_mq's hw queue, then .queuecommand can always
choose the hw queue as the reply queue. And if no any online CPU is
mapped to one hw queue, request can't be submitted to this hw queue
at all, finally the irq affinity issue is solved.

Cc: Hannes Reinecke 
Cc: Arun Easi 
Cc: Omar Sandoval ,
Cc: "Martin K. Petersen" ,
Cc: James Bottomley ,
Cc: Christoph Hellwig ,
Cc: Don Brace 
Cc: Kashyap Desai 
Cc: Peter Rivera 
Cc: Paolo Bonzini 
Cc: Mike Snitzer 
Tested-by: Laurence Oberman 
Signed-off-by: Ming Lei 
---
  drivers/scsi/hpsa.c | 51 ++-
  1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 443eabf63a9f..e517a4c74a28 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -51,6 +51,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include "hpsa_cmd.h"
@@ -956,6 +957,13 @@ static struct device_attribute *hpsa_shost_attrs[] = {
  #define HPSA_NRESERVED_CMDS   (HPSA_CMDS_RESERVED_FOR_DRIVER +\
 HPSA_MAX_CONCURRENT_PASSTHRUS)
  
+static int hpsa_map_queues(struct Scsi_Host *shost)

+{
+struct ctlr_info *h = shost_to_hba(shost);
+
+return blk_mq_pci_map_queues(>tag_set, h->pdev);
+}
+


Hi Lei Ming,
It is okay to use blk_mq_pci_map_queue to solve automatic irq affinity 
issue when the first interrupt vector for queues is 0.
But if the first interrupt vector for queues is not 0,  we seems 
couldn't use blk_mq_pci_map_queue directly,
such as blk_mq_virtio_map_queues, it realizes a interface itself. Is it 
possible to provide a general interface for those

situations?



  static struct scsi_host_template hpsa_driver_template = {
.module = THIS_MODULE,
.name   = HPSA,
@@ -974,10 +982,13 @@ static struct scsi_host_template hpsa_driver_template = {
  #ifdef CONFIG_COMPAT
.compat_ioctl   = hpsa_compat_ioctl,
  #endif
+   .map_queues = hpsa_map_queues,
.sdev_attrs = hpsa_sdev_attrs,
.shost_attrs = hpsa_shost_attrs,
.max_sectors = 1024,
.no_write_same = 1,
+   .force_blk_mq = 1,
+   .host_tagset = 1,
  };
  
  static inline u32 next_command(struct ctlr_info *h, u8 q)

@@ -1045,11 +1056,7 @@ static void set_performant_mode(struct ctlr_info *h, 
struct CommandList *c,
c->busaddr |= 1 | (h->blockFetchTable[c->Header.SGList] << 1);
if (unlikely(!h->msix_vectors))
return;
-   if (likely(reply_queue == DEFAULT_REPLY_QUEUE))
-   c->Header.ReplyQueue =
-   raw_smp_processor_id() % h->nreply_queues;
-   else
-   c->Header.ReplyQueue = reply_queue % h->nreply_queues;
+   c->Header.ReplyQueue = reply_queue;
}
  }
  
@@ -1063,10 +1070,7 @@ static void set_ioaccel1_performant_mode(struct ctlr_info *h,

 * Tell the controller to post the reply to the queue for this
 * processor.  This seems to give the best I/O throughput.
 */
-   if (likely(reply_queue == DEFAULT_REPLY_QUEUE))
-   cp->ReplyQueue = smp_processor_id() % h->nreply_queues;
-   else
-   cp->ReplyQueue = reply_queue % h->nreply_queues;
+   cp->ReplyQueue = reply_queue;
/*
 * Set the bits in the address sent down to include:
 *  - performant mode bit (bit 0)
@@ -1087,10 +1091,7 @@ static void set_ioaccel2_tmf_performant_mode(struct 
ctlr_info *h,
/* Tell the controller to post the reply to the queue for this
 * processor.  This seems to give the best I/O throughput.
 */
-   if (likely(reply_queue == DEFAULT_REPLY_QUEUE))
-   cp->reply_queue = smp_processor_id() % h->nreply_queues;
-   else
-   cp->reply_queue = reply_queue % h->nreply_queues;
+   cp->reply_queue = reply_queue;
/* Set the bits in the address sent down to include:
 *  - performant mode bit not used in ioaccel mode 2
 *  - pull count (bits 0-3)
@@ -1109,10 +1110,7 @@ static void set_ioaccel2_performant_mode(struct 
ctlr_info *h,
 * Tell the controller to post the reply to the queue for this
 * processor.  This seems to give the best I/O throughput.
 */
-   if (likely(reply_queue == DEFAULT_REPLY_QUEUE))
-   cp->reply_queue = smp_processor_id() % h->nreply_queues;
-   else
-   cp->reply_queue = reply_queue % 

Re: [LSF/MM TOPIC] Improving Asynchronous SCSI Disk Probing

[chenxiang (M)]

在 2018/1/18 7:24, Bart Van Assche 写道:

When the SCSI scanning code discovers a SCSI device it calls the driver core
function device_add() to associate a SCSI ULD with the device. The driver
core invokes the probing function for the matching SCSI ULP, e.g. sd_probe().
In order to minimize the time needed to scan SCSI targets that have a large
number of LUNs, the SCSI disk driver scans LUNs asynchronously by starting
the actual probing work asynchronously from inside sd_probe()

An unfortunate aspect of how SCSI disk probing works today is that there is
unnecessary serialization between probing and removal activity. For a
possible approach of how to increase SCSI disk probing concurrency, see also
[PATCH] sd: Increase SCSI disk probing concurrency, linux-scsi mailing list,
December 2017 (https://www.spinics.net/lists/linux-scsi/msg115657.html).

A second unfortunate aspect of SCSI disk probing is that certain race
conditions in the block layer are hit if removal starts before asynchronous
probing has finished. This is because the driver core is unaware that the
SCSI disk code works asynchronously.

Additionally, the SCSI disk asynchronous probing approach is incompatible
with the power management code. The power management code calls
wait_for_device_probe() in the driver core to wait for device probing
activity to finish. wait_for_device_probe() however is unaware of the
asynchronous probing in the SCSI sd driver and hence doesn't wait for the
SCSI sd probing activity to finish.


I encountered and reported a similar issue which it seems there is a 
race between device_resume and removing disk:

https://www.spinics.net/lists/linux-scsi/msg115069.html



My proposal is to hold a session to discus potential solutions for
increasing SCSI disk probing concurrency in a way that is compatible with
the driver core and the power management subsystem.

Re: [PATCH v2 0/4] fix dma_unmap_sg() parameter in some scsi drivers

[chenxiang (M)]

Hi, does anyone notice and review this issue?

在 2018/1/4 10:36, chenxiang 写道:

According to Documentation/DMA-API.txt, all the parameters of dma_unmap_sg()
must be the same as those and passed in to the scatter/gather mapping API.
But in scsi drivers such as ibmscsi_tgt/iscsi/mvsas/pm8001, the 
parameter of dma_unmap_sg() is number of elements after mapping. So fix them.

Part of the document is as follows:

void
dma_unmap_sg(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction direction)

Unmap the previously mapped scatter/gather list.  All the parameters
must be the same as those and passed in to the scatter/gather mapping
API.

Note:  must be the number you passed in, *not* the number of
DMA address entries returned.

Chang Log:
v1 -> v2:
Split the patch into small patchset, and one patch per driver;

chenxiang (4):
   scsi: ibmvscsis: fix dma_unmap_sg() parameter
   scsi: isci: fix dma_unmap_sg() parameter
   scsi: mvsas: fix dma_unmap_sg() parameter
   scsi: pm8001: fix dma_unmap_sg() parameter

  drivers/scsi/ibmvscsi_tgt/libsrp.c | 6 --
  drivers/scsi/isci/request.c| 2 +-
  drivers/scsi/mvsas/mv_sas.c| 4 ++--
  drivers/scsi/pm8001/pm8001_sas.c   | 2 +-
  4 files changed, 8 insertions(+), 6 deletions(-)

liramandoz...@gmail.com

[M]

liramandoz...@gmail.com<>

Re: [PATCH v2 4/4] scsi: pm8001: fix dma_unmap_sg() parameter

[chenxiang (M)]

+cc Jack Wang 

在 2018/1/4 10:36, chenxiang 写道:

For function dma_unmap_sg(), the  parameter should be number of
elements in the scatterlist prior to the mapping, not after the mapping.
Fix this usage.

Cc: Jack Wang 
Cc: lindar_...@usish.com
Fixes: dbf9bfe6("[SCSI]pm8001: add SAS/SATA/HBA driver")
Signed-off-by: Xiang Chen 
---
  drivers/scsi/pm8001/pm8001_sas.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 947d601..576a0f0 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -466,7 +466,7 @@ static int pm8001_task_exec(struct sas_task *task,
dev_printk(KERN_ERR, pm8001_ha->dev, "pm8001 exec failed[%d]!\n", rc);
if (!sas_protocol_ata(t->task_proto))
if (n_elem)
-   dma_unmap_sg(pm8001_ha->dev, t->scatter, n_elem,
+   dma_unmap_sg(pm8001_ha->dev, t->scatter, t->num_scatter,
t->data_dir);
  out_done:
spin_unlock_irqrestore(_ha->lock, flags);

Re: [PATCH v1] libsas: remove private hex2bin() implementation

[chenxiang (M)]

在 2017/12/20 1:37, Andy Shevchenko 写道:

The function sas_parse_addr() could be easily substituted by hex2bin() which is
in kernel library code.

Cc: Christoph Hellwig 
Signed-off-by: Andy Shevchenko 


Tested-by: Xiang Chen 


---
  drivers/scsi/libsas/sas_scsi_host.c | 20 
  1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
b/drivers/scsi/libsas/sas_scsi_host.c
index 58476b728c57..626727207889 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -27,6 +27,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "sas_internal.h"
  
@@ -946,21 +947,6 @@ void sas_target_destroy(struct scsi_target *starget)

sas_put_device(found_dev);
  }
  
-static void sas_parse_addr(u8 *sas_addr, const char *p)

-{
-   int i;
-   for (i = 0; i < SAS_ADDR_SIZE; i++) {
-   u8 h, l;
-   if (!*p)
-   break;
-   h = isdigit(*p) ? *p-'0' : toupper(*p)-'A'+10;
-   p++;
-   l = isdigit(*p) ? *p-'0' : toupper(*p)-'A'+10;
-   p++;
-   sas_addr[i] = (h<<4) | l;
-   }
-}
-
  #define SAS_STRING_ADDR_SIZE  16
  
  int sas_request_addr(struct Scsi_Host *shost, u8 *addr)

@@ -977,7 +963,9 @@ int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
goto out;
}
  
-	sas_parse_addr(addr, fw->data);

+   res = hex2bin(addr, fw->data, strnlen(fw->data, SAS_ADDR_SIZE * 2) / 2);
+   if (res)
+   goto out;
  
  out:

release_firmware(fw);

Re: [PATCH] scsi: fix dma_unmap_sg() parameter in some drivers

[chenxiang (M)]

在 2018/1/2 18:51, John Garry 写道:

On 21/12/2017 08:15, chenxiang wrote:

For function dma_unmap_sg(), the  parameter should be number of
elements in the scatterlist prior to the mapping, not after the mapping.
So fix this usage in ibmvscsi_tgt/isci/mvsas/pm8001.



Hi chenxiang,

I think that it may be better to rewrite the series with a patch per 
driver, so that we can add specific "fixes" ID tag. Please also 
include "CC:" tag per patch, cc'ing the respective driver maintainer 
(if any).


Ok, i will split it with a patch per driver.



BTW, I guess that there are many more instances of this issue 
throughout the kernel...


Much appreciated,
John


Signed-off-by: Xiang Chen 
---
 drivers/scsi/ibmvscsi_tgt/libsrp.c | 6 --
 drivers/scsi/isci/request.c| 2 +-
 drivers/scsi/mvsas/mv_sas.c| 4 ++--
 drivers/scsi/pm8001/pm8001_sas.c   | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ibmvscsi_tgt/libsrp.c 
b/drivers/scsi/ibmvscsi_tgt/libsrp.c

index 5a4cc28..16054c0 100644
--- a/drivers/scsi/ibmvscsi_tgt/libsrp.c
+++ b/drivers/scsi/ibmvscsi_tgt/libsrp.c
@@ -193,7 +193,8 @@ static int srp_direct_data(struct ibmvscsis_cmd 
*cmd, struct srp_direct_buf *md,

 err = rdma_io(cmd, sg, nsg, md, 1, dir, len);

 if (dma_map)
-dma_unmap_sg(iue->target->dev, sg, nsg, DMA_BIDIRECTIONAL);
+dma_unmap_sg(iue->target->dev, sg, cmd->se_cmd.t_data_nents,
+DMA_BIDIRECTIONAL);

 return err;
 }
@@ -265,7 +266,8 @@ static int srp_indirect_data(struct ibmvscsis_cmd 
*cmd, struct srp_cmd *srp_cmd,

 err = rdma_io(cmd, sg, nsg, md, nmd, dir, len);

 if (dma_map)
-dma_unmap_sg(iue->target->dev, sg, nsg, DMA_BIDIRECTIONAL);
+dma_unmap_sg(iue->target->dev, sg, cmd->se_cmd.t_data_nents,
+DMA_BIDIRECTIONAL);

 free_mem:
 if (token && dma_map) {
diff --git a/drivers/scsi/isci/request.c b/drivers/scsi/isci/request.c
index ed197bc..225d947 100644
--- a/drivers/scsi/isci/request.c
+++ b/drivers/scsi/isci/request.c
@@ -2914,7 +2914,7 @@ static void 
isci_request_io_request_complete(struct isci_host *ihost,

  task->total_xfer_len, task->data_dir);
 else  /* unmap the sgl dma addresses */
 dma_unmap_sg(>pdev->dev, task->scatter,
- request->num_sg_entries, task->data_dir);
+ task->num_scatter, task->data_dir);
 break;
 case SAS_PROTOCOL_SMP: {
 struct scatterlist *sg = >smp_task.smp_req;
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index cff43bd..4b2cf36 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -848,7 +848,7 @@ static int mvs_task_prep(struct sas_task *task, 
struct mvs_info *mvi, int is_tmf

 dev_printk(KERN_ERR, mvi->dev, "mvsas prep failed[%d]!\n", rc);
 if (!sas_protocol_ata(task->task_proto))
 if (n_elem)
-dma_unmap_sg(mvi->dev, task->scatter, n_elem,
+dma_unmap_sg(mvi->dev, task->scatter, task->num_scatter,
  task->data_dir);
 prep_out:
 return rc;
@@ -899,7 +899,7 @@ static void mvs_slot_task_free(struct mvs_info 
*mvi, struct sas_task *task,

 if (!sas_protocol_ata(task->task_proto))
 if (slot->n_elem)
 dma_unmap_sg(mvi->dev, task->scatter,
- slot->n_elem, task->data_dir);
+ task->num_scatter, task->data_dir);

 switch (task->task_proto) {
 case SAS_PROTOCOL_SMP:
diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
b/drivers/scsi/pm8001/pm8001_sas.c

index 947d601..576a0f0 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -466,7 +466,7 @@ static int pm8001_task_exec(struct sas_task *task,
 dev_printk(KERN_ERR, pm8001_ha->dev, "pm8001 exec 
failed[%d]!\n", rc);

 if (!sas_protocol_ata(t->task_proto))
 if (n_elem)
-dma_unmap_sg(pm8001_ha->dev, t->scatter, n_elem,
+dma_unmap_sg(pm8001_ha->dev, t->scatter, t->num_scatter,
 t->data_dir);
 out_done:
 spin_unlock_irqrestore(_ha->lock, flags);





.

Re: [bug report] A race between device_resume and removing disk

[chenxiang (M)]

Ping...Does anyone has some idea about this issue?

在 2017/11/16 11:54, chenxiang (M) 写道:

Hi all,

When debugging suspend and resume of hisi_sas, I find a issue: use 
commands (echo freeze > /sys/power/state) to

suspend, after 5s system will be resumed as i enable TEST_DEVICES. But if
I plug one disks during suspend, system will be blocked all the time 
and it seems that there is a deadlock.


I suspect that when device resume, it will acquire dev->mutex 
(device_lock(dev)) during almost the
whole device resume process, but if remove a disk, it also tries to 
acquire dev->mutex when device_del.

So it causes a deadlock. Do you have any idea about this issue?

Follows are part of the log of normal print and exception print. In my 
environment, there are 11 disks on expander

(5 SATA disks and 6 SAS disks).

regards,
shawn


Normal log of suspend as follows:

[  189.789974] PM: suspend entry (s2idle)
[  189.798045] PM: Syncing filesystems ... done.
[  189.935324] Freezing user space processes ... (elapsed 0.009 
seconds) done.

[  189.955044] OOM killer disabled.
[  189.959111] Freezing remaining freezable tasks ... (elapsed 0.012 
seconds) done.

[  190.065399] sd 0:0:10:0: [sdk] Synchronizing SCSI cache
[  190.090948] sd 0:0:10:0: [sdk] Stopping disk
[  190.113157] sd 0:0:9:0: [sdj] Synchronizing SCSI cache
[  190.139116] sd 0:0:8:0: [sdi] Synchronizing SCSI cache
[  190.160252] sd 0:0:7:0: [sdh] Synchronizing SCSI cache
[  190.175968] sd 0:0:7:0: [sdh] Stopping disk
[  191.401325] sd 0:0:5:0: [sdf] Synchronizing SCSI cache
[  191.429529] sd 0:0:3:0: [sdd] Synchronizing SCSI cache
[  191.452532] sd 0:0:2:0: [sdc] Synchronizing SCSI cache
[  191.752248] sd 0:0:2:0: [sdc] Stopping disk
[  192.045536] sd 0:0:1:0: [sdb] Synchronizing SCSI cache
[  192.072078] sd 0:0:1:0: [sdb] Stopping disk
[  192.744386] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  192.760704] sd 0:0:0:0: [sda] Stopping disk
[  193.397060] hisi_sas_v3_suspend
[  194.587783] PM: suspend devices took 4.604 seconds
[  194.594688] PM: suspend debug: Waiting for 5 second(s).
[  199.687272] hisi_sas_v3_resume
[  199.740433] hisi_sas_v3_hw :74:02.0: phyup: phy0 link_rate=8
[  199.747979] hisi_sas_v3_hw :74:02.0: phyup: phy1 link_rate=8
[  199.754995] hisi_sas_v3_hw :74:02.0: phyup: phy2 link_rate=8
[  199.787405] hisi_sas_v3_hw :74:02.0: phyup: phy3 link_rate=8
[  199.820411] sas: sas_form_port: phy0 belongs to port0 already(4)!
[  199.831008] sas: sas_form_port: phy1 belongs to port0 already(4)!
[  199.839963] sas: sas_form_port: phy2 belongs to port0 already(4)!
[  199.849237] sas: sas_form_port: phy3 belongs to port0 already(4)!
[  200.366863] ata7: SATA link down (SStatus 0 SControl 300)
[  200.379220] ata6: SATA link down (SStatus 0 SControl 300)
[  200.828384] sas: broadcast received: 0
[  200.838602] sas: REVALIDATING DOMAIN on port 0, pid:1454
[  200.862876] sd 0:0:0:0: [sda] Starting disk
[  200.881098] sd 0:0:1:0: [sdb] Starting disk
[  200.891951] sd 0:0:2:0: [sdc] Starting disk
[  200.949758] sd 0:0:7:0: [sdh] Starting disk
[  200.990573] sd 0:0:10:0: [sdk] Starting disk
[  201.114636] sas: Expander phy change count has changed
[  201.188709] sas: ex 500e004aaa1f phy16 originated 
BROADCAST(CHANGE)

[  201.197145] sas: phy16 part of wide port with phy17
[  201.211154] sas: ex 500e004aaa1f phy 0x10 broadcast flutter
[  201.222900] sas: ex 500e004aaa1f phy17 originated 
BROADCAST(CHANGE)

[  201.231045] sas: phy17 part of wide port with phy16
[  201.244352] sas: ex 500e004aaa1f phy 0x11 broadcast flutter
[  201.258344] sas: ex 500e004aaa1f phy18 originated 
BROADCAST(CHANGE)

[  201.268344] sas: phy18 part of wide port with phy16
[  201.283102] sas: ex 500e004aaa1f phy 0x12 broadcast flutter
[  201.294168] sas: ex 500e004aaa1f phy19 originated 
BROADCAST(CHANGE)

[  201.302386] sas: phy19 part of wide port with phy16
[  201.315813] sas: ex 500e004aaa1f phy 0x13 broadcast flutter
[  201.346615] sas: done REVALIDATING DOMAIN on port 0, pid:1454, res 0x0
[  203.640641] PM: resume devices took 3.984 seconds
[  203.648996] OOM killer enabled.
[  203.655016] Restarting tasks ... done.
[  203.690494] PM: suspend exit

The log of removing disk when suspend is as follows:

[44750.653222] PM: suspend entry (s2idle)
[44750.658991] PM: Syncing filesystems ... done.
[44750.699868] Freezing user space processes ... (elapsed 0.015 
seconds) done.

[44750.725734] OOM killer disabled.
[44750.729711] Freezing remaining freezable tasks ... (elapsed 0.018 
seconds) done.

[44750.820672] sd 0:0:13:0: [sdk] Synchronizing SCSI cache
[44750.844056] sd 0:0:13:0: [sdk] Stopping disk
[44750.917433] sd 0:0:9:0: [sdj] Synchronizing SCSI cache
[44750.949032] sd 0:0:8:0: [sdi] Synchronizing SCSI cache
[44750.972730] sd 0:0:7:0: [sdh] Synchronizing SCSI cache
[44750.989813] sd 0:0:7:0: [sdh] Stopping disk
[44752.207539] sd 0:0:5:0: [sdf] Synchronizing SCSI cache
[44752.239773] sd 0:0:3:0: [sdd] Synchroni

[bug report] A race between device_resume and removing disk

[chenxiang (M)]

Hi all,

When debugging suspend and resume of hisi_sas, I find a issue: use 
commands (echo freeze > /sys/power/state) to

suspend, after 5s system will be resumed as i enable TEST_DEVICES. But if
I plug one disks during suspend, system will be blocked all the time and 
it seems that there is a deadlock.


I suspect that when device resume, it will acquire dev->mutex 
(device_lock(dev)) during almost the
whole device resume process, but if remove a disk, it also tries to 
acquire dev->mutex when device_del.

So it causes a deadlock. Do you have any idea about this issue?

Follows are part of the log of normal print and exception print. In my 
environment, there are 11 disks on expander

(5 SATA disks and 6 SAS disks).

regards,
shawn


Normal log of suspend as follows:

[  189.789974] PM: suspend entry (s2idle)
[  189.798045] PM: Syncing filesystems ... done.
[  189.935324] Freezing user space processes ... (elapsed 0.009 seconds) 
done.

[  189.955044] OOM killer disabled.
[  189.959111] Freezing remaining freezable tasks ... (elapsed 0.012 
seconds) done.

[  190.065399] sd 0:0:10:0: [sdk] Synchronizing SCSI cache
[  190.090948] sd 0:0:10:0: [sdk] Stopping disk
[  190.113157] sd 0:0:9:0: [sdj] Synchronizing SCSI cache
[  190.139116] sd 0:0:8:0: [sdi] Synchronizing SCSI cache
[  190.160252] sd 0:0:7:0: [sdh] Synchronizing SCSI cache
[  190.175968] sd 0:0:7:0: [sdh] Stopping disk
[  191.401325] sd 0:0:5:0: [sdf] Synchronizing SCSI cache
[  191.429529] sd 0:0:3:0: [sdd] Synchronizing SCSI cache
[  191.452532] sd 0:0:2:0: [sdc] Synchronizing SCSI cache
[  191.752248] sd 0:0:2:0: [sdc] Stopping disk
[  192.045536] sd 0:0:1:0: [sdb] Synchronizing SCSI cache
[  192.072078] sd 0:0:1:0: [sdb] Stopping disk
[  192.744386] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  192.760704] sd 0:0:0:0: [sda] Stopping disk
[  193.397060] hisi_sas_v3_suspend
[  194.587783] PM: suspend devices took 4.604 seconds
[  194.594688] PM: suspend debug: Waiting for 5 second(s).
[  199.687272] hisi_sas_v3_resume
[  199.740433] hisi_sas_v3_hw :74:02.0: phyup: phy0 link_rate=8
[  199.747979] hisi_sas_v3_hw :74:02.0: phyup: phy1 link_rate=8
[  199.754995] hisi_sas_v3_hw :74:02.0: phyup: phy2 link_rate=8
[  199.787405] hisi_sas_v3_hw :74:02.0: phyup: phy3 link_rate=8
[  199.820411] sas: sas_form_port: phy0 belongs to port0 already(4)!
[  199.831008] sas: sas_form_port: phy1 belongs to port0 already(4)!
[  199.839963] sas: sas_form_port: phy2 belongs to port0 already(4)!
[  199.849237] sas: sas_form_port: phy3 belongs to port0 already(4)!
[  200.366863] ata7: SATA link down (SStatus 0 SControl 300)
[  200.379220] ata6: SATA link down (SStatus 0 SControl 300)
[  200.828384] sas: broadcast received: 0
[  200.838602] sas: REVALIDATING DOMAIN on port 0, pid:1454
[  200.862876] sd 0:0:0:0: [sda] Starting disk
[  200.881098] sd 0:0:1:0: [sdb] Starting disk
[  200.891951] sd 0:0:2:0: [sdc] Starting disk
[  200.949758] sd 0:0:7:0: [sdh] Starting disk
[  200.990573] sd 0:0:10:0: [sdk] Starting disk
[  201.114636] sas: Expander phy change count has changed
[  201.188709] sas: ex 500e004aaa1f phy16 originated BROADCAST(CHANGE)
[  201.197145] sas: phy16 part of wide port with phy17
[  201.211154] sas: ex 500e004aaa1f phy 0x10 broadcast flutter
[  201.222900] sas: ex 500e004aaa1f phy17 originated BROADCAST(CHANGE)
[  201.231045] sas: phy17 part of wide port with phy16
[  201.244352] sas: ex 500e004aaa1f phy 0x11 broadcast flutter
[  201.258344] sas: ex 500e004aaa1f phy18 originated BROADCAST(CHANGE)
[  201.268344] sas: phy18 part of wide port with phy16
[  201.283102] sas: ex 500e004aaa1f phy 0x12 broadcast flutter
[  201.294168] sas: ex 500e004aaa1f phy19 originated BROADCAST(CHANGE)
[  201.302386] sas: phy19 part of wide port with phy16
[  201.315813] sas: ex 500e004aaa1f phy 0x13 broadcast flutter
[  201.346615] sas: done REVALIDATING DOMAIN on port 0, pid:1454, res 0x0
[  203.640641] PM: resume devices took 3.984 seconds
[  203.648996] OOM killer enabled.
[  203.655016] Restarting tasks ... done.
[  203.690494] PM: suspend exit

The log of removing disk when suspend is as follows:

[44750.653222] PM: suspend entry (s2idle)
[44750.658991] PM: Syncing filesystems ... done.
[44750.699868] Freezing user space processes ... (elapsed 0.015 seconds) 
done.

[44750.725734] OOM killer disabled.
[44750.729711] Freezing remaining freezable tasks ... (elapsed 0.018 
seconds) done.

[44750.820672] sd 0:0:13:0: [sdk] Synchronizing SCSI cache
[44750.844056] sd 0:0:13:0: [sdk] Stopping disk
[44750.917433] sd 0:0:9:0: [sdj] Synchronizing SCSI cache
[44750.949032] sd 0:0:8:0: [sdi] Synchronizing SCSI cache
[44750.972730] sd 0:0:7:0: [sdh] Synchronizing SCSI cache
[44750.989813] sd 0:0:7:0: [sdh] Stopping disk
[44752.207539] sd 0:0:5:0: [sdf] Synchronizing SCSI cache
[44752.239773] sd 0:0:3:0: [sdd] Synchronizing SCSI cache
[44752.264202] sd 0:0:2:0: [sdc] Synchronizing SCSI cache
[44752.283091] sd 0:0:2:0: [sdc] 

Hello Dear...

[M,Shakour Rosarita]

Hello Dear...

I know that this message will come to you as a surprise. I hoped that
you will not expose or betray this trust and confident that I am about
to repose on you, my name is M, Shakour Rosarita. I am 19 years old
Girl, female, from Tartu Syria, (never married) 61 kg, white in
complexion, and I am currently living in the refugee camp here in
Abidjan Ivory Coast, My late beloved father M,Shakour Hassin was a
renowned businessman and owner of Natour Petrol Station in Syria, he
was killed in a stampede riot in Tartu, Syria.
When I got the news about my parents. I fled to a nearby country
Jordan before I joined a ferry to Africa and came to Abidjan capital
city Ivory Coast West Africa find safety here.
I came in 2015 to Abidjan and I now live in refugee camps here as
refugees are allowed freely to enter here without, My late father
deposited (US$6.200.000.00m) My late father kept the money at the bank
of Africa, I want you to help me transfer the money to your hand so
that you will help me bring me into your country for my continue
education.

I sent my pictures here for you to see. Who I am seriously looking for
a good-person in my life, so I want to hear from you soon and learn
more about you.

I am an open-minded and friendly girl to share a good time with you
and have fun and enjoy on the go, bird watching, the rest of our
lives. My Hobbies, tourism books, dance, music, soccer, tennis,
swimming and cinema.
I would just think about you, including your dose and doesn’t .I
believe we will do well together, and live like one family.
Thank you and God bless you, for you in your promise to help me here,
looking forward to your reply by the grace of God and have a good day.
I hope you send me your photos as well? I await your next reply in
faith please reply through my private email at
(mshakourrosarit...@gmail.com):
Thanks.
Ms Rosarita

Hello Dear...

[M,Shakour Rosarita]

Hello Dear...

I know that this message will come to you as a surprise. I hoped that
you will not expose or betray this trust and confident that I am about
to repose on you, my name is M, Shakour Rosarita. I am 19 years old
Girl, female, from Tartu Syria, (never married) 61 kg, white in
complexion, and I am currently living in the refugee camp here in
Abidjan Ivory Coast, My late beloved father M,Shakour Hassin was a
renowned businessman and owner of Natour Petrol Station in Syria, he
was killed in a stampede riot in Tartu, Syria.
When I got the news about my parents. I fled to a nearby country
Jordan before I joined a ferry to Africa and came to Abidjan capital
city Ivory Coast West Africa find safety here.
I came in 2015 to Abidjan and I now live in refugee camps here as
refugees are allowed freely to enter here without, My late father
deposited (US$6.200.000.00m) My late father kept the money at the bank
of Africa, I want you to help me transfer the money to your hand so
that you will help me bring me into your country for my continue
education.

I sent my pictures here for you to see. Who I am seriously looking for
a good-person in my life, so I want to hear from you soon and learn
more about you.

I am an open-minded and friendly girl to share a good time with you
and have fun and enjoy on the go, bird watching, the rest of our
lives. My Hobbies, tourism books, dance, music, soccer, tennis,
swimming and cinema.
I would just think about you, including your dose and doesn’t .I
believe we will do well together, and live like one family.
Thank you and God bless you, for you in your promise to help me here,
looking forward to your reply by the grace of God and have a good day.
I hope you send me your photos as well? I await your next reply in
faith please reply through my private email at
(mshakourrosarit...@gmail.com):
Thanks.
Ms Rosarita

RE: [PATCH v4 3/3] Rework handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

> Introduce struct scsi_vpd for the VPD page length, data and the
> RCU head that will be used to free the VPD data. Use kfree_rcu()
> instead of kfree() to free VPD data. Move the VPD buffer pointer
> check inside the RCU read lock in the sysfs code. Only annotate
> pointers that are shared across threads with __rcu. Use
> rcu_dereference() when dereferencing an RCU pointer. This patch
> suppresses about twenty sparse complaints about the vpd_pg8[03]
> pointers. This patch also fixes a race condition, namely that
> updating of the VPD pointers and length variables in struct
> scsi_device was not atomic with reference to the code reading
> these variables. See also "Does the update code tolerate concurrent
> accesses?" in Documentation/RCU/checklist.txt.
> 
> Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Bart Van Assche 
> Acked-by: Hannes Reinecke 
> Cc: Christoph Hellwig 
> Cc: Johannes Thumshirn 
> Cc: Shane Seymour 
> ---
>  drivers/scsi/scsi.c| 44 ++--
>  drivers/scsi/scsi_lib.c| 16 
>  drivers/scsi/scsi_sysfs.c  | 29 -
>  include/scsi/scsi_device.h | 18 ++
>  4 files changed, 60 insertions(+), 47 deletions(-)

Reviewed-by: Shane Seymour 

RE: [PATCH v4 2/3] Rework the code for caching Vital Product Data (VPD)

[Seymour, Shane M]

> Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
> functions. The only functional change in this patch is that if
> updating page 0x80 fails that it is attempted to update page 0x83.
> 
> Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
> Acked-by: Hannes Reinecke <h...@suse.de>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Johannes Thumshirn <jthumsh...@suse.de>
> Cc: Shane M Seymour <shane.seym...@hpe.com>
> ---
>  drivers/scsi/scsi.c | 144 --
> --
>  1 file changed, 66 insertions(+), 78 deletions(-)

Reviewed-by: Shane Seymour <shane.seym...@hpe.com>

RE: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

Hi Bart,

Comments inline below about the show_vpd_##_page macro.

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@wdc.com]
> Sent: Saturday, August 26, 2017 6:36 AM
> To: Martin K . Petersen <martin.peter...@oracle.com>; James E . J .
> Bottomley <j...@linux.vnet.ibm.com>
> Cc: linux-scsi@vger.kernel.org; Bart Van Assche <bart.vanass...@wdc.com>;
> Christoph Hellwig <h...@lst.de>; Hannes Reinecke <h...@suse.de>;
> Johannes Thumshirn <jthumsh...@suse.de>; Seymour, Shane M
> <shane.seym...@hpe.com>
> Subject: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]
> 
> Introduce struct scsi_vpd for the VPD page length, data and the
> RCU head that will be used to free the VPD data. Use kfree_rcu()
> instead of kfree() to free VPD data. Only annotate pointers that
> are shared across threads with __rcu. Use rcu_dereference() when
> dereferencing an RCU pointer. This patch suppresses about twenty
> sparse complaints about the vpd_pg8[03] pointers.
> 
> Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: Johannes Thumshirn <jthumsh...@suse.de>
> Cc: Shane Seymour <shane.seym...@hpe.com>
> ---
>  drivers/scsi/scsi.c| 48 
> +-
>  drivers/scsi/scsi_lib.c| 16 
>  drivers/scsi/scsi_sysfs.c  | 26 +++--
>  include/scsi/scsi_device.h | 18 +
>  4 files changed, 64 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 775f609f89e2..1cf3aef0de8a 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -415,22 +415,20 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>   * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
>   * @sdev: The device to ask
>   * @page: Which Vital Product Data to return
> - * @len: Upon success, the VPD length will be stored in *@len.
>   *
>   * Returns %NULL upon failure.
>   */
> -static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
> -int *len)
> +static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
>  {
> - unsigned char *vpd_buf;
> + struct scsi_vpd *vpd_buf;
>   int vpd_len = SCSI_VPD_PG_LEN, result;
> 
>  retry_pg:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> + vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL);
>   if (!vpd_buf)
>   return NULL;
> 
> - result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
> + result = scsi_vpd_inquiry(sdev, vpd_buf->data, page, vpd_len);
>   if (result < 0) {
>   kfree(vpd_buf);
>   return NULL;
> @@ -441,7 +439,7 @@ static unsigned char *scsi_get_vpd_buf(struct
> scsi_device *sdev, u8 page,
>   goto retry_pg;
>   }
> 
> - *len = result;
> + vpd_buf->len = result;
> 
>   return vpd_buf;
>  }
> @@ -458,52 +456,50 @@ static unsigned char *scsi_get_vpd_buf(struct
> scsi_device *sdev, u8 page,
>  void scsi_attach_vpd(struct scsi_device *sdev)
>  {
>   int i;
> - int vpd_len;
>   int pg80_supported = 0;
>   int pg83_supported = 0;
> - unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> + struct scsi_vpd *vpd_buf, *orig_vpd_buf;
> 
>   if (!scsi_device_supports_vpd(sdev))
>   return;
> 
>   /* Ask for all the pages supported by this device */
> - vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
> + vpd_buf = scsi_get_vpd_buf(sdev, 0);
>   if (!vpd_buf)
>   return;
> 
> - for (i = 4; i < vpd_len; i++) {
> - if (vpd_buf[i] == 0x80)
> + for (i = 4; i < vpd_buf->len; i++) {
> + if (vpd_buf->data[i] == 0x80)
>   pg80_supported = 1;
> - if (vpd_buf[i] == 0x83)
> + if (vpd_buf->data[i] == 0x83)
>   pg83_supported = 1;
>   }
>   kfree(vpd_buf);
> 
>   if (pg80_supported) {
> - vpd_buf = scsi_get_vpd_buf(sdev, 0x80, _len);
> + vpd_buf = scsi_get_vpd_buf(sdev, 0x80);
> 
>   mutex_lock(>inquiry_mutex);
> - orig_vpd_buf = sdev->vpd_pg80;
> - sdev->vpd_pg80_len = vpd_len;
> + orig_vpd_buf = rcu_dereference_protected(sdev-
> >vpd_pg80,
> + lockdep_is_held(
> >inquiry_mutex));
>   rcu_assign_pointer(sd

RE: [PATCH 1/2] Introduce scsi_get_vpd_buf()

[Seymour, Shane M]

Reviewed-by: Shane Seymour <shane.seym...@hpe.com>

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@wdc.com]
> Sent: Saturday, August 26, 2017 6:36 AM
> To: Martin K . Petersen <martin.peter...@oracle.com>; James E . J .
> Bottomley <j...@linux.vnet.ibm.com>
> Cc: linux-scsi@vger.kernel.org; Bart Van Assche <bart.vanass...@wdc.com>;
> Christoph Hellwig <h...@lst.de>; Hannes Reinecke <h...@suse.de>;
> Johannes Thumshirn <jthumsh...@suse.de>; Seymour, Shane M
> <shane.seym...@hpe.com>
> Subject: [PATCH 1/2] Introduce scsi_get_vpd_buf()
> 
> This patch does not change any functionality.
> 
> Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Hannes Reinecke <h...@suse.de>
> Cc: Johannes Thumshirn <jthumsh...@suse.de>
> Cc: Shane M Seymour <shane.seym...@hpe.com>
> ---
>  drivers/scsi/scsi.c | 96 +--
> --
>  1 file changed, 45 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3d38c6d463b8..775f609f89e2 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -411,6 +411,41 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8
> page, unsigned char *buf,
>  }
>  EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> 
> +/**
> + * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
> + * @sdev: The device to ask
> + * @page: Which Vital Product Data to return
> + * @len: Upon success, the VPD length will be stored in *@len.
> + *
> + * Returns %NULL upon failure.
> + */
> +static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
> +int *len)
> +{
> + unsigned char *vpd_buf;
> + int vpd_len = SCSI_VPD_PG_LEN, result;
> +
> +retry_pg:
> + vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> + if (!vpd_buf)
> + return NULL;
> +
> + result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
> + if (result < 0) {
> + kfree(vpd_buf);
> + return NULL;
> + }
> + if (result > vpd_len) {
> + vpd_len = result;
> + kfree(vpd_buf);
> + goto retry_pg;
> + }
> +
> + *len = result;
> +
> + return vpd_buf;
> +}
> +
>  /**
>   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
>   * @sdev: The device to ask
> @@ -422,8 +457,8 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>   */
>  void scsi_attach_vpd(struct scsi_device *sdev)
>  {
> - int result, i;
> - int vpd_len = SCSI_VPD_PG_LEN;
> + int i;
> + int vpd_len;
>   int pg80_supported = 0;
>   int pg83_supported = 0;
>   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> @@ -431,51 +466,25 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>   if (!scsi_device_supports_vpd(sdev))
>   return;
> 
> -retry_pg0:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> + /* Ask for all the pages supported by this device */
> + vpd_buf = scsi_get_vpd_buf(sdev, 0, _len);
>   if (!vpd_buf)
>   return;
> 
> - /* Ask for all the pages supported by this device */
> - result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
> - if (result < 0) {
> - kfree(vpd_buf);
> - return;
> - }
> - if (result > vpd_len) {
> - vpd_len = result;
> - kfree(vpd_buf);
> - goto retry_pg0;
> - }
> -
> - for (i = 4; i < result; i++) {
> + for (i = 4; i < vpd_len; i++) {
>   if (vpd_buf[i] == 0x80)
>   pg80_supported = 1;
>   if (vpd_buf[i] == 0x83)
>   pg83_supported = 1;
>   }
>   kfree(vpd_buf);
> - vpd_len = SCSI_VPD_PG_LEN;
> 
>   if (pg80_supported) {
> -retry_pg80:
> - vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> - if (!vpd_buf)
> - return;
> -
> - result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> - if (result < 0) {
> - kfree(vpd_buf);
> - return;
> - }
> - if (result > vpd_len) {
> - vpd_len = result;
> - kfree(vpd_buf);
> - goto retry_pg80;
> - }
> + vpd_buf = scsi_get_vpd_buf(sdev, 0x80, _len);
> +
>   mutex_lock(>inquiry_mutex);
>   orig_vpd_buf = sdev->vpd_pg80;
&

RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

> Hello Shane,
> 
> You have either misinterpret my statement or the SCSI VPD handling code. If
> you have a look at the SCSI VPD handling code you will see that an
> rcu_read_lock() /
> rcu_read_unlock() pair is sufficient to prevent that the VPD buffer
> rcu_dereference() points at is being modified as long as the RCU read lock is
> held, at least if
> rcu_dereference() is only called once. The update side namely does not
> modify the VPD buffer after the pointer to that buffer has been published.

Hi Bart,

I'm pretty sure I understood the code. My main point was that the only thing 
that RCU guarantees with code in between rcu_read_lock()/rcu_read_unlock() if 
you dereference an RCU pointer using rcu_dereference() is that the memory that 
the pointer returned points to won't be freed and will be valid regardless of 
if you get the new or old pointer. Anything related to the actual contents of 
what the pointer points to is code specific in regard to what happens to it.

As Christoph pointed out (which I failed to consider fully) by doing a direct 
kfree in scsi_device_dev_release_usercontext() without a call to 
synchronize_rcu() if you had some code that that got one of the RCU pointers in 
a read-side critical section and for some reason 
scsi_device_dev_release_usercontext() got called you could now have an invalid 
pointer when RCU makes a guarantee that it must be valid. That's what I believe 
Christoph was discussing in his reply to my email.

> Switching to kfree_rcu() requires more changes because all unsigned char
> pointers to VPD data have to be converted into pointers to a structure that
> contains the VPD data and the RCU head. Anyway, I will convert the kfree()
> calls to RCU pointers into kfree_rcu() pointers.
> 

Thanks, I appreciate you taking the comments onboard. I've been looking at the 
new patches today and should post something back by the end of my day today.

Shane

RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@wdc.com]
> Sent: Friday, August 25, 2017 2:54 AM
> To: h...@lst.de
> Cc: j...@linux.vnet.ibm.com; linux-scsi@vger.kernel.org; h...@suse.de;
> jthumsh...@suse.de; martin.peter...@oracle.com; Seymour, Shane M
> <shane.seym...@hpe.com>
> Subject: Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
> 
> On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote:
> > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote:
> > > Only annotate pointers that are shared across threads with __rcu.
> > > Use rcu_dereference() when dereferencing an RCU pointer. Protect
> > > also the RCU pointer dereferences when freeing RCU pointers. This
> > > patch suppresses about twenty sparse complaints about the
> > > vpd_pg8[03] pointers.
> >
> > Shouldn't the kfrees be kfree_rcu?  or where else is the rcu
> > protection for them?
> 
> Hello Christoph,
> 

Hi Bart,

> My understanding of the SCSI VPD code is as follows:
> * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread
>   updates a VPD buffer while it is being read.

My understanding is that it doesn't do that - you can update an RCU pointer 
with rcu_assign_pointer() after someone has called rcu_read_lock() and before 
they call rcu_read_unlock(). 

What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical 
section when accessing an RCU data item. If you have 2 CPUs in a read-side 
critical section and a 3rd CPU replacing the pointer using rcu_assign_pointer() 
one CPU could potentially end up with the old pointer and the other one with 
the new pointer or both old or both new (the only guarantee you have is that 
the pointer won't be partially updated with bits of old and the new pointer). 
To free the old pointer directly you have to call synchronize_rcu() after which 
you can call kfree() or if you don't call synchronize_rcu() you have to use a 
delayed freeing mechanism like kfree_rcu() so you can guarantee that the old 
pointer is still valid while used in a read-side critical section. Using 
something like kfree_rcu() means that you also don’t have to wait like I 
believe you can do if you call synchronize_rcu() since you could be forced to 
wait for a RCU grace period to end before you can call kfree().

So what they really do is ensure that someone updating the pointer can't free 
the old pointer (in case someone is using it) until everyone has left their 
read-side critical section (if the read-side critical section started before 
synchronize_rcu() was called).

You'll find a good example here that uses synchronize_rcu():

https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt

Search for " WHAT ARE SOME EXAMPLE USES OF CORE RCU API?" - it has a lot of 
other information about RCU.

> * All code that either updates or reads a VPD buffer holds a reference on
>   the SCSI device that buffer is associated with. That is why I think it is
>   not needed to use kfree_rcu() in scsi_device_dev_release_usercontext().

The only counter argument I'd put up into why it shouldn't be done the way you 
want to is that if someone else sees that code and doesn't understand the 
context and can't guarantee similar to this situation where all references to 
the structure should have already been dropped and think that it's ok to 
directly kfree something returned from rcu_dereference() when it's something 
that they really shouldn't do. The only real difference is that kfree_rcu will 
return the memory for reuse when RCU processing gets done in softirq and what 
you're doing will do it immediately. It's easier to use kfree_rcu() from what I 
can see.

It is possible I may be being too picky (it's a personal failing sometimes) but 
is it really that a large overhead to free the RCU pointers in a way that RCU 
pointers are expected to work even if the pointers shouldn't be accessible to 
anything?

Shane

> 
> Bart.
> 

Ahoj....

[K. M Leung]

Ahoj.

Dobry vecer a jak se mas? Jen rychly jedno, je tu oficialni prilezitosti 
bych chtel diskutovat s vami soukrome.


Ocenil bych vasi rychlou reakci tady na mem osobnim soukromeho e-mailu 
nize pro dalsi komunikaci.


S pratelskym pozdravem,
Paní Ko May Leung
email: kleung...@gmail.com
Místopredseda, Managing Director
a vykonny reditel Chong Hing Bank Limited
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ahoj....

[Ko M. Leung]

Ahoj.

Dobry vecer a jak se mas? Jen rychly jedno, je tu oficialni prilezitosti 
bych chtel diskutovat s vami soukrome.


Ocenil bych vasi rychlou reakci tady na mem osobnim soukromeho e-mailu 
nize pro dalsi komunikaci.


S pratelskym pozdravem,
Paní Ko May Leung
email: kmyln...@gmail.com
Místopredseda, Managing Director
a vykonny reditel Chong Hing Bank Limited

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] scsi: ufs: tc-dwc-g210 driver fix for 20-bit RMMI

[Manjunath M B]

The code was checking on PA_CONNECTEDRXLANES and
PA_CONNECTEDTXLANES attributes to program the Lane#1
attributes. The correct attributes are PA_AVAILRXDATALANES and
PA_AVAILTXDATALANES respectively.

Signed-off-by: Manjunath M B <manj...@synopsys.com>
---
 drivers/scsi/ufs/tc-dwc-g210.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/tc-dwc-g210.c b/drivers/scsi/ufs/tc-dwc-g210.c
index d6c5388..70db6d9 100644
--- a/drivers/scsi/ufs/tc-dwc-g210.c
+++ b/drivers/scsi/ufs/tc-dwc-g210.c
@@ -187,9 +187,9 @@ static int tc_dwc_g210_setup_20bit_rmmi_lane1(struct 
ufs_hba *hba)
};
 
/* Get the available lane count */
-   ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDRXDATALANES),
+   ufshcd_dme_get(hba, UIC_ARG_MIB(PA_AVAILRXDATALANES),
_rx_lanes);
-   ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
+   ufshcd_dme_get(hba, UIC_ARG_MIB(PA_AVAILTXDATALANES),
_tx_lanes);
 
if (connected_tx_lanes == 2) {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] qlogicpti: Return correct error code

[Seymour, Shane M]

Reviewed-by: Shane Seymour 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] scsi_transport_fc: implement 'disable_target_scan' module parameter

[Seymour, Shane M]

Reviewed-by: Shane Seymour 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] scsi_transport_fc: Implement 'async_user_scan' module parameter

[Seymour, Shane M]

Reviewed-by: Shane Seymour 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] snic: correctly check for array overrun on overly long version number

[Seymour, Shane M]

Reviewed-by: Shane Seymour 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/2][RESEND] scsi_transport_fc: LUN masking

[Seymour, Shane M]

> But this has nothing to do with the patchset, right?

Correct.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/2][RESEND] scsi_transport_fc: LUN masking

[Seymour, Shane M]

Hi Hannes,

How do you know that a request for an async scan is complete (I'm assuming that 
you get add or change udev events)? Assuming that someone has manually started 
a scan on something (e.g. some newly presented devices after boot) and all 
scans are going to be async how do you when it is complete rather than waiting 
in a work queue? An example may be a sysfs file that contains unscanned, 
pending, scanning, scanned so you know when it's complete at the appropriate 
level in sysfs (the hba and the rports) so you know when can continue if you're 
polling the status (e.g. checking as part of system admin work with newly 
presented rports so you can then do something with them).

Thanks
Shane

> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Hannes Reinecke
> Sent: Monday, February 22, 2016 6:51 PM
> To: Martin K . Petersen
> Cc: Christoph Hellwig; James Bottomley; Johannes Thumshirn; linux-
> s...@vger.kernel.org; Hannes Reinecke
> Subject: [PATCH 0/2][RESEND] scsi_transport_fc: LUN masking
> 
> Hi all,
> 
> having been subjected to the pain of trying to bootstrap a really large
> machine with systemd I decided to implement LUN masking in
> scsi_transport_fc.
> The principle is simple: disallow the automated LUN scanning when
> discovering a rport, and create udev rules which selectively enable individual
> LUNs by echoing the relevant values in the 'scan'
> attribute of the SCSI host.
> With that I'm able to boot an arbitrary large machine without running into any
> udev or systemd imposed timeout.
> To _disable_ LUN masking and restoring the original behaviour I've noticed
> that the 'scan' sysfs attribute is actually synchronous, ie the calling 
> process
> will be blocked until the entire LUN scan is completed.
> So I've added another module parameter 'async_user_scan' to move the
> scanning onto the existing scan workqueue, and unblock the calling process.
> 
> As usual, comments and reviews are welcome.
> 
> Hannes Reinecke (2):
>   scsi_transport_fc: implement 'disable_target_scan' module parameter
>   scsi_transport_fc: Implement 'async_user_scan' module parameter
> 
>  drivers/scsi/scsi_transport_fc.c | 47
> +---
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> --
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

 changes are correct and needed and you want to add those two changes 
please do so. I've been looking very carefully at the code changes as part of 
the testing effort and happy to add any combination of reviewed-by and 
tested-by to the patch. If required feel free to change it as needed. I'll 
retest a consolidated version of the patch if you add the changes and send it 
out.

Laurence, when you get your LTO drive test without my changes in to make sure 
you see the same issue and then if you do test with the changes I added above. 
That's just to double check that there's nothing wrong with my setup and the 
code changes are required.

Thanks
Shane
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

Hi Kai,

> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of "Kai Mäkisara (Kolumbus)"
> Sent: Tuesday, February 02, 2016 5:43 AM
> To: Seymour, Shane M
> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: Re: What partition should the MTMKPART argument specify? Was:
> Re: st driver doesn't seem to grok LTO partitioning
> 
> 
> > On 1.2.2016, at 8.31, Seymour, Shane M <shane.seym...@hpe.com>
> wrote:
> >
> > Hi Kai,
> >
> > Thanks for the changes the HPE DAT72 DDS5 drive now works as expected:
> >
> Good. Thanks for testing.
> 
> ...
> >
> > I'm asking around again one final time to see if I can lay my hands on a 
> > LTO5
> or greater drive so I can test LTO partitioning as well.
> >
> > The only other thing I can think of (I'm not sure if this is an improvement 
> > or
> not) is if bp[pgo + PP_OFF_MAX_ADD_PARTS] + bp[pgo +
> PP_OFF_NBR_ADD_PARTS] (max.parts and nbr_parts in the debug message)
> is zero just return -EINVAL unless you know of any take drives that report
> them both as 0 but can be partitioned? That is after this:
> >
> >DEBC_printk(STp, "psd_cnt %d, max.parts %d, nbr_parts %d\n",
> >psd_cnt, bp[pgo + PP_OFF_MAX_ADD_PARTS],
> >bp[pgo + PP_OFF_NBR_ADD_PARTS]);
> >
> > add (and also turn off the can-partitions option):
> >
> > if ((bp[pgo + PP_OFF_MAX_ADD_PARTS] + bp[pgo +
> PP_OFF_NBR_ADD_PARTS]) == 0) {
> > DEBC_printk(STp, "Drive not partitionable -
> max.parts+nbr_parts is 0\n");
> > STp->can_partitions = 0;
> > return -EINVAL;
> > }
> >
> > I'm not especially fussed if you don't want to add that though.
> >
> I thought about a test like this (only test maximum number) but decided not
> to add it. The reason was that I did not want to change anything that has
> worked before. I quite trust that the current drives return sense data instead
> of crashing and the end result for the user would be the same. However, one
> can argue that returning EINVAL is better than EIO but does the user notice?
> If the common opinion is that a test like this should be added, I am not
> against it. It can be added to the code for SCSI >=3 where it does not risk
> anything for the old drives.
> 
> IMHO, can_partitions should not be cleared based on the test. For example,
> trying to partition a LTO-4 tape in a LTO-5 drive should not disable 
> partitioning.
> (The mode page should return zero as maximum number of partitions when
> a LTO-4 tape is inserted.)

No problem, I didn't think of the case where you have a non-partitionable tape 
in
a drive that can do partitions. That case should have been obvious to me.

I may be able to lay my hands on a LTO5+ drive (only a small chance). Someone in
the US is checking is checking for me and will hook it up to the system I use 
for
testing tape stuff for me. I'll only have it for about a week if I'm able to 
get it.

Thanks
Shane

> 
> Thanks,
> Kai
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

 size: 4096 (1 blocks).
[ 4599.014356] st 5:0:1:0: [st0] Rewinding tape.
[ 4631.811295] st 5:0:1:0: [st0] Block limits 1 - 16777215 bytes.
[ 4631.813313] st 5:0:1:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 4631.813318] st 5:0:1:0: [st0] Density 44, tape length: 0, drv buffer: 1
[ 4631.813321] st 5:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
[ 4631.813342] st 5:0:1:0: [st0] Mode 0 options: buffer writes: 1, async 
writes: 1, read ahead: 1
[ 4631.813345] st 5:0:1:0: [st0] can bsr: 1, two FMs: 0, fast mteom: 0, 
auto lock: 0,
[ 4631.813348] st 5:0:1:0: [st0] defs for wr: 0, no block limits: 0, 
partitions: 1, s2 log: 0
[ 4631.813351] st 5:0:1:0: [st0] sysv: 0 nowait: 0 sili: 0 nowait_filemark: 0
[ 4631.813354] st 5:0:1:0: [st0] debugging: 1
[ 4631.813359] st 5:0:1:0: [st0] Rewinding tape.
[ 4648.170712] st 5:0:1:0: [st0] Block limits 1 - 16777215 bytes.
[ 4648.172774] st 5:0:1:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 4648.172779] st 5:0:1:0: [st0] Density 44, tape length: 0, drv buffer: 1
[ 4648.172783] st 5:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
[ 4648.172786] st 5:0:1:0: [st0] Updating partition number in status.
[ 4648.175584] st 5:0:1:0: [st0] Got tape pos. blk 0 part 0.
[ 4648.175604] st 5:0:1:0: [st0] Loading tape.
[ 4648.209689] st 5:0:1:0: [st0] Block limits 1 - 16777215 bytes.
[ 4648.212799] st 5:0:1:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 4648.212804] st 5:0:1:0: [st0] Density 44, tape length: 0, drv buffer: 1
[ 4648.212808] st 5:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
[ 4648.215855] st 5:0:1:0: [st0] Partition page length is 10 bytes.
[ 4648.215861] st 5:0:1:0: [st0] PP: max 0, add 0, xdp 0, psum 03, pofmetc 0, 
rec 03, units 09, sizes: 400 65535
[ 4648.215866] st 5:0:1:0: [st0] MP: 11 08 00 00 18 03 09 00 01 90 ff ff
[ 4648.215869] st 5:0:1:0: [st0] psd_cnt 1, max.parts 0, nbr_parts 0
[ 4648.215872] st 5:0:1:0: [st0] Formatting tape with two partitions (1 = 1000 
MB).
[ 4648.215875] st 5:0:1:0: [st0] Sent partition page length is 10 bytes. 
needs_format: 0
[ 4648.215879] st 5:0:1:0: [st0] PP: max 0, add 1, xdp 1, psum 03, pofmetc 0, 
rec 03, units 09, sizes: 65535 1
[ 4648.215883] st 5:0:1:0: [st0] MP: 11 08 00 01 38 03 09 00 ff ff 00 01
[ 4648.220140] st 5:0:1:0: [st0] Error: 802, cmd: 15 10 0 0 16 0
[ 4648.220145] st 5:0:1:0: [st0] Sense Key : Illegal Request [current]
[ 4648.220149] st 5:0:1:0: [st0] Add. Sense: Invalid field in parameter list
[ 4648.220153] st 5:0:1:0: [st0] Partitioning of tape failed.
[ 4648.220269] st 5:0:1:0: [st0] Rewinding tape.

I'm asking around again one final time to see if I can lay my hands on a LTO5 
or greater drive so I can test LTO partitioning as well.

The only other thing I can think of (I'm not sure if this is an improvement or 
not) is if bp[pgo + PP_OFF_MAX_ADD_PARTS] + bp[pgo + PP_OFF_NBR_ADD_PARTS] 
(max.parts and nbr_parts in the debug message) is zero just return -EINVAL 
unless you know of any take drives that report them both as 0 but can be 
partitioned? That is after this:

DEBC_printk(STp, "psd_cnt %d, max.parts %d, nbr_parts %d\n",
psd_cnt, bp[pgo + PP_OFF_MAX_ADD_PARTS],
bp[pgo + PP_OFF_NBR_ADD_PARTS]);

add (and also turn off the can-partitions option):

if ((bp[pgo + PP_OFF_MAX_ADD_PARTS] + bp[pgo + PP_OFF_NBR_ADD_PARTS]) 
== 0) {
DEBC_printk(STp, "Drive not partitionable - max.parts+nbr_parts 
is 0\n");
STp->can_partitions = 0;
return -EINVAL;
}

I'm not especially fussed if you don't want to add that though.

Thanks
Shane

> -Original Message-
> From: makisara@kai.makisara.private [mailto:makisara@kai.makisara.private]
> On Behalf Of Kai Makisara
> Sent: Saturday, January 30, 2016 4:22 AM
> To: Seymour, Shane M
> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: RE: What partition should the MTMKPART argument specify? Was:
> Re: st driver doesn't seem to grok LTO partitioning
> 
> On Friday 2016-01-29 01:12, Seymour, Shane M wrote:
> 
> >Date: Fri, 29 Jan 2016 01:12:41
> >From: "Seymour, Shane M" <shane.seym...@hpe.com>
> >To: "\"Kai Mäkisara (Kolumbus)\"" <kai.makis...@kolumbus.fi>
> >Cc: Laurence Oberman <lober...@redhat.com>,
> >Emmanuel Florac <eflo...@intellique.com>,
> >Laurence Oberman <oberma...@gmail.com>,
> >"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
> >Subject: RE: What partition should the MTMKPART argument specify? Was:
> Re: st
> >driver doesn't seem to grok LTO partitioning
> >
> >Hi Kai,
> >
> >$ pwd
> >/sys/class/scsi_tape/st1/device
> >$ cat scsi_level
> >4
> >
> OK. T

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

Hi Kai,

$ pwd
/sys/class/scsi_tape/st1/device
$ cat scsi_level
4

Thanks
Shane

> -Original Message-
> From: "Kai Mäkisara (Kolumbus)" [mailto:kai.makis...@kolumbus.fi]
> Sent: Friday, January 29, 2016 4:04 AM
> To: Seymour, Shane M
> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: Re: What partition should the MTMKPART argument specify? Was:
> Re: st driver doesn't seem to grok LTO partitioning
> 
> 
> > On 28.1.2016, at 9.36, Seymour, Shane M <shane.seym...@hpe.com>
> wrote:
> >
> > Hi Kai,
> >
> > With the changes the I get a failure partitioning a HP DAT72 drive (DDS-5):
> >
> > # ./mt -f /dev/st1 stsetoption debug
> > # ./mt -f /dev/st1 stsetoption can-partitions # ./mt -f /dev/st1
> > mkpartition 1000
> > /dev/st1: Input/output error
> >
> ...
> > [ 3976.389605] st 6:0:3:0: [st1] Partition page length is 10 bytes.
> > [ 3976.389610] st 6:0:3:0: [st1] PP: max 1, add 0, xdp 0, psum 02,
> > pofmetc 0, rec 03, units 00, sizes: 0 65535 [ 3976.389614] st 6:0:3:0:
> > [st1] MP: 11 08 01 00 10 03 00 00 00 00 ff ff [ 3976.389618] st
> > 6:0:3:0: [st1] psd_cnt 2, max.parts 1, nbr_parts 0
>  ^ The problem is 
> here
> 
> ...
> > Using a slightly older kernel to partition the DAT72 drive works (same 3
> commands as above):
> ...
> > [  351.584906] st 6:0:3:0: [st1] Partition page length is 10 bytes.
> > [  351.584908] st 6:0:3:0: [st1] psd_cnt 1, max.parts 1, nbr_parts 0
> 
> The old driver computes the psd_cnt from the returned page length. The
> same applies to the patched driver if the SCSI level of the device < SCSI_3.
> This works correctly with my drive that reports SCSI_2. So, the question is:
> what SCSI level does your device report?
> 
> Kai

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

Hi Laurence,

> -Original Message-
> From: Laurence Oberman [mailto:lober...@redhat.com]
> Sent: Friday, January 29, 2016 10:25 AM
> To: Seymour, Shane M
> Cc: Kai Mäkisara (Kolumbus); Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: Re: What partition should the MTMKPART argument specify? Was:
> Re: st driver doesn't seem to grok LTO partitioning
> 
> Meant to mention, still waiting for my new LTO5, also this is the first time I
> am testing the DAT72.
> 
> Shane, have you had the DAT working before this last patch, if so which patch

The latest test was the first chance that I've had to test the changes. I 
didn't test the previous patches because I didn't have a partitionable LTO 
drive but I wanted to test the changes to set an explicit size for partition 0 
and 1 (since it's the only partitionable drive I have) and found it didn't work 
with the DAT72.

With a fedora user space (an old FC20 install) and a modified mt-st (to remove 
the negative test) compiled up I tested 4.4.0-next-20160113 (that also has some 
PCI patches in it for testing) with Kai's patch and it failed and I had an 
older kernel around (4.4.0-rc5-next-20151215) that I tested after seeing the 
failure and that works.

Thanks
Shane

> 
> Laurence Oberman
> Principal Software Maintenance Engineer
> Red Hat Global Support Services
> 
> - Original Message -
> From: "Laurence Oberman" <lober...@redhat.com>
> To: "Shane M Seymour" <shane.seym...@hpe.com>
> Cc: "Kai Mäkisara (Kolumbus)" <kai.makis...@kolumbus.fi>, "Emmanuel
> Florac" <eflo...@intellique.com>, "Laurence Oberman"
> <oberma...@gmail.com>, linux-scsi@vger.kernel.org
> Sent: Thursday, January 28, 2016 6:23:13 PM
> Subject: Re: What partition should the MTMKPART argument specify? Was:
> Re: st driver doesn't seem to grok LTO partitioning
> 
> On My DAT tape with the latest patch
> 
> 
> [root@srp-server ~]# cat /sys/class/scsi_tape/st0/device/scsi_level
> 4
> 
> [root@srp-server ~]# mt -f /dev/st0 stsetoption can-partitions
> 
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] Block limits 1 - 16777215
> bytes.
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] Mode sense. Length 11,
> medium 0, WBS 10, BLL 8 Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0]
> Density 47, tape length: 0, drv buffer: 1 Jan 28 18:17:49 srp-server kernel: 
> st
> 6:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] Mode 0 options: buffer
> writes: 1, async writes: 1, read ahead: 1
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] can bsr: 1, two FMs: 
> 0, fast
> mteom: 0, auto lock: 0,
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] defs for wr: 0, no 
> block
> limits: 0, partitions: 1, s2 log: 0
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] sysv: 0 nowait: 0 
> sili: 0
> nowait_filemark: 0
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] debugging: 1
> Jan 28 18:17:49 srp-server kernel: st 6:0:1:0: [st0] Rewinding tape.
> 
> [root@srp-server ~]# mt -f /dev/st0 mkpartition 1000
> 
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Block limits 1 - 16777215
> bytes.
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Mode sense. Length 11,
> medium 0, WBS 10, BLL 8 Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0]
> Density 47, tape length: 0, drv buffer: 1 Jan 28 18:18:01 srp-server kernel: 
> st
> 6:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Loading tape.
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Error: 802, cmd: 0 0 
> 0 0 0
> 0 Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Sense Key : Unit 
> Attention
> [current] Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Add. Sense: Not
> ready to ready change, medium may have changed Jan 28 18:18:01 srp-server
> kernel: st 6:0:1:0: [st0] Block limits 1 - 16777215 bytes.
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Mode sense. Length 11,
> medium 0, WBS 10, BLL 8 Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0]
> Density 47, tape length: 0, drv buffer: 1 Jan 28 18:18:01 srp-server kernel: 
> st
> 6:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] Partition page length is 
> 10
> bytes.
> Jan 28 18:18:01 srp-server kernel: st 6:0:1:0: [st0] PP: max 1, add 0, xdp 0,
> psum 02, pofmetc 0, rec 03, units 00, sizes: 0 65535 Jan 28 18:18:01 
> srp-server
> kernel: st 6:0:1:0: [st0] MP: 11 08 01 00 10 03 00 00 00 00 ff ff Jan 28 
> 18:18:01
> srp-server kernel: st 6:0:1:0: [st0] psd

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

: [st1] debugging: 1
[  345.335583] st 6:0:3:0: [st1] Rewinding tape.
[  351.583495] st 6:0:3:0: [st1] Block limits 1 - 16777215 bytes.
[  351.583779] st 6:0:3:0: [st1] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[  351.583782] st 6:0:3:0: [st1] Density 47, tape length: 0, drv buffer: 1
[  351.583783] st 6:0:3:0: [st1] Block size: 0, buffer size: 4096 (1 blocks).
[  351.583785] st 6:0:3:0: [st1] Updating partition number in status.
[  351.584184] st 6:0:3:0: [st1] Got tape pos. blk 0 part 0.
[  351.584196] st 6:0:3:0: [st1] Rewinding tape.
[  351.584906] st 6:0:3:0: [st1] Partition page length is 10 bytes.
[  351.584908] st 6:0:3:0: [st1] psd_cnt 1, max.parts 1, nbr_parts 0
[  351.584910] st 6:0:3:0: [st1] Formatting tape with two partitions (1 = 1000 
MB).
[  723.976554] st 6:0:3:0: [st1] Rewinding tape.

Thanks
Shane

> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Kai Makisara
> Sent: Monday, January 25, 2016 8:05 AM
> To: Seymour, Shane M
> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: RE: What partition should the MTMKPART argument specify? Was:
> Re: st driver doesn't seem to grok LTO partitioning
> 
> On Friday 2016-01-22 04:10, Seymour, Shane M wrote:
> 
> 
> >> -Original Message-
> >> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> >> ow...@vger.kernel.org] On Behalf Of "Kai Mäkisara (Kolumbus)"
> >> Sent: Friday, January 22, 2016 7:59 AM
> >> To: Seymour, Shane M
> >> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> >> s...@vger.kernel.org
> >> Subject: What partition should the MTMKPART argument specify? Was:
> Re:
> >> st driver doesn't seem to grok LTO partitioning
> >>
> ...
> >>
> >> There seem to be lot of arguments supporting both possible choices.
> Should
> >> we use the existing definition (1) or change it for the drives supporting
> SCSI
> >> level >= 3 (or supporting FORMAT MEDIUM)? The definition can’t be
> >> changed later. This is why we should make a good decision.
> >>
> >> Opinions?
> >
> >How about using the fact the size is signed to indicate one slightly 
> >different
> >thing? I'm not sure if you'd call this using or abusing the fact that it's 
> >signed.
> >
> >Make the default behavior for a positive size the same as the current
> >behavior for SCSI-2 (size applies to partition 1). If the size is negative 
> >then
> >the absolute value of the size applies to partition 0. That provides some
> >flexibility in choosing which partition the size applies to if it worked that
> >way for all devices.
> >
> >With that you also get consistent behavior between tape drives without
> >having to know if the size will apply to partition 0 or partition 1 based on
> >the tape technology and you get the benefit of being able to set an
> >explicit size for partition 0 or partition 1.
> >
> I like this suggestion, because of the reasons you point out. I think
> this is using the fact that the argument is signed.
> 
> >You could overload the value of 0 as well to use FDP to choose the sizes
> >for the partitions (assuming 0 doesn't already have a side effect in
> >the code). Then you get whatever the tape drive wants to do.
> >
> The value zero is used to specify only one partition. The previous
> patches overloaded the size 1. However, I think supporting FDP is
> useless nowadays: the drives that support FDP=1 seem to end up with the
> same partitioning (two wraps to partition 1) with any small number as
> argument.
> 
> Below is a test patch that implements the current behaviour with
> non-negative argument (but works with LTOs etc.) and makes partition
> zero size absolute value of argument (MB) if argument is negative. If
> you want to test the patch, note that the current mt-st does not accept
> negative numbers. A minimal patch is needed.
> 
> Thanks,
> Kai
> ---8<
> --- ref/drivers/scsi/st.c 2015-12-21 18:54:05.068882001 +0200
> +++ new/drivers/scsi/st.c 2016-01-24 22:36:13.250928500 +0200
> @@ -9,7 +9,7 @@
> Steve Hirsch, Andreas Koppenh"ofer, Michael Leodolter, Eyal Lebedinsky,
> Michael Schaefer, J"org Weule, and Eric Youngdale.
> 
> -   Copyright 1992 - 2010 Kai Makisara
> +   Copyright 1992 - 2016 Kai Makisara
> email kai.makis...@kolumbus.fi
> 
> Some small formal changes - aeb, 950809
> @@ -17,7 +17,7 @@
> Last modified: 18-JAN-1998 Richard Gooch <rgo...@atnf.cs

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

Hi Emmanuel,

> Hmm in fact if we keep using MB we'll be stuck when tapes reach ~2 PB
> which leaves some time to think about it, until LTO-15 circa 2036 :)

There will be other issues to solve before then (by LTO-9 2 with compression
or LTO-10 without compression and we're at LTO-7 now). Take tar format
archives with a standard block size of 10k can take this much data to get
 to 2^32 blocks and cause the current 32bit block number to wrap:

43,980,465,111,040 (2^32 * 10240)

After that much data has been written the SCSI-2 command READ POSITION
will not be able to show the current position correctly (which is what the st
driver uses to determine the position for an MTIOCPOS). It may be less
than that since some drives include file marks in the logical block number if
the program that produced the tape writes them out.

That means switching to the extended block id form of READ POSITION
so we have 64bit counts for those values, see page 150:

https://docs.oracle.com/cd/E21419_04/en/LTO5_Vol3_E5b/LTO5_Vol3_E5b.pdf

That's going to require new ioctls like MTIOCPOS64 and other changes within
the driver to support larger types for holding some values. That will also raise
all sorts of fun compatibility questions as well (should MTIOCPOS work at all
for such a tape drive or should it work until something overflows and return
what data it can and give an errno of -EOVERFLOW etc).

That's probably the correct time to also look at adding support for more
partitions. Not sure when the extended block id form of READ POSITION
got added but it may mean only supporting the wider values only with tape
drives that support REPORT SUPPORTED OPCODES (if that can indicate that
it supports READ POSITION with extended block ids and anything else
required to support block numbers larger than 2^32).

The 0x91 version of SPACE needs to be used as well (the 32bit version 0x11
Is currently used) to allow tape movement with counts >2^32. That requires
a new ioctl call. I haven't looked at what else may need to change but there's
likely to be more. The alternate version of SPACE is from page 220 of the
above HP LTO5 tape reference.

Thanks
Shane

P.S. you could force the above changes now using a 512 byte block size since
the block number would wrap at this size on LTO (ignoring the fact that it
wouldn't make sense to use a block size that small on LTO):

2,199,023,255,552 (2^32*512)

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

Hi Kai,

> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of "Kai Mäkisara (Kolumbus)"
> Sent: Friday, January 22, 2016 7:59 AM
> To: Seymour, Shane M
> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: What partition should the MTMKPART argument specify? Was: Re:
> st driver doesn't seem to grok LTO partitioning
> 
> 
> > On 15.1.2016, at 2.21, Seymour, Shane M <shane.seym...@hpe.com>
> wrote:
> >
> > Unfortunately I'm unable to lay my hands on an LTO 5 tape drive so I'm not
> able to test that it works either. If it helps at all I can test in the 
> negative and
> make sure that for an LTO 3 drive it fails gracefully but that's about it at 
> the
> moment.
> 
> Thanks for all testers and those who attempted to test. The latest patch
> applies the standard quite strictly and I think it should work with most 
> drives.
> The implementation can be fixed later if problems are found.
> 
> However, before making the final patch, we should decide which partition
> the specified size should apply to. For the SCSI level <=2 it applies to 
> partition
> 1. For other drives we may have some freedom to “tune” the definition. The
> size should apply to the partition the users expect it to apply.

I'd argue for consistency here in the current interface and that it should 
apply in the same way more on that below.

> 
> The current documentation says "the argument gives in megabytes the size
> of partition 1 that is physically the first partition of the tape”. The
> documentation I have found for current drives (HP and IBM LTO, IBM 3592,
> Storagetek T1000) all number the partitions sequentially from the start of the
> tape. The access time for any partition is probably about the same when
> wrapwise partitioning is used. It does matter with linear partitioning.
> Unfortunately, the standards leave the numbering to the implementor.
> 
> Partitioning with two partitions is used for storing index in a small 
> partition
> and use the rest of the tape for data. In this case, it is probably natural to
> specify the size of the index. The LTFS definition supports index in any
> partition. The open source code I have seen seem to default to index in
> partition 0.
> 
> The HP and IBM LTO default partitioning (FDP=1) specifies two wraps
> (minimum) to partition 1 and the rest to 0.

It may be worth noting (if you're going to update any documentation)
that isn't 100% accurate. You actually get one wrap in partition 1 and the
rest minus one wrap into partition 0. There is one wrap used as a guard
between the two partitions. The size given to a partition is rounded up
to the size of a wrap as well.

See https://docs.oracle.com/cd/E21419_04/en/LTO5_Vol3_E5b/LTO5_Vol3_E5b.pdf

Page 100 where it gives examples of how partition sizes are calculated on HP 
LTO5 drives.

> 
> There seem to be lot of arguments supporting both possible choices. Should
> we use the existing definition (1) or change it for the drives supporting SCSI
> level >= 3 (or supporting FORMAT MEDIUM)? The definition can’t be
> changed later. This is why we should make a good decision.
> 
> Opinions?

How about using the fact the size is signed to indicate one slightly different
thing? I'm not sure if you'd call this using or abusing the fact that it's 
signed.

Make the default behavior for a positive size the same as the current
behavior for SCSI-2 (size applies to partition 1). If the size is negative then
the absolute value of the size applies to partition 0. That provides some
flexibility in choosing which partition the size applies to if it worked that
way for all devices.

With that you also get consistent behavior between tape drives without
having to know if the size will apply to partition 0 or partition 1 based on
the tape technology and you get the benefit of being able to set an
explicit size for partition 0 or partition 1.

You could overload the value of 0 as well to use FDP to choose the sizes
for the partitions (assuming 0 doesn't already have a side effect in
the code). Then you get whatever the tape drive wants to do.

Thanks
Shane

> 
> Thanks,
> Kai
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

RE: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

My applogies:

> It may be worth noting (if you're going to update any documentation) that
> isn't 100% accurate. You actually get one wrap in partition 1 and the rest
> minus one wrap into partition 0. There is one wrap used as a guard between
> the two partitions. The size given to a partition is rounded up to the size 
> of a
> wrap as well.
> 
> See
> https://docs.oracle.com/cd/E21419_04/en/LTO5_Vol3_E5b/LTO5_Vol3_E5b.
> pdf
> 
> Page 100 where it gives examples of how partition sizes are calculated on HP
> LTO5 drives.

I should have actually said:

You do get two wraps as a minimum in partition 1 and the rest minus two wraps
into partition 0. The extra two wraps are used as a guard between the two 
partitions
and all sizes will be rounded to a multiple of two wraps.

That was just to make it clear that partition sizes will always end up being a 
multiple
of 2x the wrap size and that there was some fixed overhead in partitioning an
LTO5+ drive (2 wraps).
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH] ipr: fix out-of-bounds null overwrite

[Seymour, Shane M]

>   len = snprintf(fname, 99, "%s", buf);
> - fname[len-1] = '\0';

Since it appears that's the only time len is actually used in that function can
you please remove the variable len completely as part of the patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

If you need help with anything please let me know I'd be more than happy to 
contribute (with testing and a review if you want). I have a system with an 
older LTO-3 tape drive that I can use any time (it doesn’t support partitioning 
so if nothing else I can make sure partitioning attempts fail gracefully). I 
may be able to beg, borrow, or steal access to an LTO 5 or 6 drive though to 
help out in testing.

Kai, for the source of the HPE EverStore software should be available here:

http://h20564.www2.hpe.com/hpsc/swd/public/readIndex?sp4ts.oid=5111617

Select something like RHEL7 server from the dropdown on the page that loads 
click on the + in front of "Software - Storage (4)". If you click on the first 
product listed then click on select (at the next screen you will unfortunately 
need to create a HP Passport account) and then you need to give a lot of 
personal information when you get the option to download the source make sure 
you change the pulldown above the download buttons to standard download. The 
version 3.0 source is about 1.2Mb in size (GPL version 2.1).

This seems to be the relevant code from the driver though (the same download 
has the ibm tape driver as well). You'll need to look at the following:

ltotape.c - ltotape_readposition to determine the current partition
ltotape.c - ltotape_locate - to move to a position on tape (includes setting 
the partition and has a special flag for changing partitions between the two it 
supports if required)
ltotape.c - ltotape_format - for creating and destroying partitions
ltotape.c - ltotape_remaining_capacity - will get you the remaining and maximum 
capacity for the partitions

When you look at those functions you'll see TC_FORMAT_TYPE referenced this is 
the enum referred it is in src/libltfs/tape_ops.h:

typedef enum {
TC_FORMAT_DEFAULT   = 0x00,   /* Make 1 partition medium */
TC_FORMAT_PARTITION = 0x01,   /* Make 2 partition medium */
TC_FORMAT_DEST_PART = 0x02,   /* Destroy all data and make 2 partition 
medium */
TC_FORMAT_MAX   = 0x03
} TC_FORMAT_TYPE;/* Space command operations */

The driver at that download looks like it only supports two partitions though 
and if you go looking around the code (grep for partition) some LTO drives 
(probably older ones) look like they may be partition aware but may not 
actually support partitions, see this comment:

ltotape_platform.c:  * For an LTO drive, need to determine whether it is 
partition-capable or only partition-aware:

So you may need to check for firmware that is partition aware but not partition 
capable.

The IBM LTO SCSI reference is nice and long and it looks like you can either 
make SET CAPACITY calls on the currently mounted medium to set the sizes and 
then format the medium it also refers to the medium partition mode page in 
terms of changing the partitioning of the tape.
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH] scsi: rescan VPD attributes

[Seymour, Shane M]

> The VPD page information might change, so we need to be able to
> update it. This patch implements a VPD page rescan whenever
> the 'rescan' sysfs attribute is triggered.
>
> Signed-off-by: Hannes Reinecke 
> ---

Reviewed-by: Shane Seymour 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] scsi: rescan VPD attributes

[Seymour, Shane M]

Hi Hannes,

I have one probably small nitpick about the patch. I'm not sure how likely what 
I've put below is likely to happen in real life though.

Is there any chance at all that sdev->vpd_pg83_len could change when updated? 
If there's any chance of that I'd have expected that both the length of and the 
pointer to the vpd data would need to be protected not just the pointer so 
someone would have a consistent picture of the vpd and its length. Without that 
there is a race where someone could be using a new length with the old vpd 
data. That leaves the potential for a length that exceeds the vpd size if the 
new data is larger than the old data - I don't know how likely it is but wanted 
to at least bring it up as something to consider. 

I'm not so concerned about sdev->vpd_pg80_len changing since it should have 1 
of 2 fixed sizes and it seems unlikely that once read the first time a device 
would increase it in size (but for consistency in the code if you make a change 
you might want to treat them the same way).

Thanks
Shane
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] st: allow debug output to be enabled or disabled via sysfs

[Seymour, Shane M]

Change st driver to allow enabling or disabling debug output
via sysfs file /sys/bus/scsi/drivers/st/debug_flag.

Previously the only way to enable debug output was:

1. loading the driver with the module parameter debug_flag=1
2. an ioctl call (this method was also the only way to dynamically
disable debug output).

To use the ioctl you need a second tape drive (if you are
actively testing the first tape drive) since a second process
cannot open the first tape drive if it is in use.

The this change is only functional if the value of the macro
DEBUG in st.c is a non-zero value (which it is by default).

Signed-off-by: Shane Seymour 
---
--- a/drivers/scsi/st.c 2015-10-06 17:11:16.299801789 -0500
+++ b/drivers/scsi/st.c 2015-10-11 14:45:10.595060995 -0500
@@ -4452,11 +4452,41 @@ static ssize_t version_show(struct devic
 }
 static DRIVER_ATTR_RO(version);
 
+#if DEBUG
+static ssize_t debug_flag_store(struct device_driver *ddp,
+   const char *buf, size_t count)
+{
+/* We only care what the first byte of the data is the rest is unused.
+ * if it's a '1' we turn on debug and if it's a '0' we disable it. All
+ * other values have -EINVAL returned if they are passed in.
+ */
+   if (count > 0) {
+   if (buf[0] == '0') {
+   debugging = NO_DEBUG;
+   return count;
+   } else if (buf[0] == '1') {
+   debugging = 1;
+   return count;
+   }
+   }
+   return -EINVAL;
+}
+
+static ssize_t debug_flag_show(struct device_driver *ddp, char *buf)
+{
+   return scnprintf(buf, PAGE_SIZE, "%d\n", debugging);
+}
+static DRIVER_ATTR_RW(debug_flag);
+#endif
+
 static struct attribute *st_drv_attrs[] = {
_attr_try_direct_io.attr,
_attr_fixed_buffer_size.attr,
_attr_max_sg_segs.attr,
_attr_version.attr,
+#if DEBUG
+   _attr_debug_flag.attr,
+#endif
NULL,
 };
 ATTRIBUTE_GROUPS(st_drv);
diff -uprN a/Documentation/ABI/testing/sysfs-driver-st 
b/Documentation/ABI/testing/sysfs-driver-st
--- a/Documentation/ABI/testing/sysfs-driver-st 1969-12-31 18:00:00.0 
-0600
+++ b/Documentation/ABI/testing/sysfs-driver-st 2015-10-11 14:28:43.537128220 
-0500
@@ -0,0 +1,12 @@
+What:  /sys/bus/scsi/drivers/st/debug_flag
+Date:  October 2015
+Kernel Version:?.?
+Contact:   shane.seym...@hpe.com
+Description:
+   This file allows you to turn debug output from the st driver
+   off if you write a '0' to the file or on if you write a '1'.
+   Note that debug output requires that the module be compiled
+   with the #define DEBUG set to a non-zero value (this is the
+   default). If DEBUG is set to 0 then this file will not
+   appear in sysfs as its presence is conditional upon debug
+   output support being compiled into the module.
--- a/Documentation/scsi/st.txt 2015-10-06 17:11:12.323802060 -0500
+++ b/Documentation/scsi/st.txt 2015-10-11 14:19:48.176164681 -0500
@@ -569,7 +569,9 @@ Debugging code is now compiled in by def
 with the kernel module parameter debug_flag defaulting to 0.  Debugging
 can still be switched on and off with an ioctl.  To enable debug at
 module load time add debug_flag=1 to the module load options, the
-debugging output is not voluminous.
+debugging output is not voluminous. Debugging can also be enabled
+and disabled by writing a '0' (disable) or '1' (enable) to the sysfs
+file /sys/bus/scsi/drivers/st/debug_flag.
 
 If the tape seems to hang, I would be very interested to hear where
 the driver is waiting. With the command 'ps -l' you can see the state
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3] hpsa: convert show method snprintf usage to scnprintf

[Seymour, Shane M]

Hi James,

 There's been no ack on this one.  However, there's no actual reason to
 prefer scnprintf over snprintf: the former will zero terminate, the
 latter won't if the write length is over the buffer length, but this is
 a file buffer: the routine will return as many bytes to userspace as are
 specified in the count (including zeros if they're within the count), so
 zero termination of a string in sysfs is unnecessary.

There's a patch in Greg's driver tree for the next merge that changes
the documentation about the usage of the s*printf() functions in sysfs
show() methods from/to (in Documentation/filesystems/sysfs.txt):

-- show() should always use scnprintf().
+- show() must not use snprintf() when formatting the value to be
+  returned to user space. If you can guarantee that an overflow
+  will never happen you can use sprintf() otherwise you must use
+  scnprintf().

It currently says you should use scnprintf() but will become more
explicit about what you must not use and what you can or must use.
That's probably the best reason I can offer about why to prefer one
function over the other.

This is my understanding of the difference between snprintf() and
scnprintf() in terms of sysfs show() methods - there is a subtle
difference between the two functions in the return value.

The snprintf() function returns the number of bytes that it would
have formatted given sufficient space. It doesn't matter what the
size argument was. If the size passed in is 4096 and the number of
bytes that it would have formatted is 4200 then 4200 will be what is
returned from snprintf() even though it did not modify anything
after byte 4096 in the buffer.

The scnprintf() function returns the number of bytes it actually
formatted (excluding the zero termination). Using the above data
if 4096 was the size passed in then the return value will never be
more than 4095.

There is code in sysfs_kf_seq_show() to make sure that the count
returned from the show() method is not = PAGE_SIZE and
reduce it to PAGE_SIZE-1 if it was. I don't think user space will 
ever get more than PAGE_SIZE-1 bytes regardless of which
function is used.

I don't mind if the patch isn't accepted but I thought I should at
least explain my rationale behind the change.

Thanks
Shane
N�r��yb�X��ǧv�^�)޺{.n�+{{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj��!�i

[ANNOUNCE] tapestat command now part of upstream sysstat

[Seymour, Shane M]

While tape stats were being implemented in the kernel I started working
on a program that would read them out and display the data to allow me
to test the interface. After the changes were available in linux-next
I worked with the upstream sysstat maintainer to get that code into shape
so it was suitable for inclusion into sysstat. The latest version of
sysstat on github now contains the tapestat command:

https://github.com/sysstat/sysstat

New tarballs will be available from the sysstat homepage soon. 

Some example output (this is the only format) while using the dd command
to copy a tape from a slow tape drive to a fast one (DDS to LTO):

Tape:r/s w/s   kB_read/s   kB_wrtn/s %Rd %Wr %OaRs/sOt/s
st00 523   05230   0  14  14   0   0
st1  523   05230   0  83   0  83   0   0

Tape:r/s w/s   kB_read/s   kB_wrtn/s %Rd %Wr %OaRs/sOt/s
st00 406   04060   0  11  11   0   0
st1  406   04060   0  86   0  86   0   0

Tape:r/s w/s   kB_read/s   kB_wrtn/s %Rd %Wr %OaRs/sOt/s
st00 318   03181   0   8   8   0   0
st1  318   03182   0  90   0  90   0   0

I want to thank everyone who provided advice, feedback, or helped me get
tape stats to this point - especially Sebastien who was a pleasure to work
with.

Thanks
Shane
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] hpsa: non-zero LUNs of multi-LUN devices missing on some Smart Arrays

[Seymour, Shane M]

A regression was introduced into the hpsa driver a while back so
non-zero LUNs of multi-LUN devices may no longer be presented via
a SAS based Smart Array. I have not done a bisection to discover
the change that caused it.

The CISS firmware specification (available on sourceforge)
defines an 8 byte lunid that describes devices that the Smart
Array can see/present to the system. The current code in the hpsa
driver attempts to find matches for non-zero LUNs with LUN 0 for
a bus/target by zeroing out byte 4 of the lunid and find a match.

This method is sufficient for SCSI based Smart Arrays because
byte 5 is always 0. For SAS based Smart arrays byte 5 of the
lunid contains the path number for a multipath device and
either one or two bits (the documentation does not define how
many bits are used but it appears it may be one only) that
indicate if the given path number in byte 5 must always be
used to access that device. Byte 5 may not always be zero.

The following are lunids (spaces added for clarity) for a
MSL2024 single drive library connected via a H241 Smart Array:

00 00 00 00 01 00 00 01 (changer)
00 00 00 00 00 80 00 01 (tape)

In the 4th byte (counting from 0) you can see that the tape
is LUN 0 and the changer is LUN 1. The 0x80 set in the 5th byte
for the tape drive means the driver should force access to
path 0 (the library in this case was connected to one path only
anyway).

After the changes we can see the following in the dmesg output:

scsi 0:3:0:0: RAID  HP   H241 1.18 \
PQ: 0 ANSI: 5
scsi 0:2:0:0: Sequential-Access HP   Ultrium 6-SCSI   354W \
PQ: 0 ANSI: 6
scsi 0:2:0:1: Medium ChangerHP   MSL G3 Series8.70 \
PQ: 0 ANSI: 5

Showing that the changer is correctly identified as LUN 1 of
bus 2 target 0. Before the change the changer device is not seen.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
Testing was done on RHEL 6.5 with hpsa driver version 3.4.10-120
and the below patch is the forward ported upstream patch since
the issue exists there as well.
--- a/drivers/scsi/hpsa.c   2015-07-06 19:48:22.224804233 -0500
+++ b/drivers/scsi/hpsa.c   2015-07-06 21:16:31.496444001 -0500
@@ -1083,17 +1083,19 @@ static int hpsa_scsi_add_entry(struct ct
 
/* This is a non-zero lun of a multi-lun device.
 * Search through our list and find the device which
-* has the same 8 byte LUN address, excepting byte 4.
+* has the same 8 byte LUN address, excepting byte 4 and 5.
 * Assign the same bus and target for this new LUN.
 * Use the logical unit number from the firmware.
 */
memcpy(addr1, device-scsi3addr, 8);
addr1[4] = 0;
+   addr1[5] = 0;
for (i = 0; i  n; i++) {
sd = h-dev[i];
memcpy(addr2, sd-scsi3addr, 8);
addr2[4] = 0;
-   /* differ only in byte 4? */
+   addr2[5] = 0;
+   /* differ only in byte 4 and 5? */
if (memcmp(addr1, addr2, 8) == 0) {
device-bus = sd-bus;
device-target = sd-target;
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH RFC 1/2] cxlflash: Base support for IBM CXL Flash Adapter

[Seymour, Shane M]

-Original Message-
From: linux-scsi-ow...@vger.kernel.org 
[mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Matthew R. Ochs
Sent: Monday, April 27, 2015 2:50 PM
To: linux-scsi@vger.kernel.org; james.bottom...@hansenpartnership.com; 
n...@linux-iscsi.org; brk...@linux.vnet.ibm.com
Cc: mi...@neuling.org; imun...@au1.ibm.com; Manoj N. Kumar
Subject: [PATCH RFC 1/2] cxlflash: Base support for IBM CXL Flash Adapter

 +static ssize_t cxlflash_show_port_status(struct device *dev,
 +  struct device_attribute *attr,
 +  char *buf)
 +{
 + struct Scsi_Host *shost = class_to_shost(dev);
 + struct cxlflash *cxlflash = (struct cxlflash *)shost-hostdata;
 + struct afu *afu = cxlflash-afu;
 +
 + char *disp_status;
 + int rc;
 + u32 port;
 + u64 status;
 + volatile u64 *fc_regs;
 +
 + rc = kstrtouint((attr-attr.name + 4), 10, port);
 + if (rc || (port  NUM_FC_PORTS))
 + return 0;
 +
 + fc_regs = afu-afu_map-global.fc_regs[port][0];
 + status =
 + (readq_be(fc_regs[FC_MTIP_STATUS / 8])  FC_MTIP_STATUS_MASK);
 +
 + if (status == FC_MTIP_STATUS_ONLINE)
 + disp_status = online;
 + else if (status == FC_MTIP_STATUS_OFFLINE)
 + disp_status = offline;
 + else
 + disp_status = unknown;
 +
 + return snprintf(buf, PAGE_SIZE, %s\n, disp_status);

You need to use scnprintf() instead of snprintf() here

 +static ssize_t cxlflash_show_lun_mode(struct device *dev,
 +   struct device_attribute *attr, char *buf)
 +{
 + struct Scsi_Host *shost = class_to_shost(dev);
 + struct cxlflash *cxlflash = (struct cxlflash *)shost-hostdata;
 + struct afu *afu = cxlflash-afu;
 +
 + return snprintf(buf, PAGE_SIZE, %u\n, afu-internal_lun);

See above about snprintf()

+static DEVICE_ATTR(port0, S_IRUGO, cxlflash_show_port_status, NULL);
+static DEVICE_ATTR(port1, S_IRUGO, cxlflash_show_port_status, NULL);
+static DEVICE_ATTR(lun_mode, S_IRUGO | S_IWUSR, cxlflash_show_lun_mode,
+  cxlflash_store_lun_mode);

Please use DEVICE_ATTR_RO and DEVICE_ATTR_RW as appropriate if you can, you 
will need to change the show/store function names. The main reason I know for 
doing this is (using DEVICE_ATTR_RO as an example) if someone sees a sysfs 
attribute called port0 or port1 they should be able to search the kernel source 
to find a function called port0_show or port1_show without having to spend any 
extra time searching to find out what the driver is and then look at the driver 
source to find that they need to look at cxlflash_show_port_status. Using 
DEVICE_ATTR_RO for port0 and port1 would probably change the code to looking 
something like this:

static ssize_t cxlflash_show_port_status(u32 port,
struct afu *afu, char *buf)
{
char *disp_status;
u64 status;
volatile u64 *fc_regs;

fc_regs = afu-afu_map-global.fc_regs[port][0];
/* I split this line into two because I had to really look at where
 * the brackets were and this made it more obvious to me
 * what it was doing but that could just be me. */
status = readq_be(fc_regs[FC_MTIP_STATUS / 8]);
status = FC_MTIP_STATUS_MASK;

if (status == FC_MTIP_STATUS_ONLINE)
disp_status = online;
else if (status == FC_MTIP_STATUS_OFFLINE)
disp_status = offline;
else
disp_status = unknown;

return scnprintf(buf, PAGE_SIZE, %s\n, disp_status);
}

Since you have fixed values you could use also sprintf here (although the 
documentation currently says to only use scnprintf) and that would make 
port0_show and port1_show to be:

static ssize_t port0_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct Scsi_Host *shost = class_to_shost(dev);
struct cxlflash *cxlflash = (struct cxlflash *)shost-hostdata;
struct afu *afu = cxlflash-afu;

return cxlflash_show_port_status(0, afu, buf);
}

static ssize_t port1_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct Scsi_Host *shost = class_to_shost(dev);
struct cxlflash *cxlflash = (struct cxlflash *)shost-hostdata;
struct afu *afu = cxlflash-afu;

return cxlflash_show_port_status(1, afu, buf);
}

I'm assuming that 0 and 1 are always valid for port number and don't need to be 
checked against NUM_FC_PORTS. That's just a suggestion and I haven't attempted 
to compile the above example and you'd need to test it to make sure that they 
still work as expected.

Shane
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] st: null pointer dereference panic caused by use after kref_put by st_open

[Seymour, Shane M]

Two SLES11 SP3 servers encountered similar crashes simultaneously
following some kind of SAN/tape target issue:

...
qla2xxx [:81:00.0]-801c:3: Abort command issued nexus=3:0:2 --  1 2002.
qla2xxx [:81:00.0]-801c:3: Abort command issued nexus=3:0:2 --  1 2002.
qla2xxx [:81:00.0]-8009:3: DEVICE RESET ISSUED nexus=3:0:2 
cmd=882f89c2c7c0.
qla2xxx [:81:00.0]-800c:3: do_reset failed for cmd=882f89c2c7c0.
qla2xxx [:81:00.0]-800f:3: DEVICE RESET FAILED: Task management failed 
nexus=3:0:2 cmd=882f89c2c7c0.
qla2xxx [:81:00.0]-8009:3: TARGET RESET ISSUED nexus=3:0:2 
cmd=882f89c2c7c0.
qla2xxx [:81:00.0]-800c:3: do_reset failed for cmd=882f89c2c7c0.
qla2xxx [:81:00.0]-800f:3: TARGET RESET FAILED: Task management failed 
nexus=3:0:2 cmd=882f89c2c7c0.
qla2xxx [:81:00.0]-8012:3: BUS RESET ISSUED nexus=3:0:2.
qla2xxx [:81:00.0]-802b:3: BUS RESET SUCCEEDED nexus=3:0:2.
qla2xxx [:81:00.0]-505f:3: Link is operational (8 Gbps).
qla2xxx [:81:00.0]-8018:3: ADAPTER RESET ISSUED nexus=3:0:2.
qla2xxx [:81:00.0]-00af:3: Performing ISP error recovery - 
ha=88bf04d18000.
 rport-3:0-0: blocked FC remote port time out: removing target and saving 
binding
qla2xxx [:81:00.0]-505f:3: Link is operational (8 Gbps).
qla2xxx [:81:00.0]-8017:3: ADAPTER RESET SUCCEEDED nexus=3:0:2.
 rport-2:0-0: blocked FC remote port time out: removing target and saving 
binding
sg_rq_end_io: device detached
BUG: unable to handle kernel NULL pointer dereference at 02a8
IP: [8133b268] __pm_runtime_idle+0x28/0x90
PGD 7e6586f067 PUD 7e5af06067 PMD 0 [1739975.390354] Oops: 0002 [#1] SMP
CPU 0
...
Supported: No, Proprietary modules are loaded [1739975.390463]
Pid: 27965, comm: ABCD Tainted: PF   X 3.0.101-0.29-default #1 HP 
ProLiant DL580 Gen8
RIP: 0010:[8133b268]  [8133b268] __pm_runtime_idle+0x28/0x90
RSP: 0018:8839dc1e7c68  EFLAGS: 00010202
RAX:  RBX: 883f0592fc00 RCX: 0090
RDX:  RSI: 0004 RDI: 0138
RBP: 0138 R08: 0010 R09: 81bd39d0
R10: 09c0 R11: 81025790 R12: 0001
R13: 883022212b80 R14: 0004 R15: 883022212b80
FS:  7f8e54560720() GS:88407f80() knlGS:
CS:  0010 DS:  ES:  CR0: 8005003b
CR2: 02a8 CR3: 007e6ced6000 CR4: 001407f0
DR0:  DR1:  DR2: 
DR3:  DR6: 0ff0 DR7: 0400
Process ABCD (pid: 27965, threadinfo 8839dc1e6000, task 883592e0c640)
Stack:
 883f0592fc00 fffa 0001 883022212b80
 883eff772400 a03fa309  
 a04003a0 883f063196c0 887f0379a930 8115ea1e
Call Trace:
 [a03fa309] st_open+0x129/0x240 [st]
 [8115ea1e] chrdev_open+0x13e/0x200
 [811588a8] __dentry_open+0x198/0x310
 [81167d74] do_last+0x1f4/0x800
 [81168fe9] path_openat+0xd9/0x420
 [8116946c] do_filp_open+0x4c/0xc0
 [8115a00f] do_sys_open+0x17f/0x250
 [81468d92] system_call_fastpath+0x16/0x1b
 [7f8e4f617fd0] 0x7f8e4f617fcf
Code: eb d3 90 48 83 ec 28 40 f6 c6 04 48 89 6c 24 08 4c 89 74 24 20 48 89 fd 
48 89 1c 24 4c 89 64 24 10 41 89 f6 4c 89 6c 24 18 74 11 f0 ff 8f 70 01 00 00 
0f 94 c0 45 31 ed 84 c0 74 2b 4c 8d a5 a0
RIP  [8133b268] __pm_runtime_idle+0x28/0x90
 RSP 8839dc1e7c68
CR2: 02a8

Analysis reveals the cause of the crash to be due to STp-device
being NULL. The pointer was NULLed via scsi_tape_put(STp) when it
calls scsi_tape_release(). In st_open() we jump to err_out after
scsi_block_when_processing_errors() completes and returns the
device as offline (sdev_state was SDEV_DEL):

1180 /* Open the device. Needs to take the BKL only because of incrementing the 
SCSI host
1181module count. */
1182 static int st_open(struct inode *inode, struct file *filp)
1183 {
1184 int i, retval = (-EIO);
1185 int resumed = 0;
1186 struct scsi_tape *STp;
1187 struct st_partstat *STps;
1188 int dev = TAPE_NR(inode);
1189 char *name;
...
1217 if (scsi_autopm_get_device(STp-device)  0) {
1218 retval = -EIO;
1219 goto err_out;
1220 }
1221 resumed = 1;
1222 if (!scsi_block_when_processing_errors(STp-device)) {
1223 retval = (-ENXIO);
1224 goto err_out;
1225 }
...
1264  err_out:
1265 normalize_buffer(STp-buffer);
1266 spin_lock(st_use_lock);
1267 STp-in_use = 0;
1268 spin_unlock(st_use_lock);
1269 scsi_tape_put(STp); -- STp-device = 0 after this
1270 if (resumed)
1271 scsi_autopm_put_device(STp-device);
1272 return retval;

The ref count for the 

[PATCH v2] hpsa: convert DEVICE_ATTR to RO|WO|RW and show methods must not use snprintf

[Seymour, Shane M]

Changed DEVICE_ATTR macro usage to DEVICE_ATTR_RO|WO|RW.
This also forced some show/store function names to change.

Changed all show method snprintf() usage to scnprintf() per
Documentation/filesystems/sysfs.txt.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
Changes from v1:
Dropped one sprintf() to scnprintf() change in show method.
--- a/drivers/scsi/hpsa.c   2015-06-25 15:52:15.633031319 -0500
+++ b/drivers/scsi/hpsa.c   2015-06-30 15:06:14.664263348 -0500
@@ -376,7 +376,7 @@ static int check_for_busy(struct ctlr_in
 }
 
 static u32 lockup_detected(struct ctlr_info *h);
-static ssize_t host_show_lockup_detected(struct device *dev,
+static ssize_t lockup_detected_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
int ld;
@@ -389,7 +389,7 @@ static ssize_t host_show_lockup_detected
return sprintf(buf, ld=%d\n, ld);
 }
 
-static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
+static ssize_t hp_ssd_smart_path_status_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -413,7 +413,7 @@ static ssize_t host_store_hp_ssd_smart_p
return count;
 }
 
-static ssize_t host_store_raid_offload_debug(struct device *dev,
+static ssize_t raid_offload_debug_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -438,7 +438,7 @@ static ssize_t host_store_raid_offload_d
return count;
 }
 
-static ssize_t host_store_rescan(struct device *dev,
+static ssize_t rescan_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -449,7 +449,7 @@ static ssize_t host_store_rescan(struct
return count;
 }
 
-static ssize_t host_show_firmware_revision(struct device *dev,
+static ssize_t firmware_revision_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -460,40 +460,40 @@ static ssize_t host_show_firmware_revisi
if (!h-hba_inquiry_data)
return 0;
fwrev = h-hba_inquiry_data[32];
-   return snprintf(buf, 20, %c%c%c%c\n,
+   return scnprintf(buf, 20, %c%c%c%c\n,
fwrev[0], fwrev[1], fwrev[2], fwrev[3]);
 }
 
-static ssize_t host_show_commands_outstanding(struct device *dev,
+static ssize_t commands_outstanding_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct Scsi_Host *shost = class_to_shost(dev);
struct ctlr_info *h = shost_to_hba(shost);
 
-   return snprintf(buf, 20, %d\n,
+   return scnprintf(buf, 20, %d\n,
atomic_read(h-commands_outstanding));
 }
 
-static ssize_t host_show_transport_mode(struct device *dev,
+static ssize_t transport_mode_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 20, %s\n,
+   return scnprintf(buf, 20, %s\n,
h-transMethod  CFGTBL_Trans_Performant ?
performant : simple);
 }
 
-static ssize_t host_show_hp_ssd_smart_path_status(struct device *dev,
+static ssize_t hp_ssd_smart_path_status_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 30, HP SSD Smart Path %s\n,
+   return scnprintf(buf, 30, HP SSD Smart Path %s\n,
(h-acciopath_status == 1) ?  enabled : disabled);
 }
 
@@ -582,14 +582,14 @@ static int ctlr_needs_abort_tags_swizzle
ARRAY_SIZE(needs_abort_tags_swizzled), board_id);
 }
 
-static ssize_t host_show_resettable(struct device *dev,
+static ssize_t resettable_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 20, %d\n, ctlr_is_resettable(h-board_id));
+   return scnprintf(buf, 20, %d\n, ctlr_is_resettable(h-board_id));
 }
 
 static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
@@ -631,7 +631,7 @@ static ssize_t raid_level_show(struct de
/* Is this even a logical drive? */
if (!is_logical_dev_addr_mode(hdev-scsi3addr)) {
spin_unlock_irqrestore(h-lock, flags);
-   l = snprintf(buf, PAGE_SIZE, N/A\n);
+   l = scnprintf(buf, PAGE_SIZE, N/A\n);
return l;
}
 
@@ -639,7 +639,7 @@ static ssize_t raid_level_show(struct de

[PATCH 3/3] hpsa: Convert DEVICE_ATTR macro usage to DEVICE_ATTR_RO|WR|WO

[Seymour, Shane M]

Convert DEVICE_ATTR macro usage to DEVICE_ATTR_RO|WR|WO
Changes forced some function names to change.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
--- a/drivers/scsi/hpsa.c   2015-06-30 16:34:01.403904650 -0500
+++ b/drivers/scsi/hpsa.c   2015-06-30 16:21:54.214954176 -0500
@@ -376,7 +376,7 @@ static int check_for_busy(struct ctlr_in
 }
 
 static u32 lockup_detected(struct ctlr_info *h);
-static ssize_t host_show_lockup_detected(struct device *dev,
+static ssize_t lockup_detected_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
int ld;
@@ -389,7 +389,7 @@ static ssize_t host_show_lockup_detected
return sprintf(buf, ld=%d\n, ld);
 }
 
-static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
+static ssize_t hp_ssd_smart_path_status_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -413,7 +413,7 @@ static ssize_t host_store_hp_ssd_smart_p
return count;
 }
 
-static ssize_t host_store_raid_offload_debug(struct device *dev,
+static ssize_t raid_offload_debug_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -438,7 +438,7 @@ static ssize_t host_store_raid_offload_d
return count;
 }
 
-static ssize_t host_store_rescan(struct device *dev,
+static ssize_t rescan_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -449,7 +449,7 @@ static ssize_t host_store_rescan(struct
return count;
 }
 
-static ssize_t host_show_firmware_revision(struct device *dev,
+static ssize_t firmware_revision_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -464,7 +464,7 @@ static ssize_t host_show_firmware_revisi
fwrev[0], fwrev[1], fwrev[2], fwrev[3]);
 }
 
-static ssize_t host_show_commands_outstanding(struct device *dev,
+static ssize_t commands_outstanding_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct Scsi_Host *shost = class_to_shost(dev);
@@ -474,7 +474,7 @@ static ssize_t host_show_commands_outsta
atomic_read(h-commands_outstanding));
 }
 
-static ssize_t host_show_transport_mode(struct device *dev,
+static ssize_t transport_mode_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -486,7 +486,7 @@ static ssize_t host_show_transport_mode(
performant : simple);
 }
 
-static ssize_t host_show_hp_ssd_smart_path_status(struct device *dev,
+static ssize_t hp_ssd_smart_path_status_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -582,7 +582,7 @@ static int ctlr_needs_abort_tags_swizzle
ARRAY_SIZE(needs_abort_tags_swizzled), board_id);
 }
 
-static ssize_t host_show_resettable(struct device *dev,
+static ssize_t resettable_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -692,7 +692,7 @@ static ssize_t unique_id_show(struct dev
sn[12], sn[13], sn[14], sn[15]);
 }
 
-static ssize_t host_show_hp_ssd_smart_path_enabled(struct device *dev,
+static ssize_t hp_ssd_smart_path_enabled_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -714,27 +714,18 @@ static ssize_t host_show_hp_ssd_smart_pa
return scnprintf(buf, 20, %d\n, offload_enabled);
 }
 
-static DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);
-static DEVICE_ATTR(lunid, S_IRUGO, lunid_show, NULL);
-static DEVICE_ATTR(unique_id, S_IRUGO, unique_id_show, NULL);
-static DEVICE_ATTR(rescan, S_IWUSR, NULL, host_store_rescan);
-static DEVICE_ATTR(hp_ssd_smart_path_enabled, S_IRUGO,
-   host_show_hp_ssd_smart_path_enabled, NULL);
-static DEVICE_ATTR(hp_ssd_smart_path_status, S_IWUSR|S_IRUGO|S_IROTH,
-   host_show_hp_ssd_smart_path_status,
-   host_store_hp_ssd_smart_path_status);
-static DEVICE_ATTR(raid_offload_debug, S_IWUSR, NULL,
-   host_store_raid_offload_debug);
-static DEVICE_ATTR(firmware_revision, S_IRUGO,
-   host_show_firmware_revision, NULL);
-static DEVICE_ATTR(commands_outstanding, S_IRUGO,
-   host_show_commands_outstanding, NULL);
-static DEVICE_ATTR(transport_mode, S_IRUGO,
-   host_show_transport_mode, NULL);
-static DEVICE_ATTR(resettable, S_IRUGO,
-   host_show_resettable, NULL);
-static DEVICE_ATTR(lockup_detected, S_IRUGO,
-   host_show_lockup_detected, NULL);
+static DEVICE_ATTR_RO(raid_level);
+static DEVICE_ATTR_RO(lunid);
+static 

[PATCH 2/3] hpsa: Remove unneccessary variable from raid_level_show

[Seymour, Shane M]

Remove unneccessary variable from raid_level_show

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
Was not in previous patch.
--- a/drivers/scsi/hpsa.c   2015-06-30 16:15:42.631979483 -0500
+++ b/drivers/scsi/hpsa.c   2015-06-30 16:16:45.737975186 -0500
@@ -612,7 +612,6 @@ static const char * const raid_label[] =
 static ssize_t raid_level_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
-   ssize_t l = 0;
unsigned char rlevel;
struct ctlr_info *h;
struct scsi_device *sdev;
@@ -631,16 +630,14 @@ static ssize_t raid_level_show(struct de
/* Is this even a logical drive? */
if (!is_logical_dev_addr_mode(hdev-scsi3addr)) {
spin_unlock_irqrestore(h-lock, flags);
-   l = scnprintf(buf, PAGE_SIZE, N/A\n);
-   return l;
+   return scnprintf(buf, PAGE_SIZE, N/A\n);
}
 
rlevel = hdev-raid_level;
spin_unlock_irqrestore(h-lock, flags);
if (rlevel  RAID_UNKNOWN)
rlevel = RAID_UNKNOWN;
-   l = scnprintf(buf, PAGE_SIZE, RAID %s\n, raid_label[rlevel]);
-   return l;
+   return scnprintf(buf, PAGE_SIZE, RAID %s\n, raid_label[rlevel]);
 }
 
 static ssize_t lunid_show(struct device *dev,
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] hpsa: convert show method snprintf usage to scnprintf

[Seymour, Shane M]

Changed all show method snprintf usage to scnprintf per
Documentation/filesystems/sysfs.txt.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
Please let me know if this is not the correct way to submit
patches by separating them but keeping them logically
together.
--- a/drivers/scsi/hpsa.c   2015-06-25 15:52:15.633031319 -0500
+++ b/drivers/scsi/hpsa.c   2015-06-30 16:12:58.125990687 -0500
@@ -460,7 +460,7 @@ static ssize_t host_show_firmware_revisi
if (!h-hba_inquiry_data)
return 0;
fwrev = h-hba_inquiry_data[32];
-   return snprintf(buf, 20, %c%c%c%c\n,
+   return scnprintf(buf, 20, %c%c%c%c\n,
fwrev[0], fwrev[1], fwrev[2], fwrev[3]);
 }
 
@@ -470,7 +470,7 @@ static ssize_t host_show_commands_outsta
struct Scsi_Host *shost = class_to_shost(dev);
struct ctlr_info *h = shost_to_hba(shost);
 
-   return snprintf(buf, 20, %d\n,
+   return scnprintf(buf, 20, %d\n,
atomic_read(h-commands_outstanding));
 }
 
@@ -481,7 +481,7 @@ static ssize_t host_show_transport_mode(
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 20, %s\n,
+   return scnprintf(buf, 20, %s\n,
h-transMethod  CFGTBL_Trans_Performant ?
performant : simple);
 }
@@ -493,7 +493,7 @@ static ssize_t host_show_hp_ssd_smart_pa
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 30, HP SSD Smart Path %s\n,
+   return scnprintf(buf, 30, HP SSD Smart Path %s\n,
(h-acciopath_status == 1) ?  enabled : disabled);
 }
 
@@ -589,7 +589,7 @@ static ssize_t host_show_resettable(stru
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 20, %d\n, ctlr_is_resettable(h-board_id));
+   return scnprintf(buf, 20, %d\n, ctlr_is_resettable(h-board_id));
 }
 
 static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
@@ -631,7 +631,7 @@ static ssize_t raid_level_show(struct de
/* Is this even a logical drive? */
if (!is_logical_dev_addr_mode(hdev-scsi3addr)) {
spin_unlock_irqrestore(h-lock, flags);
-   l = snprintf(buf, PAGE_SIZE, N/A\n);
+   l = scnprintf(buf, PAGE_SIZE, N/A\n);
return l;
}
 
@@ -639,7 +639,7 @@ static ssize_t raid_level_show(struct de
spin_unlock_irqrestore(h-lock, flags);
if (rlevel  RAID_UNKNOWN)
rlevel = RAID_UNKNOWN;
-   l = snprintf(buf, PAGE_SIZE, RAID %s\n, raid_label[rlevel]);
+   l = scnprintf(buf, PAGE_SIZE, RAID %s\n, raid_label[rlevel]);
return l;
 }
 
@@ -662,7 +662,7 @@ static ssize_t lunid_show(struct device
}
memcpy(lunid, hdev-scsi3addr, sizeof(lunid));
spin_unlock_irqrestore(h-lock, flags);
-   return snprintf(buf, 20, 0x%02x%02x%02x%02x%02x%02x%02x%02x\n,
+   return scnprintf(buf, 20, 0x%02x%02x%02x%02x%02x%02x%02x%02x\n,
lunid[0], lunid[1], lunid[2], lunid[3],
lunid[4], lunid[5], lunid[6], lunid[7]);
 }
@@ -686,7 +686,7 @@ static ssize_t unique_id_show(struct dev
}
memcpy(sn, hdev-device_id, sizeof(sn));
spin_unlock_irqrestore(h-lock, flags);
-   return snprintf(buf, 16 * 2 + 2,
+   return scnprintf(buf, 16 * 2 + 2,
%02X%02X%02X%02X%02X%02X%02X%02X
%02X%02X%02X%02X%02X%02X%02X%02X\n,
sn[0], sn[1], sn[2], sn[3],
@@ -714,7 +714,7 @@ static ssize_t host_show_hp_ssd_smart_pa
}
offload_enabled = hdev-offload_enabled;
spin_unlock_irqrestore(h-lock, flags);
-   return snprintf(buf, 20, %d\n, offload_enabled);
+   return scnprintf(buf, 20, %d\n, offload_enabled);
 }
 
 static DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] hpsa: convert DEVICE_ATTR to RO|WO|RW and show methods must use scnprintf

[Seymour, Shane M]

Changed DEVICE_ATTR macro usage to DEVICE_ATTR_RO|WO|RW.
This also forced some show/store function names to change.

Changed all show method sprint/snprintf usage to scnprintf per
Documentation/filesystems/sysfs.txt.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
--- a/drivers/scsi/hpsa.c   2015-06-25 15:52:15.633031319 -0500
+++ b/drivers/scsi/hpsa.c   2015-06-29 17:28:24.628475369 -0500
@@ -376,7 +376,7 @@ static int check_for_busy(struct ctlr_in
 }
 
 static u32 lockup_detected(struct ctlr_info *h);
-static ssize_t host_show_lockup_detected(struct device *dev,
+static ssize_t lockup_detected_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
int ld;
@@ -386,10 +386,10 @@ static ssize_t host_show_lockup_detected
h = shost_to_hba(shost);
ld = lockup_detected(h);
 
-   return sprintf(buf, ld=%d\n, ld);
+   return scnprintf(buf, PAGE_SIZE, ld=%d\n, ld);
 }
 
-static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
+static ssize_t hp_ssd_smart_path_status_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -413,7 +413,7 @@ static ssize_t host_store_hp_ssd_smart_p
return count;
 }
 
-static ssize_t host_store_raid_offload_debug(struct device *dev,
+static ssize_t raid_offload_debug_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -438,7 +438,7 @@ static ssize_t host_store_raid_offload_d
return count;
 }
 
-static ssize_t host_store_rescan(struct device *dev,
+static ssize_t rescan_store(struct device *dev,
 struct device_attribute *attr,
 const char *buf, size_t count)
 {
@@ -449,7 +449,7 @@ static ssize_t host_store_rescan(struct
return count;
 }
 
-static ssize_t host_show_firmware_revision(struct device *dev,
+static ssize_t firmware_revision_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
@@ -460,40 +460,40 @@ static ssize_t host_show_firmware_revisi
if (!h-hba_inquiry_data)
return 0;
fwrev = h-hba_inquiry_data[32];
-   return snprintf(buf, 20, %c%c%c%c\n,
+   return scnprintf(buf, 20, %c%c%c%c\n,
fwrev[0], fwrev[1], fwrev[2], fwrev[3]);
 }
 
-static ssize_t host_show_commands_outstanding(struct device *dev,
+static ssize_t commands_outstanding_show(struct device *dev,
 struct device_attribute *attr, char *buf)
 {
struct Scsi_Host *shost = class_to_shost(dev);
struct ctlr_info *h = shost_to_hba(shost);
 
-   return snprintf(buf, 20, %d\n,
+   return scnprintf(buf, 20, %d\n,
atomic_read(h-commands_outstanding));
 }
 
-static ssize_t host_show_transport_mode(struct device *dev,
+static ssize_t transport_mode_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 20, %s\n,
+   return scnprintf(buf, 20, %s\n,
h-transMethod  CFGTBL_Trans_Performant ?
performant : simple);
 }
 
-static ssize_t host_show_hp_ssd_smart_path_status(struct device *dev,
+static ssize_t hp_ssd_smart_path_status_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 30, HP SSD Smart Path %s\n,
+   return scnprintf(buf, 30, HP SSD Smart Path %s\n,
(h-acciopath_status == 1) ?  enabled : disabled);
 }
 
@@ -582,14 +582,14 @@ static int ctlr_needs_abort_tags_swizzle
ARRAY_SIZE(needs_abort_tags_swizzled), board_id);
 }
 
-static ssize_t host_show_resettable(struct device *dev,
+static ssize_t resettable_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
 
h = shost_to_hba(shost);
-   return snprintf(buf, 20, %d\n, ctlr_is_resettable(h-board_id));
+   return scnprintf(buf, 20, %d\n, ctlr_is_resettable(h-board_id));
 }
 
 static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
@@ -631,7 +631,7 @@ static ssize_t raid_level_show(struct de
/* Is this even a logical drive? */
if (!is_logical_dev_addr_mode(hdev-scsi3addr)) {
spin_unlock_irqrestore(h-lock, flags);
-   l = snprintf(buf, PAGE_SIZE, N/A\n);
+   l = scnprintf(buf, PAGE_SIZE, N/A\n);
return l;
}
 
@@ -639,7 +639,7 @@ static ssize_t 

scsi_dh: convert __ATTR macro to DEVICE_ATTR_RW and snprintf show method cleanup

[Seymour, Shane M]

Converted dh_state to use DEVICE_ATTR_RW macro
instead of __ATTR.  That forced a change to the associated
show/store function names and the name of the attribute.

Changed usage of snprintf in show function to scnprintf per
Documentation/filesystems/sysfs.txt.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
--- a/drivers/scsi/device_handler/scsi_dh.c 2015-06-25 15:52:15.616031320 
-0500
+++ b/drivers/scsi/device_handler/scsi_dh.c 2015-06-25 18:10:54.246839220 
-0500
@@ -174,7 +174,7 @@ static void scsi_dh_handler_detach(struc
  * Functions for sysfs attribute 'dh_state'
  */
 static ssize_t
-store_dh_state(struct device *dev, struct device_attribute *attr,
+dh_state_store(struct device *dev, struct device_attribute *attr,
   const char *buf, size_t count)
 {
struct scsi_device *sdev = to_scsi_device(dev);
@@ -215,19 +215,17 @@ store_dh_state(struct device *dev, struc
 }
 
 static ssize_t
-show_dh_state(struct device *dev, struct device_attribute *attr, char *buf)
+dh_state_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct scsi_device *sdev = to_scsi_device(dev);
 
if (!sdev-scsi_dh_data)
-   return snprintf(buf, 20, detached\n);
+   return scnprintf(buf, 20, detached\n);
 
-   return snprintf(buf, 20, %s\n, sdev-scsi_dh_data-scsi_dh-name);
+   return scnprintf(buf, 20, %s\n, sdev-scsi_dh_data-scsi_dh-name);
 }
 
-static struct device_attribute scsi_dh_state_attr =
-   __ATTR(dh_state, S_IRUGO | S_IWUSR, show_dh_state,
-  store_dh_state);
+static DEVICE_ATTR_RW(dh_state);
 
 /*
  * scsi_dh_sysfs_attr_add - Callback for scsi_init_dh
@@ -243,7 +241,7 @@ static int scsi_dh_sysfs_attr_add(struct
sdev = to_scsi_device(dev);
 
err = device_create_file(sdev-sdev_gendev,
-scsi_dh_state_attr);
+   dev_attr_dh_state);
 
return 0;
 }
@@ -261,7 +259,7 @@ static int scsi_dh_sysfs_attr_remove(str
sdev = to_scsi_device(dev);
 
device_remove_file(sdev-sdev_gendev,
-  scsi_dh_state_attr);
+   dev_attr_dh_state);
 
return 0;
 }
@@ -283,13 +281,13 @@ static int scsi_dh_notifier(struct notif
sdev = to_scsi_device(dev);
 
if (action == BUS_NOTIFY_ADD_DEVICE) {
-   err = device_create_file(dev, scsi_dh_state_attr);
+   err = device_create_file(dev, dev_attr_dh_state);
/* don't care about err */
devinfo = device_handler_match(NULL, sdev);
if (devinfo)
err = scsi_dh_handler_attach(sdev, devinfo);
} else if (action == BUS_NOTIFY_DEL_DEVICE) {
-   device_remove_file(dev, scsi_dh_state_attr);
+   device_remove_file(dev, dev_attr_dh_state);
scsi_dh_handler_detach(sdev, NULL);
}
return err;
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi_dh: convert __ATTR macro to DEVICE_ATTR_RW and snprintf show method cleanup

[Seymour, Shane M]

Converted dh_state to use DEVICE_ATTR_RW macro
instead of __ATTR.  That forced a change to the associated
show/store function names and the name of the attribute.

Changed usage of snprintf in show function to scnprintf per
Documentation/filesystems/sysfs.txt.

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
--- a/drivers/scsi/device_handler/scsi_dh.c 2015-06-25 15:52:15.616031320 
-0500
+++ b/drivers/scsi/device_handler/scsi_dh.c 2015-06-25 18:10:54.246839220 
-0500
@@ -174,7 +174,7 @@ static void scsi_dh_handler_detach(struc
  * Functions for sysfs attribute 'dh_state'
  */
 static ssize_t
-store_dh_state(struct device *dev, struct device_attribute *attr,
+dh_state_store(struct device *dev, struct device_attribute *attr,
   const char *buf, size_t count)
 {
struct scsi_device *sdev = to_scsi_device(dev);
@@ -215,19 +215,17 @@ store_dh_state(struct device *dev, struc
 }
 
 static ssize_t
-show_dh_state(struct device *dev, struct device_attribute *attr, char *buf)
+dh_state_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct scsi_device *sdev = to_scsi_device(dev);
 
if (!sdev-scsi_dh_data)
-   return snprintf(buf, 20, detached\n);
+   return scnprintf(buf, 20, detached\n);
 
-   return snprintf(buf, 20, %s\n, sdev-scsi_dh_data-scsi_dh-name);
+   return scnprintf(buf, 20, %s\n, sdev-scsi_dh_data-scsi_dh-name);
 }
 
-static struct device_attribute scsi_dh_state_attr =
-   __ATTR(dh_state, S_IRUGO | S_IWUSR, show_dh_state,
-  store_dh_state);
+static DEVICE_ATTR_RW(dh_state);
 
 /*
  * scsi_dh_sysfs_attr_add - Callback for scsi_init_dh
@@ -243,7 +241,7 @@ static int scsi_dh_sysfs_attr_add(struct
sdev = to_scsi_device(dev);
 
err = device_create_file(sdev-sdev_gendev,
-scsi_dh_state_attr);
+   dev_attr_dh_state);
 
return 0;
 }
@@ -261,7 +259,7 @@ static int scsi_dh_sysfs_attr_remove(str
sdev = to_scsi_device(dev);
 
device_remove_file(sdev-sdev_gendev,
-  scsi_dh_state_attr);
+   dev_attr_dh_state);
 
return 0;
 }
@@ -283,13 +281,13 @@ static int scsi_dh_notifier(struct notif
sdev = to_scsi_device(dev);
 
if (action == BUS_NOTIFY_ADD_DEVICE) {
-   err = device_create_file(dev, scsi_dh_state_attr);
+   err = device_create_file(dev, dev_attr_dh_state);
/* don't care about err */
devinfo = device_handler_match(NULL, sdev);
if (devinfo)
err = scsi_dh_handler_attach(sdev, devinfo);
} else if (action == BUS_NOTIFY_DEL_DEVICE) {
-   device_remove_file(dev, scsi_dh_state_attr);
+   device_remove_file(dev, dev_attr_dh_state);
scsi_dh_handler_detach(sdev, NULL);
}
return err;
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Seymour, Shane M]

Convert DRIVER_ATTR macros to DRIVER_ATTR_RO requested by
Greg KH. Also switched to using scnprintf instead of snprintf
per Documentation/filesystems/sysfs.txt.

Suggested-by: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Shane Seymour shane.seym...@hp.com
---
This patch was implemented on top of the previous patch to
convert to using driver attr groups.

Changes from v1:
- switched to scnprintf from sprintf after feedback from Sergey
Senozhatsky.
--- a/drivers/scsi/st.c 2015-06-22 20:38:16.061386739 -0500
+++ b/drivers/scsi/st.c 2015-06-23 19:59:21.302775649 -0500
@@ -4427,29 +4427,29 @@ module_exit(exit_st);
 
 
 /* The sysfs driver interface. Read-only at the moment */
-static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
+static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, try_direct_io);
+   return scnprintf(buf, PAGE_SIZE, %d\n, try_direct_io);
 }
-static DRIVER_ATTR(try_direct_io, S_IRUGO, st_try_direct_io_show, NULL);
+static DRIVER_ATTR_RO(try_direct_io);
 
-static ssize_t st_fixed_buffer_size_show(struct device_driver *ddp, char *buf)
+static ssize_t fixed_buffer_size_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, st_fixed_buffer_size);
+   return scnprintf(buf, PAGE_SIZE, %d\n, st_fixed_buffer_size);
 }
-static DRIVER_ATTR(fixed_buffer_size, S_IRUGO, st_fixed_buffer_size_show, 
NULL);
+static DRIVER_ATTR_RO(fixed_buffer_size);
 
-static ssize_t st_max_sg_segs_show(struct device_driver *ddp, char *buf)
+static ssize_t max_sg_segs_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, st_max_sg_segs);
+   return scnprintf(buf, PAGE_SIZE, %d\n, st_max_sg_segs);
 }
-static DRIVER_ATTR(max_sg_segs, S_IRUGO, st_max_sg_segs_show, NULL);
+static DRIVER_ATTR_RO(max_sg_segs);
 
-static ssize_t st_version_show(struct device_driver *ddd, char *buf)
+static ssize_t version_show(struct device_driver *ddd, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, [%s]\n, verstr);
+   return scnprintf(buf, PAGE_SIZE, [%s]\n, verstr);
 }
-static DRIVER_ATTR(version, S_IRUGO, st_version_show, NULL);
+static DRIVER_ATTR_RO(version);
 
 static struct attribute *st_drv_attrs[] = {
driver_attr_try_direct_io.attr,
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Seymour, Shane M]

Convert DRIVER_ATTR macros to DRIVER_ATTR_RO as requested by
Greg KH. Also switched to using sprintf as nothing printed should
exceed PAGE_SIZE - based on feedback from Greg when implementing
show functions for tape stats.

Suggested-by: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Shane Seymour shane.seym...@hp.com
---
This patch was implemented on top of the previous patch to
convert to using driver attr groups.

Resending with [PATCH] at the front since I forgot to add it.
--- a/drivers/scsi/st.c 2015-06-22 20:38:16.061386739 -0500
+++ b/drivers/scsi/st.c 2015-06-23 17:29:03.547867682 -0500
@@ -4427,29 +4427,29 @@ module_exit(exit_st);
 
 
 /* The sysfs driver interface. Read-only at the moment */
-static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
+static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, try_direct_io);
+   return sprintf(buf, %d\n, try_direct_io);
 }
-static DRIVER_ATTR(try_direct_io, S_IRUGO, st_try_direct_io_show, NULL);
+static DRIVER_ATTR_RO(try_direct_io);
 
-static ssize_t st_fixed_buffer_size_show(struct device_driver *ddp, char *buf)
+static ssize_t fixed_buffer_size_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, st_fixed_buffer_size);
+   return sprintf(buf, %d\n, st_fixed_buffer_size);
 }
-static DRIVER_ATTR(fixed_buffer_size, S_IRUGO, st_fixed_buffer_size_show, 
NULL);
+static DRIVER_ATTR_RO(fixed_buffer_size);
 
-static ssize_t st_max_sg_segs_show(struct device_driver *ddp, char *buf)
+static ssize_t max_sg_segs_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, st_max_sg_segs);
+   return sprintf(buf, %d\n, st_max_sg_segs);
 }
-static DRIVER_ATTR(max_sg_segs, S_IRUGO, st_max_sg_segs_show, NULL);
+static DRIVER_ATTR_RO(max_sg_segs);
 
-static ssize_t st_version_show(struct device_driver *ddd, char *buf)
+static ssize_t version_show(struct device_driver *ddd, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, [%s]\n, verstr);
+   return sprintf(buf, [%s]\n, verstr);
 }
-static DRIVER_ATTR(version, S_IRUGO, st_version_show, NULL);
+static DRIVER_ATTR_RO(version);
 
 static struct attribute *st_drv_attrs[] = {
driver_attr_try_direct_io.attr,
--
To unsubscribe from this list: send the line unsubscribe linux-api in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Seymour, Shane M]

Convert DRIVER_ATTR macros to DRIVER_ATTR_RO as requested by
Greg KH. Also switched to using sprintf as nothing printed should
exceed PAGE_SIZE - based on feedback from Greg when implementing
show functions for tape stats.

Suggested-by: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Shane Seymour shane.seym...@hp.com
---
This patch was implemented on top of the previous patch to
convert to using driver attr groups.
--- a/drivers/scsi/st.c 2015-06-22 20:38:16.061386739 -0500
+++ b/drivers/scsi/st.c 2015-06-23 17:29:03.547867682 -0500
@@ -4427,29 +4427,29 @@ module_exit(exit_st);
 
 
 /* The sysfs driver interface. Read-only at the moment */
-static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
+static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, try_direct_io);
+   return sprintf(buf, %d\n, try_direct_io);
 }
-static DRIVER_ATTR(try_direct_io, S_IRUGO, st_try_direct_io_show, NULL);
+static DRIVER_ATTR_RO(try_direct_io);
 
-static ssize_t st_fixed_buffer_size_show(struct device_driver *ddp, char *buf)
+static ssize_t fixed_buffer_size_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, st_fixed_buffer_size);
+   return sprintf(buf, %d\n, st_fixed_buffer_size);
 }
-static DRIVER_ATTR(fixed_buffer_size, S_IRUGO, st_fixed_buffer_size_show, 
NULL);
+static DRIVER_ATTR_RO(fixed_buffer_size);
 
-static ssize_t st_max_sg_segs_show(struct device_driver *ddp, char *buf)
+static ssize_t max_sg_segs_show(struct device_driver *ddp, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, %d\n, st_max_sg_segs);
+   return sprintf(buf, %d\n, st_max_sg_segs);
 }
-static DRIVER_ATTR(max_sg_segs, S_IRUGO, st_max_sg_segs_show, NULL);
+static DRIVER_ATTR_RO(max_sg_segs);
 
-static ssize_t st_version_show(struct device_driver *ddd, char *buf)
+static ssize_t version_show(struct device_driver *ddd, char *buf)
 {
-   return snprintf(buf, PAGE_SIZE, [%s]\n, verstr);
+   return sprintf(buf, [%s]\n, verstr);
 }
-static DRIVER_ATTR(version, S_IRUGO, st_version_show, NULL);
+static DRIVER_ATTR_RO(version);
 
 static struct attribute *st_drv_attrs[] = {
driver_attr_try_direct_io.attr,
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] st: convert to using driver attr groups for sysfs

[Seymour, Shane M]

This patch changes the st driver to use attribute groups so
driver sysfs files are created automatically. See the
following for reference:

http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
--- a/drivers/scsi/st.c 2015-06-22 14:20:40.829612661 -0500
+++ b/drivers/scsi/st.c 2015-06-22 15:49:49.357248393 -0500
@@ -85,6 +85,7 @@ static int debug_flag;
 
 static struct class st_sysfs_class;
 static const struct attribute_group *st_dev_groups[];
+static const struct attribute_group *st_drv_groups[];
 
 MODULE_AUTHOR(Kai Makisara);
 MODULE_DESCRIPTION(SCSI tape (st) driver);
@@ -198,15 +199,13 @@ static int sgl_unmap_user_pages(struct s
 static int st_probe(struct device *);
 static int st_remove(struct device *);
 
-static int do_create_sysfs_files(void);
-static void do_remove_sysfs_files(void);
-
 static struct scsi_driver st_template = {
.gendrv = {
.name   = st,
.owner  = THIS_MODULE,
.probe  = st_probe,
.remove = st_remove,
+   .groups = st_drv_groups,
},
 };
 
@@ -4404,14 +4403,8 @@ static int __init init_st(void)
if (err)
goto err_chrdev;
 
-   err = do_create_sysfs_files();
-   if (err)
-   goto err_scsidrv;
-
return 0;
 
-err_scsidrv:
-   scsi_unregister_driver(st_template.gendrv);
 err_chrdev:
unregister_chrdev_region(MKDEV(SCSI_TAPE_MAJOR, 0),
 ST_MAX_TAPE_ENTRIES);
@@ -4422,7 +4415,6 @@ err_class:
 
 static void __exit exit_st(void)
 {
-   do_remove_sysfs_files();
scsi_unregister_driver(st_template.gendrv);
unregister_chrdev_region(MKDEV(SCSI_TAPE_MAJOR, 0),
 ST_MAX_TAPE_ENTRIES);
@@ -4459,44 +4451,14 @@ static ssize_t st_version_show(struct de
 }
 static DRIVER_ATTR(version, S_IRUGO, st_version_show, NULL);
 
-static int do_create_sysfs_files(void)
-{
-   struct device_driver *sysfs = st_template.gendrv;
-   int err;
-
-   err = driver_create_file(sysfs, driver_attr_try_direct_io);
-   if (err)
-   return err;
-   err = driver_create_file(sysfs, driver_attr_fixed_buffer_size);
-   if (err)
-   goto err_try_direct_io;
-   err = driver_create_file(sysfs, driver_attr_max_sg_segs);
-   if (err)
-   goto err_attr_fixed_buf;
-   err = driver_create_file(sysfs, driver_attr_version);
-   if (err)
-   goto err_attr_max_sg;
-
-   return 0;
-
-err_attr_max_sg:
-   driver_remove_file(sysfs, driver_attr_max_sg_segs);
-err_attr_fixed_buf:
-   driver_remove_file(sysfs, driver_attr_fixed_buffer_size);
-err_try_direct_io:
-   driver_remove_file(sysfs, driver_attr_try_direct_io);
-   return err;
-}
-
-static void do_remove_sysfs_files(void)
-{
-   struct device_driver *sysfs = st_template.gendrv;
-
-   driver_remove_file(sysfs, driver_attr_version);
-   driver_remove_file(sysfs, driver_attr_max_sg_segs);
-   driver_remove_file(sysfs, driver_attr_fixed_buffer_size);
-   driver_remove_file(sysfs, driver_attr_try_direct_io);
-}
+static struct attribute *st_drv_attrs[] = {
+   driver_attr_try_direct_io.attr,
+   driver_attr_fixed_buffer_size.attr,
+   driver_attr_max_sg_segs.attr,
+   driver_attr_version.attr,
+   NULL,
+};
+ATTRIBUTE_GROUPS(st_drv);
 
 /* The sysfs simple class interface */
 static ssize_t
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

[Seymour, Shane M]

With a size of 48 while it won't overflow since you're using snprintf the 
string with a maximum value in %d:

echo -n cmd 2147483647 RESET FAILED, new lockup detected |wc -c
48

is 48 characters long without a null terminator on the string (and in the 
unlikely event that it's somehow a the largest possible negative value it would 
be 49 characters after including the minus sign without a null terminator). If 
you always want a complete message no matter what the value formatted as %d 
will be I believe it needs to have a length of 50. The worst that will happen 
now though because you're using snprintf is you'll drop the trailing d or 
ed in the message with very large positive or negative numbers.

My somewhat sketchy memory of the hpsa driver is that the nr_cmds member of the 
struct ctlr_info is never more than 1040 (?) anyway. If things are working 
correctly I don't think it should ever happen but I thought I should point out 
that msg isn't large enough to contain the complete contents of the longest 
possible character string.


-Original Message-
From: linux-scsi-ow...@vger.kernel.org 
[mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Don Brace
Sent: Thursday, June 18, 2015 11:36 PM
To: Dan Carpenter
Cc: James E.J. Bottomley; ISS StorageDev; dl Team ESD Storage Dev Support; 
linux-scsi@vger.kernel.org; kernel-janit...@vger.kernel.org
Subject: RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

 -Original Message-
 From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
 Sent: Thursday, June 04, 2015 9:48 AM
 To: Don Brace
 Cc: James E.J. Bottomley; iss_storage...@hp.com; dl Team ESD Storage 
 Dev Support; linux-scsi@vger.kernel.org; 
 kernel-janit...@vger.kernel.org
 Subject: [patch] hpsa: fix an sprintf() overflow in the reset handler
 
 The string cmd %d RESET FAILED, new lockup detected is not quite 
 large enough so the sprintf() will overflow.  I have increased the 
 size of the buffer and also changed the sprintf calls to snprintf.
 
 Fixes: 73153fe533bc ('hpsa: use block layer tag for command 
 allocation')
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 
 1dafeb4..cab4e98 100644
 --- a/drivers/scsi/hpsa.c
 +++ b/drivers/scsi/hpsa.c
 @@ -5104,7 +5104,7 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
   int rc;
   struct ctlr_info *h;
   struct hpsa_scsi_dev_t *dev;
 - char msg[40];
 + char msg[48];
 
   /* find the controller to which the command to be aborted was sent */
   h = sdev_to_hba(scsicmd-device);
 @@ -5122,16 +5122,18 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 
   /* if controller locked up, we can guarantee command won't complete 
 */
   if (lockup_detected(h)) {
 - sprintf(msg, cmd %d RESET FAILED, lockup detected,
 - hpsa_get_cmd_index(scsicmd));
 + snprintf(msg, sizeof(msg),
 +  cmd %d RESET FAILED, lockup detected,
 +  hpsa_get_cmd_index(scsicmd));
   hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
   return FAILED;
   }
 
   /* this reset request might be the result of a lockup; check */
   if (detect_controller_lockup(h)) {
 - sprintf(msg, cmd %d RESET FAILED, new lockup detected,
 - hpsa_get_cmd_index(scsicmd));
 + snprintf(msg, sizeof(msg),
 +  cmd %d RESET FAILED, new lockup detected,
 +  hpsa_get_cmd_index(scsicmd));
   hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
   return FAILED;
   }
 @@ -5145,7 +5147,8 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
   /* send a reset to the SCSI LUN which the command was sent to */
   rc = hpsa_do_reset(h, dev, dev-scsi3addr, HPSA_RESET_TYPE_LUN,
  DEFAULT_REPLY_QUEUE);
 - sprintf(msg, reset %s, rc == 0 ? completed successfully : failed);
 + snprintf(msg, sizeof(msg), reset %s,
 +  rc == 0 ? completed successfully : failed);
   hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
   return rc == 0 ? SUCCESS : FAILED;
  }

Signed-off-by: Don Brace don.br...@pmcs.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in the 
body of a message to majord...@vger.kernel.org More majordomo info at  
http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9] st: implement tape statistics

[Seymour, Shane M]

st: implement tape statistics

This patch implements tape statistics in the st module via
sysfs. Current no statistics are available for tape I/O and there
is no easy way to reuse the block layer statistics for tape
as tape is a character device and does not have perform I/O in
sector sized chunks (the size of the data written to tape
can change). For tapes we also need extra stats related to
things like tape movement (via other I/O).

There have been multiple end users requesting statistics
including ATT (and some HP customers who have not given
permission to be named). It is impossible for them
to investigate any issues related to tape performance
in a non-invasive way.

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
Reviewed-by: Christoph Hellwig h...@lst.de
---
Changes from v8
- Fixed warnings when printing statistics information in 32bit mode
by providing a PRId64 definition for 64 and 32 kernels. Used
PRId64 so if someone introduces those defines they will be able
to find this code and remove this definition of it.
- Updated kernel version given in sysfs ABI doco from ?? to 4.2.
Changes from v7
- Changed name of patch added colon
- Added better description of patch
- Removed tested by (myself) line
Changes from v6
- Removed tested by Laurence Oberman since the code has changed
significantly.
- Changed code to use ktime so time resolution is now in ns (Robert
Elliot) for virtual tape drives that may be able to submit and
complete an I/O within a jiffy.
- Changed code to use atomic64_t types for statistics to ensure
consistency between being updated and being read via sysfs (Robert
Elliot). This should help ensure that we do not have MP or
multi-threaded issues when accessing the data.
- Some tape stats structure members changed names after they
changed types to make their usage clearer.
- Sysfs stats files that changed from ms to ns changed names to
indicate that they were in ns now.
- Fixed typo in incremnented in sysfs description (Robert Elliot)
- Shorten statistics directory name to stats (Robert Elliot)
- Moved the update of in_flight so it's incremenented before we update
any statstics and decrememented after we're done.
- Missing braces between if/else added to more clearly show where
the if/else is.
- Tested on 4.0.0-next-20150423+
--- a/drivers/scsi/st.c 2015-04-22 18:13:13.097855752 -0500
+++ b/drivers/scsi/st.c 2015-05-31 17:52:08.891204751 -0500
@@ -471,6 +471,47 @@ static void st_release_request(struct st
kfree(streq);
 }
 
+static void st_do_stats(struct scsi_tape *STp, struct request *req)
+{
+   ktime_t now;
+
+   now = ktime_get();
+   if (req-cmd[0] == WRITE_6) {
+   now = ktime_sub(now, STp-stats-write_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_write_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_io_time);
+   atomic64_inc(STp-stats-write_cnt);
+   if (req-errors) {
+   atomic64_add(atomic_read(STp-stats-last_write_size)
+   - STp-buffer-cmdstat.residual,
+   STp-stats-write_byte_cnt);
+   if (STp-buffer-cmdstat.residual  0)
+   atomic64_inc(STp-stats-resid_cnt);
+   } else
+   atomic64_add(atomic_read(STp-stats-last_write_size),
+   STp-stats-write_byte_cnt);
+   } else if (req-cmd[0] == READ_6) {
+   now = ktime_sub(now, STp-stats-read_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_read_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_io_time);
+   atomic64_inc(STp-stats-read_cnt);
+   if (req-errors) {
+   atomic64_add(atomic_read(STp-stats-last_read_size)
+   - STp-buffer-cmdstat.residual,
+   STp-stats-read_byte_cnt);
+   if (STp-buffer-cmdstat.residual  0)
+   atomic64_inc(STp-stats-resid_cnt);
+   } else
+   atomic64_add(atomic_read(STp-stats-last_read_size),
+   STp-stats-read_byte_cnt);
+   } else {
+   now = ktime_sub(now, STp-stats-other_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_io_time);
+   atomic64_inc(STp-stats-other_cnt);
+   }
+   atomic64_dec(STp-stats-in_flight);
+}
+
 static void st_scsi_execute_end(struct request *req, int uptodate)
 {
struct st_request *SRpnt = req-end_io_data;
@@ -480,6 +521,8 @@ static void st_scsi_execute_end(struct r
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   st_do_stats(STp, req);
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 

RE: [scsi:misc 114/120] drivers/scsi/st.c:4594:3: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'long long int'

[Seymour, Shane M]

My apologies for this I'm fixing it now.

-Original Message-
From: kbuild test robot [mailto:fengguang...@intel.com] 
Sent: Monday, June 01, 2015 12:36 PM
To: Seymour, Shane M
Cc: kbuild-...@01.org; James Bottomley; Christoph Hellwig; 
linux-scsi@vger.kernel.org
Subject: [scsi:misc 114/120] drivers/scsi/st.c:4594:3: warning: format '%ld' 
expects argument of type 'long int', but argument 3 has type 'long long int'

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git misc
head:   f6e08d7c118cc44f10bffdf52a31d7f40ce86ba7
commit: fc5280a80c124fe2d8b661513670a574fcd8bcbc [114/120] st: implement tape 
statistics
config: i386-allyesconfig (attached as .config)
reproduce:
  git checkout fc5280a80c124fe2d8b661513670a574fcd8bcbc
  # save the attached .config to linux build tree
  make ARCH=i386 

All warnings:

   drivers/scsi/st.c: In function 'read_cnt_show':
 drivers/scsi/st.c:4594:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-read_cnt));
  ^
   drivers/scsi/st.c: In function 'read_byte_cnt_show':
 drivers/scsi/st.c:4612:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-read_byte_cnt));
  ^
   drivers/scsi/st.c: In function 'read_ns_show':
 drivers/scsi/st.c:4628:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-tot_read_time));
  ^
   drivers/scsi/st.c: In function 'write_cnt_show':
 drivers/scsi/st.c:4645:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-write_cnt));
  ^
   drivers/scsi/st.c: In function 'write_byte_cnt_show':
 drivers/scsi/st.c:4662:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-write_byte_cnt));
  ^
   drivers/scsi/st.c: In function 'write_ns_show':
 drivers/scsi/st.c:4679:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-tot_write_time));
  ^
   drivers/scsi/st.c: In function 'in_flight_show':
 drivers/scsi/st.c:4697:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-in_flight));
  ^
   drivers/scsi/st.c: In function 'io_ns_show':
 drivers/scsi/st.c:4717:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-tot_io_time));
  ^
   drivers/scsi/st.c: In function 'other_cnt_show':
 drivers/scsi/st.c:4736:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-other_cnt));
  ^
   drivers/scsi/st.c: In function 'resid_cnt_show':
 drivers/scsi/st.c:4754:3: warning: format '%ld' expects argument of type 
 'long int', but argument 3 has type 'long long int' [-Wformat=]
  atomic64_read(STm-tape-stats-resid_cnt));
  ^

vim +4594 drivers/scsi/st.c

  4588  static ssize_t read_cnt_show(struct device *dev,
  4589  struct device_attribute *attr, char *buf)
  4590  {
  4591  struct st_modedef *STm = dev_get_drvdata(dev);
  4592  
  4593  return sprintf(buf, %ld,
 4594  atomic64_read(STm-tape-stats-read_cnt));
  4595  }
  4596  static DEVICE_ATTR_RO(read_cnt);
  4597  
  4598  /**
  4599   * read_byte_cnt_show - return read byte count - tape drives
  4600   * may use blocks less than 512 bytes this gives the raw byte count of
  4601   * of data read from the tape drive.
  4602   * @dev: struct device
  4603   * @attr: attribute structure
  4604   * @buf: buffer to return formatted data in
  4605   */
  4606  static ssize_t read_byte_cnt_show(struct device *dev,
  4607  struct device_attribute *attr, char *buf)
  4608  {
  4609  struct st_modedef *STm = dev_get_drvdata(dev);
  4610  
  4611  return sprintf(buf, %ld,
 4612  atomic64_read(STm-tape-stats-read_byte_cnt));
  4613  }
  4614  static DEVICE_ATTR_RO(read_byte_cnt);
  4615  
  4616  /**
  4617   * read_us_show - return read us - overall time spent waiting on reads 
in ns.
  4618   * @dev: struct device
  4619   * @attr: attribute structure
  4620   * @buf: buffer to return formatted data in
  4621   */
  4622  static ssize_t read_ns_show(struct device *dev,
  4623  struct device_attribute *attr, char *buf)
  4624  {
  4625  struct st_modedef *STm = dev_get_drvdata(dev);
  4626  
  4627  return sprintf(buf, %ld,
 4628  atomic64_read(STm-tape-stats-tot_read_time));
  4629

[PATCH v7] st implement tape statistics

[Seymour, Shane M]

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
---
Changes from v6
- Removed tested by Laurence Oberman since the code has changed
significantly.
- Changed code to use ktime so time resolution is now in ns (Robert
Elliot) for virtual tape drives that may be able to submit and
complete an I/O within a jiffy.
- Changed code to use atomic64_t types for statistics to ensure
consistency between being updated and being read via sysfs (Robert
Elliot). This should help ensure that we do not have MP or
multi-threaded issues when accessing the data.
- Some tape stats structure members changed names after they
changed types to make their usage clearer.
- Sysfs stats files that changed from ms to ns changed names to
indicate that they were in ns now.
- Fixed typo in incremnented in sysfs description (Robert Elliot)
- Shorten statistics directory name to stats (Robert Elliot)
- Moved the update of in_flight so it's incremenented before we update
any statstics and decrememented after we're done.
- Missing braces between if/else added to more clearly show where
the if/else is.
- Tested on 4.0.0-next-20150423+
--- a/drivers/scsi/st.c 2015-03-04 18:04:34.428575747 -0600
+++ b/drivers/scsi/st.c 2015-04-22 17:04:43.908645984 -0500
@@ -471,6 +471,47 @@ static void st_release_request(struct st
kfree(streq);
 }
 
+static void st_do_stats(struct scsi_tape *STp, struct request *req)
+{
+   ktime_t now;
+
+   now = ktime_get();
+   if (req-cmd[0] == WRITE_6) {
+   now = ktime_sub(now, STp-stats-write_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_write_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_io_time);
+   atomic64_inc(STp-stats-write_cnt);
+   if (req-errors) {
+   atomic64_add(atomic_read(STp-stats-last_write_size)
+   - STp-buffer-cmdstat.residual,
+   STp-stats-write_byte_cnt);
+   if (STp-buffer-cmdstat.residual  0)
+   atomic64_inc(STp-stats-resid_cnt);
+   } else
+   atomic64_add(atomic_read(STp-stats-last_write_size),
+   STp-stats-write_byte_cnt);
+   } else if (req-cmd[0] == READ_6) {
+   now = ktime_sub(now, STp-stats-read_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_read_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_io_time);
+   atomic64_inc(STp-stats-read_cnt);
+   if (req-errors) {
+   atomic64_add(atomic_read(STp-stats-last_read_size)
+   - STp-buffer-cmdstat.residual,
+   STp-stats-read_byte_cnt);
+   if (STp-buffer-cmdstat.residual  0)
+   atomic64_inc(STp-stats-resid_cnt);
+   } else
+   atomic64_add(atomic_read(STp-stats-last_read_size),
+   STp-stats-read_byte_cnt);
+   } else {
+   now = ktime_sub(now, STp-stats-other_time);
+   atomic64_add(ktime_to_ns(now), STp-stats-tot_io_time);
+   atomic64_inc(STp-stats-other_cnt);
+   }
+   atomic64_dec(STp-stats-in_flight);
+}
+
 static void st_scsi_execute_end(struct request *req, int uptodate)
 {
struct st_request *SRpnt = req-end_io_data;
@@ -480,6 +521,8 @@ static void st_scsi_execute_end(struct r
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   st_do_stats(STp, req);
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +539,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +560,17 @@ static int st_scsi_execute(struct st_req
}
}
 
+   atomic64_inc(STp-stats-in_flight);
+   if (cmd[0] == WRITE_6) {
+   atomic_set(STp-stats-last_write_size, bufflen);
+   STp-stats-write_time = ktime_get();
+   } else if (cmd[0] == READ_6) {
+   atomic_set(STp-stats-last_read_size, bufflen);
+   STp-stats-read_time = ktime_get();
+   } else {
+   STp-stats-other_time = ktime_get();
+   }
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4222,6 +4277,12 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+   tpnt-stats = kzalloc(sizeof(struct 

[PATCH RESEND v6] st implement tape statistics

[Seymour, Shane M]

The following patch exposes statistics for the st driver via sysfs.
There is a need for companies with large numbers of tape drives
numbering in the tens of thousands to track the performance of
those tape drives (e.g. when a backup exceeds its window). The
statistics provided should allow the calculation of throughput,
average block sizes for read and write, and time spent waiting
for I/O to complete or doing tape movement.

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
Tested-by: Laurence Oberman lober...@redhat.com
---
- Removed comment
- Found an issue where reads and writes were over reported (fixed)
In all the test cases I have the stats now report what I expect to be
the correct value. Some of the values to be used with statistics
are now stored in temporary variables and used to calculate the
stats when the I/O ends. Separated out the timestamp into 3 since
I found it was possible for other tape I/O to happen during writes
updating the stamp value causing the time tracked to be wrong.
- Moved the end statistics into a separate function because it
had made the function that it was in too large.
- Added a new statistic - A count of the number of times we had
a resdual greater than 0.
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-11 22:37:01.382243090 -0600
@@ -471,6 +471,47 @@ static void st_release_request(struct st
kfree(streq);
 }
 
+static void st_do_stats(struct scsi_tape *STp, struct request *req)
+{
+   u64 ticks;
+
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   if (req-cmd[0] == WRITE_6) {
+   ticks -= STp-stats-write_stamp;
+   STp-stats-write_ticks += ticks;
+   STp-stats-io_ticks += ticks;
+   STp-stats-write_cnt++;
+   if (req-errors) {
+   STp-stats-write_byte_cnt +=
+   STp-stats-last_write_size -
+   STp-buffer-cmdstat.residual;
+   if (STp-buffer-cmdstat.residual  0)
+   STp-stats-resid_cnt++;
+   } else
+   STp-stats-write_byte_cnt +=
+   STp-stats-last_write_size;
+   } else if (req-cmd[0] == READ_6) {
+   ticks -= STp-stats-read_stamp;
+   STp-stats-read_ticks += ticks;
+   STp-stats-io_ticks += ticks;
+   STp-stats-read_cnt++;
+   if (req-errors)
+   STp-stats-read_byte_cnt +=
+   STp-stats-last_read_size -
+   STp-buffer-cmdstat.residual;
+   if (STp-buffer-cmdstat.residual  0)
+   STp-stats-resid_cnt++;
+   else
+   STp-stats-read_byte_cnt +=
+   STp-stats-last_read_size;
+   } else {
+   ticks -= STp-stats-other_stamp;
+   STp-stats-io_ticks += ticks;
+   STp-stats-other_cnt++;
+   }
+}
+
 static void st_scsi_execute_end(struct request *req, int uptodate)
 {
struct st_request *SRpnt = req-end_io_data;
@@ -480,6 +521,8 @@ static void st_scsi_execute_end(struct r
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   st_do_stats(STp, req);
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +539,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +560,17 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (cmd[0] == WRITE_6) {
+   STp-stats-last_write_size = bufflen;
+   STp-stats-write_stamp = get_jiffies_64();
+   } else if (cmd[0] == READ_6) {
+   STp-stats-last_read_size = bufflen;
+   STp-stats-read_stamp = get_jiffies_64();
+   } else {
+   STp-stats-other_stamp = get_jiffies_64();
+   }
+   STp-stats-in_flight++;
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4222,6 +4277,12 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+   tpnt-stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_KERNEL);
+   if (tpnt-stats == NULL) {
+   sdev_printk(KERN_ERR, SDp,
+   st: Can't allocate statistics.\n);
+   goto out_idr_remove;
+   }
 
dev_set_drvdata(dev, tpnt);
 
@@ 

RE: [PATCH v6] st implement tape statistics

[Seymour, Shane M]

Retested with patch applied to 4.0.0-rc2-next-20150304 - all successful with no 
issues found.

-Original Message-
From: Laurence Oberman [mailto:oberma...@gmail.com] 
Sent: Thursday, February 26, 2015 5:47 AM
To: Seymour, Shane M
Cc: Greg KH; linux-scsi@vger.kernel.org; Laurence Oberman 
(lober...@redhat.com); kai.makis...@kolumbus.fi; James E.J. Bottomley 
(jbottom...@parallels.com)
Subject: Re: [PATCH v6] st implement tape statistics

Hello,
I pulled the latest revision of this patch and tested it. I can vouch for it 
working as expected with out any obvious impact to the existing st driver Is 
there any way we can move this along.
Thanks

Tested-by:Laurence Oberman lober...@redhat.com

On Thu, Feb 12, 2015 at 6:15 AM, Seymour, Shane M shane.seym...@hp.com wrote:
 The following patch exposes statistics for the st driver via sysfs.
 There is a need for companies with large numbers of tape drives 
 numbering in the tens of thousands to track the performance of those 
 tape drives (e.g. when a backup exceeds its window). The statistics 
 provided should allow the calculation of throughput, average block 
 sizes for read and write, and time spent waiting for I/O to complete 
 or doing tape movement.

 Signed-off-by: Shane Seymour shane.seym...@hp.com
 Tested-by: Shane Seymour shane.seym...@hp.com
 ---
 - Removed comment
 - Found an issue where read and write sizes were over reported
 (fixed) In all the test cases I have the stats now report what I 
 expect to be the correct value. Some of the values to be used with 
 statistics are now stored in temporary variables and used to calculate 
 the stats when the I/O ends. Separated out the timestamp into 3 since 
 I found it was possible for other tape I/O to happen during writes 
 updating the stamp value causing the time tracked to be wrong.
 - Moved the end statistics into a separate function because it had 
 made the function that it was in too large.
 - Added a new statistic - A count of the number of times we had a 
 residual greater than 0.
 --- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
 +++ b/drivers/scsi/st.c 2015-02-11 22:37:01.382243090 -0600
 @@ -471,6 +471,47 @@ static void st_release_request(struct st
 kfree(streq);
  }

 +static void st_do_stats(struct scsi_tape *STp, struct request *req) {
 +   u64 ticks;
 +
 +   ticks = get_jiffies_64();
 +   STp-stats-in_flight--;
 +   if (req-cmd[0] == WRITE_6) {
 +   ticks -= STp-stats-write_stamp;
 +   STp-stats-write_ticks += ticks;
 +   STp-stats-io_ticks += ticks;
 +   STp-stats-write_cnt++;
 +   if (req-errors) {
 +   STp-stats-write_byte_cnt +=
 +   STp-stats-last_write_size -
 +   STp-buffer-cmdstat.residual;
 +   if (STp-buffer-cmdstat.residual  0)
 +   STp-stats-resid_cnt++;
 +   } else
 +   STp-stats-write_byte_cnt +=
 +   STp-stats-last_write_size;
 +   } else if (req-cmd[0] == READ_6) {
 +   ticks -= STp-stats-read_stamp;
 +   STp-stats-read_ticks += ticks;
 +   STp-stats-io_ticks += ticks;
 +   STp-stats-read_cnt++;
 +   if (req-errors)
 +   STp-stats-read_byte_cnt +=
 +   STp-stats-last_read_size -
 +   STp-buffer-cmdstat.residual;
 +   if (STp-buffer-cmdstat.residual  0)
 +   STp-stats-resid_cnt++;
 +   else
 +   STp-stats-read_byte_cnt +=
 +   STp-stats-last_read_size;
 +   } else {
 +   ticks -= STp-stats-other_stamp;
 +   STp-stats-io_ticks += ticks;
 +   STp-stats-other_cnt++;
 +   }
 +}
 +
  static void st_scsi_execute_end(struct request *req, int uptodate)  {
 struct st_request *SRpnt = req-end_io_data; @@ -480,6 +521,8 
 @@ static void st_scsi_execute_end(struct r
 STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
 STp-buffer-cmdstat.residual = req-resid_len;

 +   st_do_stats(STp, req);
 +
 tmp = SRpnt-bio;
 if (SRpnt-waiting)
 complete(SRpnt-waiting); @@ -496,6 +539,7 @@ static 
 int st_scsi_execute(struct st_req
 struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
 int err = 0;
 int write = (data_direction == DMA_TO_DEVICE);
 +   struct scsi_tape *STp = SRpnt-stp;

 req = blk_get_request(SRpnt-stp-device-request_queue, write,
   GFP_KERNEL); @@ -516,6 +560,17 @@ static 
 int st_scsi_execute(struct st_req
 }
 }

 +   if (cmd[0] == WRITE_6) {
 +   STp-stats-last_write_size = bufflen;
 +   STp-stats-write_stamp

[PATCH v6] st implement tape statistics

[Seymour, Shane M]

The following patch exposes statistics for the st driver via sysfs.
There is a need for companies with large numbers of tape drives
numbering in the tens of thousands to track the performance of
those tape drives (e.g. when a backup exceeds its window). The
statistics provided should allow the calculation of throughput,
average block sizes for read and write, and time spent waiting
for I/O to complete or doing tape movement.

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
---
- Removed comment
- Found an issue where read and write sizes were over reported
(fixed) In all the test cases I have the stats now report what I
expect to be the correct value. Some of the values to be used
with statistics are now stored in temporary variables and used
to calculate the stats when the I/O ends. Separated out the
timestamp into 3 since I found it was possible for other tape
I/O to happen during writes updating the stamp value causing
the time tracked to be wrong.
- Moved the end statistics into a separate function because it
had made the function that it was in too large.
- Added a new statistic - A count of the number of times we had
a residual greater than 0.
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-11 22:37:01.382243090 -0600
@@ -471,6 +471,47 @@ static void st_release_request(struct st
kfree(streq);
 }
 
+static void st_do_stats(struct scsi_tape *STp, struct request *req)
+{
+   u64 ticks;
+
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   if (req-cmd[0] == WRITE_6) {
+   ticks -= STp-stats-write_stamp;
+   STp-stats-write_ticks += ticks;
+   STp-stats-io_ticks += ticks;
+   STp-stats-write_cnt++;
+   if (req-errors) {
+   STp-stats-write_byte_cnt +=
+   STp-stats-last_write_size -
+   STp-buffer-cmdstat.residual;
+   if (STp-buffer-cmdstat.residual  0)
+   STp-stats-resid_cnt++;
+   } else
+   STp-stats-write_byte_cnt +=
+   STp-stats-last_write_size;
+   } else if (req-cmd[0] == READ_6) {
+   ticks -= STp-stats-read_stamp;
+   STp-stats-read_ticks += ticks;
+   STp-stats-io_ticks += ticks;
+   STp-stats-read_cnt++;
+   if (req-errors)
+   STp-stats-read_byte_cnt +=
+   STp-stats-last_read_size -
+   STp-buffer-cmdstat.residual;
+   if (STp-buffer-cmdstat.residual  0)
+   STp-stats-resid_cnt++;
+   else
+   STp-stats-read_byte_cnt +=
+   STp-stats-last_read_size;
+   } else {
+   ticks -= STp-stats-other_stamp;
+   STp-stats-io_ticks += ticks;
+   STp-stats-other_cnt++;
+   }
+}
+
 static void st_scsi_execute_end(struct request *req, int uptodate)
 {
struct st_request *SRpnt = req-end_io_data;
@@ -480,6 +521,8 @@ static void st_scsi_execute_end(struct r
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   st_do_stats(STp, req);
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +539,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +560,17 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (cmd[0] == WRITE_6) {
+   STp-stats-last_write_size = bufflen;
+   STp-stats-write_stamp = get_jiffies_64();
+   } else if (cmd[0] == READ_6) {
+   STp-stats-last_read_size = bufflen;
+   STp-stats-read_stamp = get_jiffies_64();
+   } else {
+   STp-stats-other_stamp = get_jiffies_64();
+   }
+   STp-stats-in_flight++;
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4222,6 +4277,12 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+   tpnt-stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_KERNEL);
+   if (tpnt-stats == NULL) {
+   sdev_printk(KERN_ERR, SDp,
+   st: Can't allocate statistics.\n);
+   goto out_idr_remove;
+   }
 
dev_set_drvdata(dev, tpnt);
 
@@ -4241,6 +4302,8 @@ static int 

Re: [RFC] implementing tape statistics single file vs multi-file in sysfs

[Bryn M. Reeves]

On Wed, Feb 11, 2015 at 06:30:27AM +0800, Greg KH wrote:
 On Tue, Feb 10, 2015 at 02:27:20PM +, Bryn M. Reeves wrote:
  On Sat, Feb 07, 2015 at 12:07:43PM +0800, Greg KH wrote:
  
  $ cat /sys/fs/selinux/avc/cache_stats 
  lookups hits misses allocations reclaims frees
  18938916 18921707 17209 17209 17328 22215
  38164283 38146514 17769 17769 16800 19049
  18078108 18056991 21117 21117 21344 19305
  15168204 15150079 18125 18125 17776 13149
  0 0 0 0 0 0
  0 0 0 0 0 0
  0 0 0 0 0 0
  0 0 0 0 0 0
  
  $ cat /sys/fs/selinux/avc/hash_stats
  entries: 506
  buckets used: 290/512
  longest chain: 5
 
 Ugh, those look like they should be debugfs interfaces.  Thanks, I'll
 add them to my list of things to nag people about...

Actually looking properly these are outside sysfs:

$ mount | grep selinux
selinuxfs on /sys/fs/selinux type selinuxfs (rw,relatime)

So everything below the mount point is in their own vfstype.

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] implementing tape statistics single file vs multi-file in sysfs

[Bryn M. Reeves]

On Sat, Feb 07, 2015 at 12:07:43PM +0800, Greg KH wrote:
 On Fri, Feb 06, 2015 at 03:41:58PM +, Bryn M. Reeves wrote:
  I can't speak for Shane but wouldn't spend too much time looking at the
  current v2 patch: it's the result of a pretty ugly compromise suggested
  on linux-scsi.
 
 Fair enough, but please feel free to cc: me on the patch that you do
 feel is correct to get a sysfs-related review.

Will do; I'm back from travels this week  will have some time to look at
this.
 
  Likewise for disk stats: although fluff like maj:min/name etc. has been
  shuffled a few times the basic fields have remained unchanged for a very
  long time and sysfs already removes the need to include an identity
  field.
 
 We already handle i/o stats just fine, why create a special sysfs
 interface for just a tape device interface?  What makes them so special?

But the iostats use exactly the sort of array file we're talking about:

$ cat /sys/block/sda/stat 
  12764420869  4320505  2305697   15404530056  3834036  9065092
0   931842 11371357

And we can't simply extend these to tapes as they are not block devices.
 
  I understand the fact that you can't change them; I just don't think it's
  a big problem in this specific case (and much less than some of the
  more imaginative sysfs content - 2d int arrays with column headers
  anyone?).
 
 What sysfs file is a 2d int array?  I'll be glad to fix it.

$ cat /sys/fs/selinux/avc/cache_stats 
lookups hits misses allocations reclaims frees
18938916 18921707 17209 17209 17328 22215
38164283 38146514 17769 17769 16800 19049
18078108 18056991 21117 21117 21344 19305
15168204 15150079 18125 18125 17776 13149
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0
0 0 0 0 0 0

$ cat /sys/fs/selinux/avc/hash_stats
entries: 506
buckets used: 290/512
longest chain: 5

 If you want to measure tens of thousands of tape devices then you
 shouldn't be using sysfs in the first place as it is not designed for
 speed at all.  Use the existing i/o rate interfaces instead, don't try
 to cram something into sysfs that doesn't belong there.

So far as I'm aware there is no other way to obtain performance data
for the SCSI tape subsystem (without resorting to ftrace/systemtap).

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND v5] st implement tape statistics

[Seymour, Shane M]

The following patch exposes statistics for the st driver via sysfs.
There is a need for companies with large numbers of tape drives
numbering in the tens of thousands to track the performance of those
tape drives (e.g. when a backup exceeds its window). The statistics
provided should allow the calculation of throughput, average block
sizes for read and write, and time spent waiting for I/O to complete
or doing tape movement.

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
---
- Fixed comment style issue, removed a second comment
- The tape driver will no longer claim a device if the stats
structure cannot be allocated.
- Show functions no longer need to protect against the
stats printer being a NULL pointer.
- Other code conditioned by a NULL pointer check for the
stats pointer has had the condition removed.
- Removed valid file from statistics directory in sysfs
no longer required.
- Change ABI descriptions of statistics file to remove
the reference to when they may only return 0. Removed
description of valid file from ABI definition.

--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-09 22:42:49.651988119 -0600
@@ -476,10 +476,25 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks;
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   /*
+* Irrespective of the I/O succeeding or not we count it. We don't
+* have bufflen any more so a read at end of file will over count
+* the blocks by one.
+*/
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +511,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +532,18 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-stats-read_cnt++;
+   STp-stats-read_byte_cnt += bufflen;
+   } else {
+   STp-stats-other_cnt++;
+   }
+   STp-stats-stamp = get_jiffies_64();
+   STp-stats-in_flight++;
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4222,6 +4250,12 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+   tpnt-stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_KERNEL);
+   if (tpnt-stats == NULL) {
+   sdev_printk(KERN_ERR, SDp,
+   st: Can't allocate statistics.\n);
+   goto out_idr_remove;
+   }
 
dev_set_drvdata(dev, tpnt);
 
@@ -4241,6 +4275,8 @@ static int st_probe(struct device *dev)
 
 out_remove_devs:
remove_cdevs(tpnt);
+   kfree(tpnt-stats);
+out_idr_remove:
spin_lock(st_index_lock);
idr_remove(st_index_idr, tpnt-index);
spin_unlock(st_index_lock);
@@ -4298,6 +4334,7 @@ static void scsi_tape_release(struct kre
 
disk-private_data = NULL;
put_disk(disk);
+   kfree(tpnt-stats);
kfree(tpnt);
return;
 }
@@ -4513,6 +4550,161 @@ options_show(struct device *dev, struct
 }
 static DEVICE_ATTR_RO(options);
 
+/* Support for tape stats */
+
+/**
+ * read_cnt_show - return read count - count of reads made from tape drive
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_cnt_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct st_modedef *STm = dev_get_drvdata(dev);
+
+   return sprintf(buf, %llu, STm-tape-stats-read_cnt);
+}
+static DEVICE_ATTR_RO(read_cnt);
+
+/**
+ * read_byte_cnt_show - return read byte count - tape drives
+ * may use blocks less than 512 bytes this gives the raw byte count of
+ * of data read from the tape drive.
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_byte_cnt_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+ 

RE: [PATCH] st implement tape statistics v3 (updated patch)

[Seymour, Shane M]

Hi Greg,

Thank you for pushing me to go that little further. The statistics directory is 
back. Any feedback from anyone would be appreciated.

Thanks
Shane
--

Signed-off-by: shane.seym...@hp.com
Tested-by: shane.seym...@hp.com
---
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-09 00:05:46.838967740 -0600
@@ -476,10 +476,25 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks;
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+/* Note that irrespective of the I/O succeeding or not we count it. We
+   don't have bufflen any more so a read at end of file will over count
+   the blocks by one. */
+   if (STp-stats != NULL) {
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+   }
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +511,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +532,20 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (STp-stats != NULL) {
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-stats-read_cnt++;
+   STp-stats-read_byte_cnt += bufflen;
+   } else {
+   STp-stats-other_cnt++;
+   }
+   STp-stats-stamp = get_jiffies_64();
+   STp-stats-in_flight++;
+   }
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4064,6 +4094,7 @@ out:
 static int create_cdevs(struct scsi_tape *tape)
 {
int mode, error;
+
for (mode = 0; mode  ST_NBR_MODES; ++mode) {
error = create_one_cdev(tape, mode, 0);
if (error)
@@ -4222,6 +4253,8 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+/* Allocate stats structure */
+   tpnt-stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_ATOMIC);
 
dev_set_drvdata(dev, tpnt);
 
@@ -4248,6 +4281,8 @@ out_put_queue:
blk_put_queue(disk-queue);
 out_put_disk:
put_disk(disk);
+   if (tpnt-stats != NULL)
+   kfree(tpnt-stats);
kfree(tpnt);
 out_buffer_free:
kfree(buffer);
@@ -4298,6 +4333,10 @@ static void scsi_tape_release(struct kre
 
disk-private_data = NULL;
put_disk(disk);
+   if (tpnt-stats != NULL) {
+   kfree(tpnt-stats);
+   tpnt-stats = NULL;
+   }
kfree(tpnt);
return;
 }
@@ -4513,6 +4552,223 @@ options_show(struct device *dev, struct
 }
 static DEVICE_ATTR_RO(options);
 
+/* Support for tape stats */
+
+/**
+ * read_cnt_how - return read count - count of reads made from tape drive
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_cnt_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct st_modedef *STm = dev_get_drvdata(dev);
+   struct scsi_tape *STp = STm-tape;
+   struct scsi_tape_stats *STt;
+
+   if ((STp == 0) || (STp-stats == 0))
+   return -EINVAL;
+   STt = STp-stats;
+   if (STt == 0)
+   return -EINVAL;
+   return snprintf(buf, PAGE_SIZE, %llu, STt-read_cnt);
+}
+static DEVICE_ATTR_RO(read_cnt);
+
+/**
+ * read_byte_cnt_show - return read byte count - tape drives
+ * may use blocks less than 512 bytes this gives the raw byte count of
+ * of data read from the tape drive.
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_byte_cnt_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct st_modedef *STm = dev_get_drvdata(dev);
+   struct scsi_tape *STp = STm-tape;
+   struct scsi_tape_stats *STt;
+
+   if ((STp == 0) || (STp-stats == 0))
+   return -EINVAL;
+   STt = STp-stats;
+   if (STt == 0)
+   return 

[PATCH RESEND] st implement tape statistics v3

[Seymour, Shane M]

The following patch exposes statistics for the st driver via sysfs. There is a 
need for companies with large numbers of tape drives numbering in the tens of 
thousands to track the performance of those tape drives (e.g. when a backup 
exceeds its window). The statistics provided should allow the calculation of 
throughput, average block sizes for read and write, and time spent waiting for 
I/O to complete or doing tape movement.

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
---
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-09 00:05:46.838967740 -0600
@@ -476,10 +476,25 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks;
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+/* Note that irrespective of the I/O succeeding or not we count it. We
+   don't have bufflen any more so a read at end of file will over count
+   the blocks by one. */
+   if (STp-stats != NULL) {
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+   }
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +511,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +532,20 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (STp-stats != NULL) {
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-stats-read_cnt++;
+   STp-stats-read_byte_cnt += bufflen;
+   } else {
+   STp-stats-other_cnt++;
+   }
+   STp-stats-stamp = get_jiffies_64();
+   STp-stats-in_flight++;
+   }
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4064,6 +4094,7 @@ out:
 static int create_cdevs(struct scsi_tape *tape)
 {
int mode, error;
+
for (mode = 0; mode  ST_NBR_MODES; ++mode) {
error = create_one_cdev(tape, mode, 0);
if (error)
@@ -4222,6 +4253,8 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+/* Allocate stats structure */
+   tpnt-stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_ATOMIC);
 
dev_set_drvdata(dev, tpnt);
 
@@ -4248,6 +4281,8 @@ out_put_queue:
blk_put_queue(disk-queue);
 out_put_disk:
put_disk(disk);
+   if (tpnt-stats != NULL)
+   kfree(tpnt-stats);
kfree(tpnt);
 out_buffer_free:
kfree(buffer);
@@ -4298,6 +4333,10 @@ static void scsi_tape_release(struct kre
 
disk-private_data = NULL;
put_disk(disk);
+   if (tpnt-stats != NULL) {
+   kfree(tpnt-stats);
+   tpnt-stats = NULL;
+   }
kfree(tpnt);
return;
 }
@@ -4513,6 +4552,223 @@ options_show(struct device *dev, struct
 }
 static DEVICE_ATTR_RO(options);
 
+/* Support for tape stats */
+
+/**
+ * read_cnt_how - return read count - count of reads made from tape drive
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_cnt_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct st_modedef *STm = dev_get_drvdata(dev);
+   struct scsi_tape *STp = STm-tape;
+   struct scsi_tape_stats *STt;
+
+   if ((STp == 0) || (STp-stats == 0))
+   return -EINVAL;
+   STt = STp-stats;
+   if (STt == 0)
+   return -EINVAL;
+   return snprintf(buf, PAGE_SIZE, %llu, STt-read_cnt);
+}
+static DEVICE_ATTR_RO(read_cnt);
+
+/**
+ * read_byte_cnt_show - return read byte count - tape drives
+ * may use blocks less than 512 bytes this gives the raw byte count of
+ * of data read from the tape drive.
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_byte_cnt_show(struct device *dev,
+   struct device_attribute 

RE: [PATCH RESEND] st implement tape statistics v3

[Seymour, Shane M]

 And you need to put below the --- line what has changed from the last
version, I don't see any of my comments address here :(

Doh! My appologies Greg, I'd missed your inline comments - I haven't had enough 
coffee this morning.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] st implement tape statistics v3

[Seymour, Shane M]

Hi All,

Please find attached v3 of the patch. It implements the changes requested by 
Greg. The statistics files aren't in a separate directory any more they're 
implemented directly as device attributes unless someone has objections to them 
being in a place like /sys/class/scsi_tape/*/.

There's also an ABI for file them in the patch for the testing directory. The 
kernel version is ?? because I'm not sure if there will be more changes 
based on feedback - let me know if I should drop that from the patch than 
submit a separate patch requesting the file be created once I know the kernel 
version it makes it into. Is the ABI file named correctly as 
sysfs-class-scsi_tape?

I've dropped the sync file stuff as well - As much as I'd like consistent 
statistics Greg is right it won't hurt if they're not quite in sync. The patch 
including the additional ABI documentation is about two thirds of the original 
size.

Thanks
Shane
--

Signed-off-by: shane.seym...@hp.com
Tested-by: shane.seym...@hp.com
---
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-02-08 16:50:51.368552780 -0600
@@ -476,10 +476,22 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks;
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   if (STp-stats != NULL) {
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+   }
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +508,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +529,20 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (STp-stats != NULL) {
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-stats-read_cnt++;
+   STp-stats-read_byte_cnt += bufflen;
+   } else {
+   STp-stats-other_cnt++;
+   }
+   STp-stats-stamp = get_jiffies_64();
+   STp-stats-in_flight++;
+   }
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4064,6 +4091,7 @@ out:
 static int create_cdevs(struct scsi_tape *tape)
 {
int mode, error;
+
for (mode = 0; mode  ST_NBR_MODES; ++mode) {
error = create_one_cdev(tape, mode, 0);
if (error)
@@ -4222,6 +4250,8 @@ static int st_probe(struct device *dev)
}
tpnt-index = error;
sprintf(disk-disk_name, st%d, tpnt-index);
+/* Allocate stats structure */
+   tpnt-stats = kzalloc(sizeof(struct scsi_tape_stats), GFP_ATOMIC);
 
dev_set_drvdata(dev, tpnt);
 
@@ -4248,6 +4278,8 @@ out_put_queue:
blk_put_queue(disk-queue);
 out_put_disk:
put_disk(disk);
+   if (tpnt-stats != NULL)
+   kfree(tpnt-stats);
kfree(tpnt);
 out_buffer_free:
kfree(buffer);
@@ -4298,6 +4330,10 @@ static void scsi_tape_release(struct kre
 
disk-private_data = NULL;
put_disk(disk);
+   if (tpnt-stats != NULL) {
+   kfree(tpnt-stats);
+   tpnt-stats = NULL;
+   }
kfree(tpnt);
return;
 }
@@ -4513,12 +4549,238 @@ options_show(struct device *dev, struct
 }
 static DEVICE_ATTR_RO(options);
 
+/* Support for tape stats */
+
+/**
+ * read_cnt_how - return read count - count of reads made from tape drive
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t read_cnt_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct st_modedef *STm = dev_get_drvdata(dev);
+   struct scsi_tape *STp = STm-tape;
+   struct scsi_tape_stats *STt;
+
+   if ((STp == 0) || (STp-stats == 0))
+   return -EINVAL;
+   STt = STp-stats;
+   if (STt == 0)
+   return -EINVAL;
+   return snprintf(buf, PAGE_SIZE, %llu, STt-read_cnt);
+}
+static DEVICE_ATTR_RO(read_cnt);
+
+/**
+ * read_byte_cnt_show - return read byte count - tape 

RE: [PATCH] st: implement sysfs based tape statistics v2

[Seymour, Shane M]

Kai - see last 3 paragraphs for question about if something is a bug or not.

BTW I had a look - I couldn't quickly find out if there was a way to tell if 
the medium has changed in a tape drive (there could be something device 
specific). At the device level there's a record of I/O errors:

[root@tapedrive device]# pwd
/sys/class/scsi_tape/st0/device
[root@tapedrive device]# cat ioerr_cnt
0x5

That doesn't distinguish between read or write or any other kind of error 
though - it doesn't even really have to mean an error since reading with a 
block size too small also increments the error count:

[root@tapedrive tape-stats]# ./tape_exercise write /dev/st0 /dev/urandom 100
About to write from /dev/urandom to /dev/st0, max size = 100, blksize = 4096
Write complete on /dev/st0 after 1003520 bytes
./tape_[root@tapedrive tape-stats]# ./tape_exercise read /dev/st0
About to read from /dev/st0 blksize = 256
Failed to read from /dev/st0 using current blksize, will try using a bigger 
blksize
About to read from /dev/st0 blksize = 512
Failed to read from /dev/st0 using current blksize, will try using a bigger 
blksize
About to read from /dev/st0 blksize = 1024
Failed to read from /dev/st0 using current blksize, will try using a bigger 
blksize
About to read from /dev/st0 blksize = 2048
Failed to read from /dev/st0 using current blksize, will try using a bigger 
blksize
About to read from /dev/st0 blksize = 4096
Reading complete for /dev/st0, 987136 bytes read

[root@tapedrive tape-stats]# cd /sys/class/scsi_tape/st0/device
[root@tapedrive device]# cat ioerr_cnt
0xa

It would seem that ioerr_cnt is probably a count of SCSI check conditions 
encountered?

Unfortunately for the MTIOCGET ioctl the value of mt_resid is the partition 
number of the tape not what I would have expected it to be - the residual left 
after the last read or write that returned an error (and 0 if the last 
read/write didn't return an error).

Kai - is that a bug? Shouldn't mt_resid be the residual from the last failed 
read or write indicating how much data didn't make it to the device and 0 on a 
successful read or write? I've used mt_resid from MTIOCGET on HP-UX previously 
when issuing reads and repositioning and retrying tape reads when starting with 
too low a block size to try and automatically determine the block size in use 
(it's never a good idea to under or overread tape blocks because it always 
results in check conditions and in the st driver under reading the block size 
always creates messages in dmesg).

If you don't have time to look at it I may look at if it's possible to provide 
the correct mt_resid for MTIOCGET - assuming that a long time if misuse 
prevents us from correcting it. If there's no way to export a partition number 
for the devices that support it I can add a new sysfs file (call it partition) 
to export it that way and see if I can get the correct value into mt_resid.

-Original Message-
From: Seymour, Shane M 
Sent: Monday, February 09, 2015 10:19 AM
To: 'Dale R. Worley'
Cc: linux-scsi@vger.kernel.org
Subject: RE: [PATCH] st: implement sysfs based tape statistics v2

Both of those things would have to be futures and require discussion - the very 
original version cleared stats on a device open but I got asked to keep it the 
stats cumulative so they would be more similar to disk stats. I'm not sure 
about the bad reads idea I'd have to look and see if some other layer provided 
device error information. I've got some changes to make and don't want to 
change it into a moving target that needs more discussion at this point.

-Original Message-
From: Dale R. Worley [mailto:wor...@alum.mit.edu] 
Sent: Monday, February 09, 2015 4:08 AM
To: Seymour, Shane M
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH] st: implement sysfs based tape statistics v2

One feature of tape statistics is that they're as much about the *tape* as they 
are about the *drive*, which is uncommon for block devices.  It might be useful 
to have a set of counters which is cleared every time a new tape is inserted 
into the drive.  In particular, bad reads since this tape was inserted would 
be very useful for monitoring the quality of tapes.

Dale
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] st: implement sysfs based tape statistics v2

[Seymour, Shane M]

Both of those things would have to be futures and require discussion - the very 
original version cleared stats on a device open but I got asked to keep it the 
stats cumulative so they would be more similar to disk stats. I'm not sure 
about the bad reads idea I'd have to look and see if some other layer provided 
device error information. I've got some changes to make and don't want to 
change it into a moving target that needs more discussion at this point.

-Original Message-
From: Dale R. Worley [mailto:wor...@alum.mit.edu] 
Sent: Monday, February 09, 2015 4:08 AM
To: Seymour, Shane M
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH] st: implement sysfs based tape statistics v2

One feature of tape statistics is that they're as much about the *tape* as they 
are about the *drive*, which is uncommon for block devices.  It might be useful 
to have a set of counters which is cleared every time a new tape is inserted 
into the drive.  In particular, bad reads since this tape was inserted would 
be very useful for monitoring the quality of tapes.

Dale
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] implementing tape statistics single file vs multi-file in sysfs

[Bryn M. Reeves]

On Fri, Feb 06, 2015 at 12:20:53AM +, Seymour, Shane M wrote:
 There has been some ongoing discussion about the best way to implement
 tape statistics. The original method suggested a long time ago used a
 single file in sysfs similar to block statistics in sysfs. That lead to
 an impass about the code on the linux-scsi mailing list.

I would have a strong preference for a single file containing an array
of integer counters. This is in keeping with other statistics attributes
in sysfs and follows the principle of least surprise: it's essentially
the same general format as /proc/diskstats and sysfs disk and partition
stats (dm statistics also follow this convention via the @print_stats
message).

This simplifies userspace code to read and parse the counters and avoids
additional sample jitter when reading stats for very large numbers of
devices; each device would require at least eleven open()/read()/close()
cycles.

For a small number of devices this shouldn't matter too much but
eventually the additional syscall overhead could become significant (I
think you mentioned users with ~20k devices?).

The sync file mechanism in the v2 patch that addresses this problem is
kinda cute but also significantly more complex than a plain old array
and as you pointed out adds hundreds of lines to the patch..

Sticking to arrays also allows existing tools like sysstat to be easily
adapted to the new data source.

 The sysfs documentation says that files should contain one item per file 
 (with some small exceptions):
 
  Attributes should be ASCII text files, preferably with only one value
  per file. It is noted that it may not be efficient to contain only one
  value per file, so it is socially acceptable to express an array of
  values of the same type.

Right: I think there's good precedent for the array file style when
dealing with counter sets.

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] implementing tape statistics single file vs multi-file in sysfs

[Bryn M. Reeves]

On Fri, Feb 06, 2015 at 04:59:16AM -0800, Greg KH wrote:
 On Fri, Feb 06, 2015 at 12:20:53AM +, Seymour, Shane M wrote:
  The current patch that implements tape statistics is here:
  
  http://marc.info/?l=linux-scsim=142112067313723w=2
 
 Aside from the do we want to do this all in a single file issue that I
 will say more on below, this patch has issues.  Please don't use a
 kobject for _ANYTHING_ in sysfs that has a struct device as a parent.
 If you do that, it can't be seen by userspace tools very well, if at
 all.

I can't speak for Shane but wouldn't spend too much time looking at the
current v2 patch: it's the result of a pretty ugly compromise suggested
on linux-scsi.

This thread was really to try to settle the discussion on the structure
of the stats files.

  Recently there was was another discussion here about one file vs a 
  collection of files for tape statistics:
  
  http://marc.info/?l=linux-scsim=142316255501550w=2
  
  The result was that I should ask here what method I should use. I
  would like to get feedback in relation to tape statistics and one file
  vs multi-file in sysfs. I'm happy to keep the existing code or change
  to a single file approach.
 
 One of the primary reasons we created sysfs and the one value per file
 rule is that multi-value files just do not work well.  Yes, you get an
 atomic snapshot, and you save some open/read/close syscall roundtrips,
 but you do so at the expense of forcing userspace to know what the
 format of the file is.  And once you create it, you can NEVER CHANGE IT
 AGAIN.

I am not convinced this is a concern for tape statistics: they are pretty
much a solved problem. The commercial *nixes have had this for decades.

Likewise for disk stats: although fluff like maj:min/name etc. has been
shuffled a few times the basic fields have remained unchanged for a very
long time and sysfs already removes the need to include an identity
field.
 
 Yes, that's right, if you come up with some new statistic in the future,
 or realize that one of the ones you have now is wrong, you can't change
 it, you have to make a whole new file, otherwise you could break
 userspace tools.

I understand the fact that you can't change them; I just don't think it's
a big problem in this specific case (and much less than some of the
more imaginative sysfs content - 2d int arrays with column headers
anyone?).

 And yes, open/read/close does take take a few extra cycles, but you
 can't really measure it for a virtual filesystem like this on any modern
 system.

I'll try to get some numbers when I get back home next week - Shane is
talking about use cases involving tens of thousands of tape devices. I
am not certain that the overhead would be unmeasurable in that case: the
additional context switching  TLB flushes alone seem like they would
add up.

 Hope this helps explain why we have the sysfs rule, and why you should
 continue to follow it as well.

 Yes, it's not always followed, but that's usually because people forgot
 why we had this rule, and no one noticed or pointed it out to me that it
 was wrong.

Perhaps sysfs.txt should be updated to make the position more clear? The
current wording seems rather more liberal than this thread would
suggest. Maybe something like the patch below?

This would help people who are trying to dtrt by reading the documentation.

Regards,
Bryn.


  From 3081aad4cc4d19b68f39499dbeb3837f0642f70e Mon Sep 17 00:00:00 2001
  From: Bryn M. Reeves b...@redhat.com
  Date: Fri, 6 Feb 2015 15:19:39 +
  Subject: [PATCH] docs/sysfs: Specify array valued attribute review
   requirements
  
  Although the linux-api position that one-value-per-file is a strong rule
  is very clear in mailing list discussions the sysfs.txt documentation
  suggests a rather more liberal stance:
  
  ... it is socially acceptable to express an array of values of the same
  type.
  
  Fix the documentation to make it clear that such uses should be
  discussed on linux-api first.

Signed-off-by: Bryn M. Reeves b...@redhat.com
---
 Documentation/filesystems/sysfs.txt | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/sysfs.txt 
b/Documentation/filesystems/sysfs.txt
index b35a64b..494fa78 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -57,8 +57,15 @@ attributes.
 
 Attributes should be ASCII text files, preferably with only one value
 per file. It is noted that it may not be efficient to contain only one
-value per file, so it is socially acceptable to express an array of
-values of the same type. 
+value per file, so it may be socially acceptable to express an array of
+values of the same type.
+
+If you are considering adding such an array attribute to sysfs please
+discuss it via the linux-api mailing list first to ensure that your
+proposed use is acceptable:
+
+  https://www.kernel.org/doc/man-pages/linux-api-ml.html
+  linux-...@vger.kernel.org
 
 Mixing types

[RFC] implementing tape statistics single file vs multi-file in sysfs

[Seymour, Shane M]

Hello linux-api'ers

There has been some ongoing discussion about the best way to implement tape 
statistics. The original method suggested a long time ago used a single file in 
sysfs similar to block statistics in sysfs. That lead to an impass about the 
code on the linux-scsi mailing list.

The sysfs documentation says that files should contain one item per file (with 
some small exceptions):

 Attributes should be ASCII text files, preferably with only one value
 per file. It is noted that it may not be efficient to contain only one
 value per file, so it is socially acceptable to express an array of
 values of the same type.

The current patch that implements tape statistics is here:

http://marc.info/?l=linux-scsim=142112067313723w=2

Recently there was was another discussion here about one file vs a collection 
of files for tape statistics:

http://marc.info/?l=linux-scsim=142316255501550w=2

The result was that I should ask here what method I should use. I would like to 
get feedback in relation to tape statistics and one file vs multi-file in 
sysfs. I'm happy to keep the existing code or change to a single file approach.

Thanks
Shane
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] st: implement sysfs based tape statistics v2

[Bryn M. Reeves]

On Thu, Feb 05, 2015 at 07:46:32PM +0200, Kai Mäkisara (Kolumbus) wrote:
  On 5.2.2015, at 19.40, Laurence Oberman lober...@redhat.com wrote:
  From: Kai Mäkisara (Kolumbus) kai.makis...@kolumbus.fi
  I still think that the tape statistics should be exported like the 
  statistics of “real” block devices, i.e., one sysfs file exporting on a 
  single line the statistics that temporally belong together. James rejected 
  this approach. I am leaving the decision about this code to him. I will 
  neither ack nor nak this code.
  
  I missed the earlier conversations with James, I will go search for them.
  Do you mean add them so they are similar to the /proc/diskstats

http://comments.gmane.org/gmane.linux.scsi/80497

On Fri, Feb 22 2013 James Bottomley wrote:
 I'm afraid we can't do it the way you're proposing.  files in sysfs must
 conform to the one value per file rule (so we avoid the ABI nastiness
 that plagues /proc).  You can create a stat directory with a bunch of
 files, but not a single file that gives all values.

Documentation/filesystems/sysfs.txt does not agree:

  Attributes should be ASCII text files, preferably with only one value
  per file. It is noted that it may not be efficient to contain only one
  value per file, so it is socially acceptable to express an array of
  values of the same type.

There's also ample precedent for this: sysfs disk and partition stats,
SELinux cache and hash table stats (which have a pretty yucky 2d int
array with column headers and a name: val format respectively).

There's also a bunch of multivariate name=value format stats files in
the cgroups sysfs tree.

 Not exactly. I mean the data exported in sysfs, for example:
 
  cat /sys/block/sda/sda1/stat
   159740 9006  594150664461   12472455907 12772208  3598677   
  0   299875  3663235

I'd prefer to consume tape stats in this format too; it follows the
principle of least surprise since it's shared with every other IO stats
source (including device-mapper statistics) and it simplifies handling
the counters in user space.

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] st: implement sysfs based tape statistics v2

[Bryn M. Reeves]

On Thu, Feb 05, 2015 at 10:55:50AM -0800, James Bottomley wrote:
 OK, the sysfs bikeshedders hang out on linux-api
 
 https://www.kernel.org/doc/man-pages/linux-api-ml.html
 
 If you can convince them, we'll do the single file approach.

Will do - I've got a couple of stats projects on the go at the moment so
this ties in well with that.

I'll sync up with Shane and see if he's interested in running the int array
version via the linux-api folks.

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] st: implement sysfs based tape statistics v2

[Seymour, Shane M]

I was wondering if anyone had any feedback or had any chance to review the 
changes?

-Original Message-
From: linux-scsi-ow...@vger.kernel.org 
[mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Seymour, Shane M
Sent: Tuesday, January 13, 2015 2:43 PM
To: linux-scsi@vger.kernel.org
Cc: kai.makis...@kolumbus.fi; James E.J. Bottomley (jbottom...@parallels.com); 
je...@suse.com
Subject: [PATCH] st: implement sysfs based tape statistics v2

Some small changes since the last version - this version removes two files from 
sysfs compared to the last version (read and write block counts since they're 
derived from the byte counts they can be calculated in user space) but that's 
the only change. This version has been rebased to 3.19.0-rc3-next-20150108.

Since posting the last version an email was received by Kai and myself from an 
ATT employee who has a need for tape statistics to be implemented (who gave 
permission to quote their email), I've included part of the email here:

There are over 20,000 tape devices managed by our operations group zoned to 
tens of thousands of servers.

My challenge is that I cannot provide operations a solution that gives them 
visibility into the tape drive performance metrics when that platform is linux. 
Our legacy platforms (Solaris,HPUX,AIX) provide facilities to use iostat and 
sar to determine the write speed of the tape drives. We took for granted that 
this would be available in linux and its absence has been very troublesome. 
Because operations cannot measure tape drive performance in this way they 
cannot easily determine when there is a tape drive performance problem and 
whether the change improved or worsened the performance problem.
...
I have followed the debate https://lkml.org/lkml/2013/3/20/696 and from a 
service provide perspective we would expect the same maturity and functionality 
that we have from the traditional unix platform in regards to iostat/sar. This 
issue is important and urgent because tape drive performance issues are common 
and I am unable to provide standards and processes to identify and remediate 
these issues.

Another HP customer has also requested the same functionality (but hasn't given 
permission to be named), they own tape drives numbering in the 1000s and also 
need the ability to investigate performance issues.

Signed-off-by: shane.seym...@hp.com
Tested-by: shane.seym...@hp.com
---
diff -uprN a/drivers/scsi/st.c b/drivers/scsi/st.c
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-01-12 13:54:52.549117333 -0600
@@ -20,6 +20,7 @@
 static const char *verstr = 20101219;
 
 #include linux/module.h
+#include linux/kobject.h
 
 #include linux/fs.h
 #include linux/kernel.h
@@ -226,6 +227,20 @@ static DEFINE_SPINLOCK(st_index_lock);
 static DEFINE_SPINLOCK(st_use_lock);
 static DEFINE_IDR(st_index_idr);
 
+static inline void st_stats_remove_files(struct scsi_tape *);
+static inline void st_stats_create_files(struct scsi_tape *);
+static ssize_t st_tape_attr_show(struct kobject *, struct attribute *, char *);
+static ssize_t st_tape_attr_store(struct kobject *, struct attribute *,
+   const char *, size_t);
+static void st_release_stats_kobj(struct kobject *);
+static const struct sysfs_ops st_stats_sysfs_ops = {
+   .show   = st_tape_attr_show,
+   .store  = st_tape_attr_store,
+};
+static struct kobj_type st_stats_ktype = {
+   .release = st_release_stats_kobj,
+   .sysfs_ops = st_stats_sysfs_ops,
+};
 
 

 #include osst_detect.h
@@ -476,10 +491,22 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks;
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   if (STp-stats != NULL) {
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+   }
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +523,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +544,20 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (STp-stats != NULL) {
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt

[PATCH] st: implement sysfs based tape statistics v2

[Seymour, Shane M]

Some small changes since the last version - this version removes two files from 
sysfs compared to the last version (read and write block counts since they're 
derived from the byte counts they can be calculated in user space) but that's 
the only change. This version has been rebased to 3.19.0-rc3-next-20150108.

Since posting the last version an email was received by Kai and myself from an 
ATT employee who has a need for tape statistics to be implemented (who gave 
permission to quote their email), I've included part of the email here:

There are over 20,000 tape devices managed by our operations group zoned to 
tens of thousands of servers.

My challenge is that I cannot provide operations a solution that gives them 
visibility into the tape drive performance metrics when that platform is linux. 
Our legacy platforms (Solaris,HPUX,AIX) provide facilities to use iostat and 
sar to determine the write speed of the tape drives. We took for granted that 
this would be available in linux and its absence has been very troublesome. 
Because operations cannot measure tape drive performance in this way they 
cannot easily determine when there is a tape drive performance problem and 
whether the change improved or worsened the performance problem.
...
I have followed the debate https://lkml.org/lkml/2013/3/20/696 and from a 
service provide perspective we would expect the same maturity and functionality 
that we have from the traditional unix platform in regards to iostat/sar. This 
issue is important and urgent because tape drive performance issues are common 
and I am unable to provide standards and processes to identify and remediate 
these issues.

Another HP customer has also requested the same functionality (but hasn't given 
permission to be named), they own tape drives numbering in the 1000s and also 
need the ability to investigate performance issues.

Signed-off-by: shane.seym...@hp.com
Tested-by: shane.seym...@hp.com
---
diff -uprN a/drivers/scsi/st.c b/drivers/scsi/st.c
--- a/drivers/scsi/st.c 2015-01-11 14:46:00.243814755 -0600
+++ b/drivers/scsi/st.c 2015-01-12 13:54:52.549117333 -0600
@@ -20,6 +20,7 @@
 static const char *verstr = 20101219;
 
 #include linux/module.h
+#include linux/kobject.h
 
 #include linux/fs.h
 #include linux/kernel.h
@@ -226,6 +227,20 @@ static DEFINE_SPINLOCK(st_index_lock);
 static DEFINE_SPINLOCK(st_use_lock);
 static DEFINE_IDR(st_index_idr);
 
+static inline void st_stats_remove_files(struct scsi_tape *);
+static inline void st_stats_create_files(struct scsi_tape *);
+static ssize_t st_tape_attr_show(struct kobject *, struct attribute *, char *);
+static ssize_t st_tape_attr_store(struct kobject *, struct attribute *,
+   const char *, size_t);
+static void st_release_stats_kobj(struct kobject *);
+static const struct sysfs_ops st_stats_sysfs_ops = {
+   .show   = st_tape_attr_show,
+   .store  = st_tape_attr_store,
+};
+static struct kobj_type st_stats_ktype = {
+   .release = st_release_stats_kobj,
+   .sysfs_ops = st_stats_sysfs_ops,
+};
 
 

 #include osst_detect.h
@@ -476,10 +491,22 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks;
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   if (STp-stats != NULL) {
+   ticks = get_jiffies_64();
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+   }
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +523,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +544,20 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (STp-stats != NULL) {
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-stats-read_cnt++;
+   STp-stats-read_byte_cnt += bufflen;
+   } else {
+   STp-stats-other_cnt++;
+   }
+   STp-stats-stamp = get_jiffies_64();
+   STp-stats-in_flight++;
+   }
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, 

[RFC] Deprecate, modify, or do nothing to SCSI_IOCTL_GET_IDLUN

[Seymour, Shane M]

I'd like to ask if SCSI_IOCTL_GET_IDLUN should be deprecated? This is in 
response to [Bug 88591] SCSI_IOCTL_GET_IDLUN only returns 8 bits for the SCSI 
Target value of which has been seen on the mailing list.

It only returns one byte of id, lun, channel, and host number but we have 
SG_GET_SCSI_ID which fills a structure with a separate int for each of those 4 
values.

There's two choices in terms of deprecation. The first is immediately 
deprecating it by moving it in scsi_ioctl() the other deprecated ioctls so it 
no longer works ad issues a warning or the second keep it working for the time 
being and add the following before it returns 0 from that function:

if (printk_ratelimit())
printk (KERN_WARNING program %s is using a deprecated SCSI 
ioctl SCSI_IOCTL_GET_IDLUN, please convert it to use 
SG_GET_SCSI_ID \n,
 current-comm);

After some suitable period of time move it to the other deprecated ioctls 
scsi_ioctl() where it will stop working and print the same deprecation message 
as other deprecated ioctls. 

Alternative 3 is to look at the values being compacted into the 4 bytes and if 
any of id, lun, channel or host overflow their single byte compacted value 
return -EOVERFLOW if any do and then provide a hint to the caller that they 
should be using SG_GET_SCSI_ID instead. That would instead change the return 0 
in scsi_ioctl() for SCSI_IOCTL_GET_IDLUN to be something like:

if ((sdev-id  0xff) || (sdev-lun  0xff) || (sdev-channel  
0xff)
|| (sdev-host-host_no  0xff)) {
printk(KERN_WARNING program %s - overflow in ioctl 
SCSI_IOCTL_GET_IDLUN, 
please convert it to use SG_GET_SCSI_ID\n, 
current-comm);
return -EOVERFLOW;
} else {
return 0;
}

It allows backwards compatibility (it will still overflow in exactly the same 
way) and for a lot of systems it will still work for most uses but provides an 
indication that something has gone wrong and the data returned shouldn't be 
relied upon to be valid when any of the information has overflowed what it's 
been stored in. 

Alternative 4 is do nothing people should understand the limitations of what 
ioctls they use (they have the source) and TLDP documents SG_GET_SCSI_ID so 
anyone using SCSI_IOCTL_GET_IDLUN should be aware of the fact that there are 
alternative SG ioctls with wider values for id, lun, channel, and host number.

There may be other possibilities that I've overlooked. After some consensus 
about what should be done I'm happy to put together a patch for review.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] st: implement sysfs based tape statistics

[Seymour, Shane M]

I was wondering if anyone had a chance to review the patch? Comments are 
appreciated and I'm more than happy to make changes that will allow it to be 
accepted.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] st: set owner in struct device_driver

[Seymour, Shane M]

I can, but at this point it will be tomorrow (11pm where I am).
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] st: implement sysfs based tape statistics

[Seymour, Shane M]

It's been a year since my last attempt at doing this as I got distracted by 
some other things. Comments are appreciated and any questions will be answered.

The following implements sysfs based per device tape statistics files with one 
file per statistic and a method of trying to allow a consistent set of 
statistics to be gathered. Included this time (see very end) is also 
documentation about the changes. These patches should apply on top of 
yesterdays changes to update the struct device_driver owner field. Tested with 
kernel version 3.18.0-rc4-next-20141112+.
---
Signed-off-by: Shane Seymour shane.seym...@hp.com
--- a/drivers/scsi/st.c 2014-11-12 11:31:54.416289214 -0600
+++ b/drivers/scsi/st.c 2014-11-12 11:44:24.243238146 -0600
@@ -20,6 +20,7 @@
 static const char *verstr = 20101219;
 
 #include linux/module.h
+#include linux/kobject.h
 
 #include linux/fs.h
 #include linux/kernel.h
@@ -226,6 +227,20 @@ static DEFINE_SPINLOCK(st_index_lock);
 static DEFINE_SPINLOCK(st_use_lock);
 static DEFINE_IDR(st_index_idr);
 
+static inline void st_stats_remove_files(struct scsi_tape *);
+static inline void st_stats_create_files(struct scsi_tape *);
+static ssize_t st_tape_attr_show(struct kobject *, struct attribute *, char *);
+static ssize_t st_tape_attr_store(struct kobject *, struct attribute *,
+   const char *, size_t);
+static void st_release_stats_kobj(struct kobject *);
+static const struct sysfs_ops st_stats_sysfs_ops = {
+   .show   = st_tape_attr_show,
+   .store  = st_tape_attr_store,
+};
+static struct kobj_type st_stats_ktype = {
+   .release = st_release_stats_kobj,
+   .sysfs_ops = st_stats_sysfs_ops,
+};
 
 

 #include osst_detect.h
@@ -476,10 +491,21 @@ static void st_scsi_execute_end(struct r
struct st_request *SRpnt = req-end_io_data;
struct scsi_tape *STp = SRpnt-stp;
struct bio *tmp;
+   u64 ticks = get_jiffies_64();
 
STp-buffer-cmdstat.midlevel_result = SRpnt-result = req-errors;
STp-buffer-cmdstat.residual = req-resid_len;
 
+   if (STp-stats != NULL) {
+   STp-stats-in_flight--;
+   ticks -= STp-stats-stamp;
+   STp-stats-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-stats-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-stats-read_ticks += ticks;
+   }
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -496,6 +522,7 @@ static int st_scsi_execute(struct st_req
struct rq_map_data *mdata = SRpnt-stp-buffer-map_data;
int err = 0;
int write = (data_direction == DMA_TO_DEVICE);
+   struct scsi_tape *STp = SRpnt-stp;
 
req = blk_get_request(SRpnt-stp-device-request_queue, write,
  GFP_KERNEL);
@@ -516,6 +543,20 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (STp-stats != NULL) {
+   if (cmd[0] == WRITE_6) {
+   STp-stats-write_cnt++;
+   STp-stats-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-stats-read_cnt++;
+   STp-stats-read_byte_cnt += bufflen;
+   } else {
+   STp-stats-other_cnt++;
+   }
+   STp-stats-stamp = get_jiffies_64();
+   STp-stats-in_flight++;
+   }
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4064,6 +4105,8 @@ out:
 static int create_cdevs(struct scsi_tape *tape)
 {
int mode, error;
+   struct scsi_tape_stats *tmp;
+
for (mode = 0; mode  ST_NBR_MODES; ++mode) {
error = create_one_cdev(tape, mode, 0);
if (error)
@@ -4072,6 +4115,26 @@ static int create_cdevs(struct scsi_tape
if (error)
return error;
}
+/* Create statistics directory under device, if it fails we dont
+   have statistics. */
+   if (tape-stats != NULL) {
+   kobject_init(tape-stats-statistics, st_stats_ktype);
+   error = kobject_add(tape-stats-statistics,
+   tape-device-sdev_gendev.kobj,
+   statistics);
+   if (error) {
+   kobject_del(tape-stats-statistics);
+   tmp = tape-stats;
+   tape-stats = NULL;
+   kfree(tmp);
+   } else {
+   st_stats_create_files(tape);
+   }
+   } else {
+   tmp = tape-stats;
+   tape-stats = NULL;
+   kfree(tmp);
+   }
 
return sysfs_create_link(tape-device-sdev_gendev.kobj,
 tape-modes[0].devs[0]-kobj, tape);
@@ -4081,6 +4144,10 @@ static void remove_cdevs(struct scsi_tap
 {
 

[PATCH] st: set owner in struct device_driver

[Seymour, Shane M]

After moving from from branch next-20141106 to next-2014 to pick up recent 
changes to the st driver I found that the following message was being logged by 
the kernel (for many other modules as well):

Driver 'st' needs an owner

There was a change in driver_register to check the struct module *owner and if 
it's not set complain about it, the code path for the st driver is:

static int __init init_st(void)
{
...
err = scsi_register_driver(st_template.gendrv);

Which calls:

int scsi_register_driver(struct device_driver *drv)
{
drv-bus = scsi_bus_type;

return driver_register(drv);
}
EXPORT_SYMBOL(scsi_register_driver);

Which calls:

int driver_register(struct device_driver *drv)
{
int ret;
struct device_driver *other;

BUG_ON(!drv-bus-p);

if (!drv-owner)
printk(KERN_WARNING Driver '%s' needs an owner, drv-name);
...

This patch sets the owner field in the struct device_driver contained in the 
struct scsi_driver for this module. Tested with kernel version 
3.18.0-rc4-next-2014. My assumption here is that the check added in 
driver_register() is correct and that forces this change and there's a lot of 
other modules that require a similar change (at least 72 including sd, sr, and 
osst).

Signed-off-by: Shane Seymour shane.seym...@hp.com
---
diff -up a/drivers/scsi/st.c b/drivers/scsi/st.c
--- a/drivers/scsi/st.c 2014-11-10 21:23:27.088567337 -0600
+++ b/drivers/scsi/st.c 2014-11-11 14:07:37.312721375 -0600
@@ -207,6 +207,7 @@ static struct scsi_driver st_template =
.name   = st,
.probe  = st_probe,
.remove = st_remove,
+   .owner  = THIS_MODULE,
},
 };
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: dummy scsi read or scsi command periodically for external USB Hard Disk

[Bryn M. Reeves]

On Sun, Jul 06, 2014 at 01:18:03AM +0800, loody wrote:
 hi all:
 we met a USB Hard Disk that will go to suspend if host stop
 sending scsi command over 5mins.
 To save the IO, kernel will keep the file in page cache as much as
 he can and under this circumstances, the scsi command may disappear
 for a while longer enough to cause the device suspend.
 
 is there any kernel config or module parameter can do the dummy
 read or scsi command periodically?

No but you could set up a simple cron job to call an sg3_utils command.

E.g. issue an sg_read for one sector to the device every 4m:

  */4 * * * * sg_read count=1 if=/dev/disk

You'll probably want to disable mail notification for the job or have
it dropped or it'll get a bit noisy running that frequently.

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: dummy scsi read or scsi command periodically for external USB Hard Disk

[Bryn M. Reeves]

On Mon, Jul 07, 2014 at 11:39:05PM +0800, loody wrote:
 hi David:
 
 2014-07-07 23:06 GMT+08:00 David Laight david.lai...@aculab.com:
  From: Lars Melin
  ...
  sgread is not included in BusyBox but you should have touch.
  Create a dummy file on the disk and let cron touch it every 4 minutes.
 
  You don't need 'touch' a shell redirect eg : file will do open(..., 
  O_CREAT|O_TRUNC).
  However that still might not force an actual disc access.
 
  In any case you really only want to do a read, doing a write will kill the 
  NAND memory.
 
 actually I have searched the scsi/usb layer for possible dummy read,
 even read sector 0 is fine, but in vain.
 
 I found the read
 a. determined by VFS - block layer,
 b. Block layer put it in queue
 c. call scsi pre-queue function to usb layer.
 
 That mean if I try to read sector from usb devices, I have to create a
 queue and follow above b) and c) rule.
 is there any already kernel API I can use?
 
 sincerely appreciate all yours help,

If you don't want to put sg_read into your image you could just use a dd;
busybox includes an implementation that should be good enough.

Just make sure you use the right flags to use O_DIRECT access or you'll
just end up hammering on the page cache. Iirc that's iflags=direct (check
the busybox docs to make sure it's the same).

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: dummy scsi read or scsi command periodically for external USB Hard Disk

[Bryn M. Reeves]

On Tue, Jul 08, 2014 at 12:15:54AM +0800, loody wrote:
 so sg_read will not hammer on the page cache like dd without iflags=direct
 
 thanks for your kind help,

The sg_read program (and other programs in sg3_utils) sends a command directly
to the device using an SG_IO ioctl. This bypasses all the caching layers in
the kernel and always results in IO to the device (if it succeeds).

Regards,
Bryn.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] scsi: break from queue depth adjusting loops when device found

[Stephen M. Cameron]

From: Stephen M. Cameron scame...@beardog.cce.hp.com

Don't loop through all the devices even after
finding the one we're looking for

Signed-off-by: Stephen M. Cameron scame...@beardog.cce.hp.com
Reviewed-by: Robert Elliott elli...@hp.com
---
 drivers/scsi/scsi_error.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cbe38e5..db8a488 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -635,6 +635,7 @@ static void scsi_handle_queue_ramp_up(struct scsi_device 
*sdev)
sht-change_queue_depth(tmp_sdev, tmp_sdev-queue_depth + 1,
SCSI_QDEPTH_RAMP_UP);
sdev-last_queue_ramp_up = jiffies;
+   break;
}
 }
 
@@ -657,6 +658,7 @@ static void scsi_handle_queue_full(struct scsi_device *sdev)
 */
sht-change_queue_depth(tmp_sdev, tmp_sdev-queue_depth - 1,
SCSI_QDEPTH_QFULL);
+   break;
}
 }
 

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] hpsa a few small updates for early July 2014

[Stephen M. Cameron]

Nothing very big here, just a few small updates for now.

I still have a giant ball of patches waiting in the wings, but it
is unfortunately not quite ready yet.

---

Robert Elliott (1):
  hpsa: do not unconditionally copy sense data

Stephen M. Cameron (4):
  hpsa: remove online devices from offline device list
  hpsa: fix bad -ENOMEM return value in hpsa_big_passthru_ioctl
  hpsa: make hpsa_init_one return -ENOMEM if allocation of 
h-lockup_detected fails
  hpsa: fix 6-byte READ/WRITE with 0 length data xfer


 drivers/scsi/hpsa.c |   31 +++
 1 files changed, 19 insertions(+), 12 deletions(-)

-- 
-- steve

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] hpsa: remove online devices from offline device list

[Stephen M. Cameron]

From: Stephen M. Cameron scame...@beardog.cce.hp.com

When devices come on line, they should be removed from the list of
offline devices that are monitored.

Signed-off-by: Stephen M. Cameron scame...@beardog.cce.hp.com
Reviewed-by: Scott Teel scott.t...@hp.com
Reviewed-by: Joe Handzik joseph.t.hand...@hp.com
---
 drivers/scsi/hpsa.c |6 +-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 31184b3..8cd1a9b 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6913,8 +6913,12 @@ static int hpsa_offline_devices_ready(struct ctlr_info 
*h)
d = list_entry(this, struct offline_device_entry,
offline_list);
spin_unlock_irqrestore(h-offline_device_lock, flags);
-   if (!hpsa_volume_offline(h, d-scsi3addr))
+   if (!hpsa_volume_offline(h, d-scsi3addr)) {
+   spin_lock_irqsave(h-offline_device_lock, flags);
+   list_del(d-offline_list);
+   spin_unlock_irqrestore(h-offline_device_lock, flags);
return 1;
+   }
spin_lock_irqsave(h-offline_device_lock, flags);
}
spin_unlock_irqrestore(h-offline_device_lock, flags);

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] hpsa: fix bad -ENOMEM return value in hpsa_big_passthru_ioctl

[Stephen M. Cameron]

From: Stephen M. Cameron scame...@beardog.cce.hp.com

When copy_from_user fails, return -EFAULT, not -ENOMEM

Signed-off-by: Stephen M. Cameron scame...@beardog.cce.hp.com
Reported-by: Robert Elliott elli...@hp.com
Reviewed-by: Joe Handzik joseph.t.hand...@hp.com
Reviewed-by: Scott Teel scott.t...@hp.com
Cc: sta...@vger.kernel.org
---
 drivers/scsi/hpsa.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 8cd1a9b..08b34e9 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5092,7 +5092,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, 
void __user *argp)
}
if (ioc-Request.Type.Direction  XFER_WRITE) {
if (copy_from_user(buff[sg_used], data_ptr, sz)) {
-   status = -ENOMEM;
+   status = -EFAULT;
goto cleanup1;
}
} else

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] hpsa: fix 6-byte READ/WRITE with 0 length data xfer

[Stephen M. Cameron]

From: Stephen M. Cameron scame...@beardog.cce.hp.com

a 6-byte READ/WRITE CDB with a 0 block data transfer really
means a 256 block data transfer.  The RAID mapping code failed
to handle this case.  For 10/12/16 byte READ/WRITEs, 0 just means
no data should be transferred, and should not trigger BUG_ON.

Signed-off-by: Stephen M. Cameron scame...@beardog.cce.hp.com
Reported-by: Robert Elliott elli...@hp.com
Reviewed-by: Robert Elliott elli...@hp.com
---
 drivers/scsi/hpsa.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 794d726..a97a7ff 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3686,6 +3686,8 @@ static int hpsa_scsi_ioaccel_raid_map(struct ctlr_info *h,
(((u64) cmd-cmnd[2])  8) |
cmd-cmnd[3];
block_cnt = cmd-cmnd[4];
+   if (block_cnt == 0)
+   block_cnt = 256;
break;
case WRITE_10:
is_write = 1;
@@ -3734,7 +3736,6 @@ static int hpsa_scsi_ioaccel_raid_map(struct ctlr_info *h,
default:
return IO_ACCEL_INELIGIBLE; /* process via normal I/O path */
}
-   BUG_ON(block_cnt == 0);
last_block = first_block + block_cnt - 1;
 
/* check for write to non-RAID-0 */

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html