[Bug 212183] New: st read statistics inaccurate when requested and physical block mismatch

[Bug 212183] New: st read statistics inaccurate when requested and physical block mismatch

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=212183

            Bug ID: 212183
           Summary: st read statistics inaccurate when requested and
                    physical block mismatch
           Product: IO/Storage
           Version: 2.5
    Kernel Version: 5.3.1
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: low
          Priority: P1
         Component: SCSI
          Assignee: linux-scsi@vger.kernel.org
          Reporter: etienne.mollier@cgg.com
        Regression: No

Created attachment 295769
  --> https://bugzilla.kernel.org/attachment.cgi?id=295769&action=edit
st.c patch working around stats issue when blocks size mismatch

Greetings,

when reading from tape with requested blocks larger than physical, statistics
go wrong as using the requested size for the calculation, instead of the actual
size of the block returned.  So, when running `dd if=/dev/st0 bs=10240
of=/dev/null`, a tool such as `tapestat` will work out the bandwidth using the
bs=10240.  However, if the block on tape was of size 1024, then the metric
would go wrong by a factor 10.

Most configurations won't notice, as tapes are usually fixed size block,
typically for backup use case.  However on our end, the problem exacerbates due
to reading field acquisition cartridges with varying block size.  We are
currently working this around with the patch in attachment.

While we've observed the issue on a somewhat old kernel revision, checking out
the "master" branch of the linux tree, we believe this is present since
introduction of the capability in Linux 4.2, and likely to reappear with more
recent kernel revisions if left as-is.

Kind Regards,
Étienne.

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are the assignee for the bug.

Re: [Bug 212183] New: st read statistics inaccurate when requested and physical block mismatch

["Kai Mäkisara (Kolumbus)"]

> On 9. Mar 2021, at 16.34, bugzilla-daemon@bugzilla.kernel.org wrote:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=212183
> 
>            Bug ID: 212183
>           Summary: st read statistics inaccurate when requested and
>                    physical block mismatch
>           Product: IO/Storage
>           Version: 2.5
>    Kernel Version: 5.3.1
>          Hardware: All
>                OS: Linux
>              Tree: Mainline
>            Status: NEW
>          Severity: low
>          Priority: P1
>         Component: SCSI
>          Assignee: linux-scsi@vger.kernel.org
>          Reporter: etienne.mollier@cgg.com
>        Regression: No
> 
> Created attachment 295769
>  --> https://bugzilla.kernel.org/attachment.cgi?id=295769&action=edit
> st.c patch working around stats issue when blocks size mismatch
> 
> Greetings,
> 
> when reading from tape with requested blocks larger than physical, statistics
> go wrong as using the requested size for the calculation, instead of the actual

…

The code around your suggested patch looks like this:

		if (scsi_req(req)->result) {
			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);

Your patch makes the else branch look like the first command in the
if branch. If the SILI option bit is not set, the command result should be
non-zero when the read block is shorter than the requested size. If the
SILI bit is set, this is not considered error and the else part is executed.
Your patch applies to this case?

If we trust that the residual (resid_len) is set correctly, the conditional
branches could be omitted and the residual could be subtracted always:
-----
-diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 841ad2fc369a..4f1f2abfbca3 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -498,15 +498,11 @@ static void st_do_stats(struct scsi_tape *STp, struct request *req)
 		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 (scsi_req(req)->result) {
-			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);
+		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 {
 		now = ktime_sub(now, STp->stats->other_time);
 		atomic64_add(ktime_to_ns(now), &STp->stats->tot_io_time);
----

Opinions? (I don’t nowadays have access to any reasonable SCSI tape drive to test
this.)

Thanks,
Kai

[Bug 212183] st read statistics inaccurate when requested and physical block mismatch

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=212183

--- Comment #1 from Kai Mäkisara (kai.makisara@kolumbus.fi) ---
> On 9. Mar 2021, at 16.34, bugzilla-daemon@bugzilla.kernel.org wrote:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=212183
> 
>            Bug ID: 212183
>           Summary: st read statistics inaccurate when requested and
>                    physical block mismatch
>           Product: IO/Storage
>           Version: 2.5
>    Kernel Version: 5.3.1
>          Hardware: All
>                OS: Linux
>              Tree: Mainline
>            Status: NEW
>          Severity: low
>          Priority: P1
>         Component: SCSI
>          Assignee: linux-scsi@vger.kernel.org
>          Reporter: etienne.mollier@cgg.com
>        Regression: No
> 
> Created attachment 295769
>  --> https://bugzilla.kernel.org/attachment.cgi?id=295769&action=edit
> st.c patch working around stats issue when blocks size mismatch
> 
> Greetings,
> 
> when reading from tape with requested blocks larger than physical, statistics
> go wrong as using the requested size for the calculation, instead of the
> actual

…

The code around your suggested patch looks like this:

                if (scsi_req(req)->result) {
                        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);

Your patch makes the else branch look like the first command in the
if branch. If the SILI option bit is not set, the command result should be
non-zero when the read block is shorter than the requested size. If the
SILI bit is set, this is not considered error and the else part is executed.
Your patch applies to this case?

If we trust that the residual (resid_len) is set correctly, the conditional
branches could be omitted and the residual could be subtracted always:
-----
-diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 841ad2fc369a..4f1f2abfbca3 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -498,15 +498,11 @@ static void st_do_stats(struct scsi_tape *STp, struct
request *req)
                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 (scsi_req(req)->result) {
-                       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);
+               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 {
                now = ktime_sub(now, STp->stats->other_time);
                atomic64_add(ktime_to_ns(now), &STp->stats->tot_io_time);
----

Opinions? (I don’t nowadays have access to any reasonable SCSI tape drive to
test
this.)

Thanks,
Kai

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are the assignee for the bug.

[Bug 212183] st read statistics inaccurate when requested and physical block mismatch

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=212183

Étienne Mollier (etienne.mollier@cgg.com) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #295769|0                           |1
        is obsolete|                            |

--- Comment #2 from Étienne Mollier (etienne.mollier@cgg.com) ---
Created attachment 295817
  --> https://bugzilla.kernel.org/attachment.cgi?id=295817&action=edit
st.c patch stats when blocks size mismatch while SILI set

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are the assignee for the bug.

[Bug 212183] st read statistics inaccurate when requested and physical block mismatch

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=212183

Étienne Mollier (etienne.mollier@cgg.com) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Kernel Version|5.3.1                       |5.3.1, 5.11.6

--- Comment #3 from Étienne Mollier (etienne.mollier@cgg.com) ---
Hi Kai,

Thanks for your comment about SILI, and the suggestion to factorize things a
bit.
Yes the correction is relevant when SILI is set, so we derived a bit from your
proposal to apply the residual correction only when SILI is set.  The patch is
in
attachment of the report.

We took that opportunity to try out Linux 5.11.6.  The patch applies
successfully
on this kernel version, and statistics reported by `tapestat` look much closer
to
the actual bandwidth of the tape.

Have a nice day,  :)
Étienne.

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are the assignee for the bug.