diff mbox series

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

Message ID bug-212183-11613-dbRt4d1VIG@https.bugzilla.kernel.org/ (mailing list archive)
State Not Applicable
Headers show
Series [Bug,212183] st read statistics inaccurate when requested and physical block mismatch | expand

Commit Message

bugzilla-daemon@bugzilla.kernel.org March 11, 2021, 6:42 p.m. UTC 
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
                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
diff mbox series

Patch

--- 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);