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

[bugzilla-daemon@xxxxxxxxxxxxxxxxxxx]

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

--- Comment #1 from Kai Mäkisara (kai.makisara@xxxxxxxxxxx) ---
> On 9. Mar 2021, at 16.34, bugzilla-daemon@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx
>          Reporter: etienne.mollier@xxxxxxx
>        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.