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 
> Acked-by: Hannes Reinecke 
> Cc: Christoph Hellwig 
> Cc: Johannes Thumshirn 
> Cc: Shane M Seymour 
> ---
>  drivers/scsi/scsi.c | 144 --
> --
>  1 file changed, 66 insertions(+), 78 deletions(-)

Reviewed-by: Shane Seymour 

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.
> 

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]

Hi Kai,

I've done more tested. Some stuff didn't work and I've got some suggested 
changes (there are two changes to the patch and one for the mt command). 
Testing results first:

# echo 1 > /sys/bus/scsi/drivers/st/debug_flag
# mt -f /dev/st2 stsetoption can-partitions
# mt -f /dev/st1 stsetoption can-partitions
# mt -f /dev/st0 stsetoption can-partitions

Expect to fail LTO3:

# mt -f /dev/st0 mkpartition 500
/dev/st0: Input/output error

[ 3197.901583] st 5:0:1:0: [st0] Block limits 1 - 16777215 bytes.
[ 3197.903613] st 5:0:1:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 3197.903618] st 5:0:1:0: [st0] Density 44, tape length: 0, drv buffer: 1
[ 3197.903622] st 5:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
[ 3197.903625] st 5:0:1:0: [st0] Updating partition number in status.
[ 3197.906406] st 5:0:1:0: [st0] Got tape pos. blk 0 part 0.
[ 3197.906429] st 5:0:1:0: [st0] Loading tape.
[ 3197.929484] st 5:0:1:0: [st0] Block limits 1 - 16777215 bytes.
[ 3197.931518] st 5:0:1:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 3197.931524] st 5:0:1:0: [st0] Density 44, tape length: 0, drv buffer: 1
[ 3197.931527] st 5:0:1:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
[ 3197.933840] st 5:0:1:0: [st0] Partition page length is 10 bytes.
[ 3197.933846] st 5:0:1:0: [st0] PP: max 0, add 0, xdp 0, psum 03, pofmetc 0, 
rec 03, units 09, sizes: 400 65535
[ 3197.933851] st 5:0:1:0: [st0] MP: 11 08 00 00 18 03 09 00 01 90 ff ff
[ 3197.933854] st 5:0:1:0: [st0] psd_cnt 1, max.parts 0, nbr_parts 0
[ 3197.933857] st 5:0:1:0: [st0] Formatting tape with two partitions (1 = 500 
MB).
[ 3197.933860] st 5:0:1:0: [st0] Sent partition page length is 10 bytes. 
needs_format: 0
[ 3197.933865] st 5:0:1:0: [st0] PP: max 0, add 1, xdp 1, psum 02, pofmetc 0, 
rec 03, units 00, sizes: 65535 500
[ 3197.933869] st 5:0:1:0: [st0] MP: 11 08 00 01 30 03 00 00 ff ff 01 f4
[ 3197.937706] st 5:0:1:0: [st0] Error: 802, cmd: 15 10 0 0 16 0
[ 3197.937712] st 5:0:1:0: [st0] Sense Key : Illegal Request [current]
[ 3197.937716] st 5:0:1:0: [st0] Add. Sense: Invalid field in parameter list
[ 3197.937719] st 5:0:1:0: [st0] Partitioning of tape failed.
[ 3197.937847] st 5:0:1:0: [st0] Rewinding tape.

Works DDS5:

# mt -f /dev/st1 mkpartition 500

[ 3241.355474] st 6:0:3:0: [st1] Block limits 1 - 16777215 bytes.
[ 3241.355775] st 6:0:3:0: [st1] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 3241.355779] st 6:0:3:0: [st1] Density 47, tape length: 0, drv buffer: 1
[ 3241.355783] st 6:0:3:0: [st1] Block size: 0, buffer size: 4096 (1 blocks).
[ 3241.355785] st 6:0:3:0: [st1] Updating partition number in status.
[ 3241.356385] st 6:0:3:0: [st1] Got tape pos. blk 0 part 0.
[ 3241.356397] st 6:0:3:0: [st1] Loading tape.
[ 3241.357249] st 6:0:3:0: [st1] Block limits 1 - 16777215 bytes.
[ 3241.357540] st 6:0:3:0: [st1] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 3241.357544] st 6:0:3:0: [st1] Density 47, tape length: 0, drv buffer: 1
[ 3241.357547] st 6:0:3:0: [st1] Block size: 0, buffer size: 4096 (1 blocks).
[ 3241.357882] st 6:0:3:0: [st1] Partition page length is 10 bytes.
[ 3241.357887] st 6:0:3:0: [st1] PP: max 1, add 1, xdp 0, psum 02, pofmetc 0, 
rec 03, units 00, sizes: 500 65535
[ 3241.357892] st 6:0:3:0: [st1] MP: 11 08 01 01 10 03 00 00 01 f4 ff ff
[ 3241.357895] st 6:0:3:0: [st1] psd_cnt 1, max.parts 1, nbr_parts 1
[ 3241.357898] st 6:0:3:0: [st1] Formatting tape with two partitions (1 = 500 
MB).
[ 3241.357901] st 6:0:3:0: [st1] Sent partition page length is 10 bytes. 
needs_format: 0
[ 3241.357906] st 6:0:3:0: [st1] PP: max 1, add 1, xdp 1, psum 02, pofmetc 0, 
rec 03, units 00, sizes: 500 65535
[ 3241.357910] st 6:0:3:0: [st1] MP: 11 08 01 01 30 03 00 00 01 f4 ff ff
[ 3464.721058] st 6:0:3:0: [st1] Rewinding tape.

# mt -f /dev/st2 mkpartition 200G

Fails and doesn't print all of the messages related for partitioning:

[ 3514.306582] st 8:0:0:0: [st2] Block limits 1 - 16777215 bytes.
[ 3514.307126] st 8:0:0:0: [st2] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 3514.307129] st 8:0:0:0: [st2] Density 5a, tape length: 0, drv buffer: 1
[ 3514.307132] st 8:0:0:0: [st2] Block size: 0, buffer size: 4096 (1 blocks).
[ 3514.307133] st 8:0:0:0: [st2] Updating partition number in status.
[ 3514.308133] st 8:0:0:0: [st2] Got tape pos. blk 0 part 0.
[ 3514.308159] st 8:0:0:0: [st2] Loading tape.
[ 3514.323173] st 8:0:0:0: [st2] Block limits 1 - 16777215 bytes.
[ 3514.323624] st 8:0:0:0: [st2] Mode sense. Length 11, medium 0, WBS 10, BLL 8
[ 3514.323628] st 8:0:0:0: [st2] Density 5a, tape length: 0, drv buffer: 1
[ 3514.323632] st 8:0:0:0: [st2] Block size: 0, buffer size: 4096 (1 blocks).
[ 3514.324507] st 8:0:0:0: [st2] Partition page length is 16 bytes.
[ 3514.324513] st 8:0:0:0: [st2] PP: max 3, add 0, xdp 1, psum 03, pofmetc 4, 
rec 03, units 09, sizes: 2620 0
[ 3514.324518] st 8:0:0:0: [st2] MP: 11 0e 03 00 3c 03 09 00 0a 3c 00 00
[ 3514.324521] st 8:0:0:0: [st2] Sending FORMAT MEDIUM
[ 3519.097142] 

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 

[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

[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

[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

[PATCH v2]st: provide tape statistics via sysfs

[Seymour, Shane M]

New version of statistics patch:

- Removed sysfs file containing hint for number of tape drives
- Removed disk like stat file - there is now one file per statistic as per 
feedback from James
- Like all other kernel stats these start at zero and keep incrementing (cannot 
zero them any more)
- Implemented a method where the statistics files can be frozen at a point in 
time to allow consistent statistics to be read from the group of statistics 
files (permissions on the sync file limit mean you need an euid of 0 to freeze 
the statistics)
- Example output of statistics directory:

# ll /sys/class/scsi_tape/st0/device/statistics
total 0
-r--r--r--. 1 root root 4096 May  3 06:32 in_flight
-r--r--r--. 1 root root 4096 May  3 06:32 io_ms
-r--r--r--. 1 root root 4096 May  3 06:32 other_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 read_block_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 read_byte_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 read_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 read_ms
-rw-rw-r--. 1 root root 4096 May  3 06:41 sync
-r--r--r--. 1 root root 4096 May  3 06:32 write_block_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 write_byte_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 write_cnt
-r--r--r--. 1 root root 4096 May  3 06:32 write_ms

Needs the previous patch I sent this week titled st: clear driver data from 
struct device when released to be applied before this one (the diff below is 
not against a vanilla 3.9 kernel it is against 3.9 with that patch applied to 
it to get this patch).

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
---
diff -uprN -X linux-3.9-vanilla/Documentation/dontdiff 
linux-3.9-vanilla/drivers/scsi/st.c linux-3.9/drivers/scsi/st.c
--- linux-3.9-vanilla/drivers/scsi/st.c 2013-05-03 05:46:32.0 +0100
+++ linux-3.9/drivers/scsi/st.c 2013-05-03 06:46:55.0 +0100
@@ -218,6 +218,15 @@ 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 const struct sysfs_ops st_stats_sysfs_ops = {
+   .show   = st_tape_attr_show,
+   .store  = st_tape_attr_store,
+};
 
 

 #include osst_detect.h
@@ -458,10 +467,19 @@ 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;
 
+   STp-in_flight--;
+   ticks -= STp-stamp;
+   STp-io_ticks += ticks;
+   if (req-cmd[0] == WRITE_6)
+   STp-write_ticks += ticks;
+   else if (req-cmd[0] == READ_6)
+   STp-read_ticks += ticks;
+
tmp = SRpnt-bio;
if (SRpnt-waiting)
complete(SRpnt-waiting);
@@ -478,6 +496,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);
@@ -498,6 +517,18 @@ static int st_scsi_execute(struct st_req
}
}
 
+   if (cmd[0] == WRITE_6) {
+   STp-write_cnt++;
+   STp-write_byte_cnt += bufflen;
+   } else if (cmd[0] == READ_6) {
+   STp-read_cnt++;
+   STp-read_byte_cnt += bufflen;
+   } else {
+   STp-other_cnt++;
+   }
+   STp-stamp = get_jiffies_64();
+   STp-in_flight++;
+
SRpnt-bio = req-bio;
req-cmd_len = COMMAND_SIZE(cmd[0]);
memset(req-cmd, 0, BLK_MAX_CDB);
@@ -4048,6 +4079,14 @@ static int create_cdevs(struct scsi_tape
if (error)
return error;
}
+/* Create statistics directory under device, if it fails we dont
+   have statistics. */
+   tape-statistics = kobject_create_and_add(statistics,
+   tape-device-sdev_gendev.kobj);
+   if (tape-statistics != 0) {
+   st_stats_create_files(tape);
+   tape-statistics-ktype-sysfs_ops = st_stats_sysfs_ops;
+   }
 
return sysfs_create_link(tape-device-sdev_gendev.kobj,
 tape-modes[0].devs[0]-kobj, tape);
@@ -4057,6 +4096,10 @@ static void remove_cdevs(struct scsi_tap
 {
int mode, rew;
sysfs_remove_link(tape-device-sdev_gendev.kobj, tape);
+   if (tape-statistics != 0) {
+   st_stats_remove_files(tape);
+   

[PATCH]st: clear driver data from struct device when released

[Seymour, Shane M]

Patch to clear driver specific data from struct device associated with
a tape drive when released by st unload or because there was a problem
attaching to the device. Currently set in st_probe but never cleared.

Signed-off-by: Shane Seymour shane.seym...@hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
---
diff -uprN linux-3.9-vanilla/drivers/scsi/st.c linux-3.9/drivers/scsi/st.c
--- linux-3.9-vanilla/drivers/scsi/st.c 2013-04-29 01:36:01.0 +0100
+++ linux-3.9/drivers/scsi/st.c 2013-04-30 02:39:51.0 +0100
@@ -4212,6 +4212,7 @@ static int st_probe(struct device *dev)
 
 out_remove_devs:
remove_cdevs(tpnt);
+   dev_set_drvdata(dev, NULL);
spin_lock(st_index_lock);
idr_remove(st_index_idr, tpnt-index);
spin_unlock(st_index_lock);
@@ -4258,6 +4259,7 @@ static void scsi_tape_release(struct kre
struct scsi_tape *tpnt = to_scsi_tape(kref);
struct gendisk *disk = tpnt-disk;
 
+   dev_set_drvdata(tpnt-device-sdev_gendev, NULL);
tpnt-device = NULL;
 
if (tpnt-buffer) {
--
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: Take additional queue ref in st_probe

[Seymour, Shane M]

With the things I've been doing in st it was easier to reboot than unload/load 
the st driver since it would likely cause an oops. I applied the patch and have 
tested it and the st module unloads/reloads with no problems now. 
--
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] st: provide tape statistics via sysfs

[Seymour, Shane M]

-Original Message-
From: Kai Mäkisara [mailto:kai.makis...@kolumbus.fi] 
Sent: Wednesday, February 27, 2013 4:38 AM
To: James Bottomley
Cc: Seymour, Shane M; linux-scsi@vger.kernel.org
Subject: Re: [Patch][RFC] st: provide tape statistics via sysfs


 The sysfs files in the patch export the statistics in the same way as the 
 disk statistics are exported
 (cf. linux/Documentation/iostats.txt). This would enable the tools parson 
 iostats easily analyze
 statistics also for tapes.

Can I make a counter proposal that doesn't introduce any more ABI compatibility 
issues than already exist in sysfs? I'll create a statistics directory as James 
suggested and expose all of the individual statistics in their own files one 
value per file. I'll remove the extra field I wanted to add from the stat file 
and make it identical to the file provided for disk statistics and move the 
file down into the statistics directory under the device (with a warning in the 
code that this file must provide exactly the same information as the disks stat 
file and cannot provide more than that). To get the statistics I want to get 
I'll use the stat file plus one of the individual files. I'd still like to keep 
the file with the hint about how many tape drives there will be as a maximum.

 The statistics represent the status at one time point and must be read 
 together (within reason).
 This means that splitting the values to several files is not a proper way to 
 export the information.

One file would be better than two, but two is a lot better than having to open 
and read all of them individually and get stats that really are an 
approximation. 
--
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][RFC] st: provide tape statistics via sysfs

[Seymour, Shane M]

First forgive me for using outlook for this, if there are any issues with what 
I sent let me know and I'll send it again from gmail. This is also my first 
attempt at a kernel patch so please be gentle.

This patch was written to enable tape statistics via sysfs for the dt driver 
based on kernel 3.8.0-rc6. It creates two new files in sysfs and is based on 
work done previously in 2005 by Kai Mäkisara. Any feedback would be greatly 
appreciated.

Assuming sysfs is mounted at /sys the first file is 
/sys/bus/scsi/drivers/st/drives which gives a single number indicating what the 
largest tape drive instance assigned by st_probe in the st module is. If it's 4 
it possible that st0, st1, st2, and st3 exist on the system. Since tape drives 
can later be disconnected they don't have to exist, the count is a hint so it's 
possible to gather statistics in a loop with an upper bound. This makes it 
easier in iostat to gather statistcs.

The second file is /sys/class/scsi_tape/stxx/stat where xx is the instance of 
the tape drive. The file contents are almost the same as the stat file for 
disks except the merge statistics are always 0 (since tape drives are 
sequential merged I/Os don't make sense) and the inflight value is either a 0 
or 1 since the st module always only has either one read or write outstanding. 
I've also added one field to the end of the file - a count other I/Os - this 
could be commands issued by the driver within the kernel (e.g. rewind) or via 
an ioctl from user space. For tape drives some commands involving actions like 
tape movement can take a long time, it's important to keep track of scsi 
requests sent to the tape drive other than reads and writes so when delays 
happen they can be explained.

With some future patches to iostat this figure will be reported and used to 
calculate an average wait for all I/Os (a_await and oio/s in this output):

tape:   wr/s   KiB_write/srd/s  KiB_read/s  r_await  w_await  a_await  oio/s
st0   186.50 46.750.000.000.0000.2760.276   0.00
st1   186.00 93.000.000.000.0000.1800.180   0.00
st2 0.00  0.00  181.50   45.500.3470.0000.347   0.00
st3 0.00  0.00  183.00   45.750.2240.0000.224   0.00

Q: Does anyone have strong objections to extending the stat format to include 
another field (a count of scsi commands issue to the target other than reads or 
writes), or should the format stay in common with disks and a new device class 
specific file be created that provides extra statistics that may be useful only 
for a specific class of SCSI device? For example called stat-tape, stat-st or 
something else?

Onto justification we have a customer using virtual tape libraries (lots of 
drives) and they wanted to be able to monitor the activity and performance of 
their backups. Because of a lack of functionality they resorted to using a 
publicly available SystemTap script (created by RedHat presumably when they 
received similar requests from other customers):

http://sourceware.org/systemtap/wiki/WSiostatSCSI

Unfortunately, using this script occasionally results in kernel panics on older 
kernels, those issues have been addressed but most customers still don't end up 
running the SystemTap script unless they have to and they still wait to monitor 
performance of their tape drives.

Just googling: linux tape throughput statistics is enough to yield many hits on 
the topic including these:

1. http://www.ibm.com/developerworks/forums/thread.jspa?messageID=14775056
2. 
http://h30499.www3.hp.com/t5/System-Administration/How-to-get-tape-drive-performance-stats/td-p/3880235#.UKoJxNGloUo
3. http://docs.oracle.com/cd/E19455-01/816-3319/6m9k06r58/index.html

The first two are asking about getting tape stats on Linux, the reply for 1. is 
that you can get the information on AIX. 2. is similar but the reply is that 
you can get the information for HP-UX 11.31. The last one is the iostat manual 
page for Solaris which can report tape stats as well. All 3 point out that 
iostat can print tape statistics on the largest of the commercial unix 
operating systems.

Q: Does anyone have any general feedback about things that need to change or 
demands about changing the implementation before being accepted?

The checkpatch.pl script generates warnings for the diffs because of CamelToe 
however the CamelToe warnings are because I wanted to stay consistent with the 
module (look for things like STp).

Signed-off-by: Shane Seymour shane.seym...@hp.com
Signed-off-by: Darren Lavender d...@hppine99.gbr.hp.com
Tested-by: Shane Seymour shane.seym...@hp.com
Tested-by: Darren Lavender d...@hppine99.gbr.hp.com
---
diff -uprN -X linux-3.8-rc6-vanilla/Documentation/dontdiff 
linux-3.8-rc6-vanilla/drivers/scsi/st.c linux-3.8-rc6/drivers/scsi/st.c
--- linux-3.8-rc6-vanilla/drivers/scsi/st.c 2013-02-08 14:35:27.0 
+
+++ linux-3.8-rc6/drivers/scsi/st.c