> 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
