Re: st.c broken ? (may have been: Re: CVS commit: src/sys - remove b_flags manipulation)

[Juergen Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost>]

On Sat, Aug 25, 2007 at 10:24:01AM +0200, Frank Kardel wrote:
> Hi Andrew, * !
> 
> I am experiencing quite a few strange error with my DAT tape since about
> this checkin. A kernel from the beginning of July works flawlessly with the
> drive and the tape.
> 
> Symptoms:
>     I/O errors claimed where no real I/O error where. Possibly
>    triggered by length indicator mismatches reported by the drive. These
>   are ok for variable block size formatted media. Those indications where
>   handled properly at the beginning of July and years before.
> 
> The dmesg output shows a READ that is terminated with an I/O error,
> but data was transferred (with the size mismatch indication).
> The requested block was just too big - a smaller block was read.
> The second attempt with a block too small was terminated with a panic 
> instead
> of an I/O error.
> 
> Could somebody check what broke st.c between beginning of July and now ?
> Currently st.c is unusable for variable block size tape I/O.
> 
> dmesg atteched.
> 
> Regards,
>  Frank
> 
> Andrew Doran wrote:
> >Module Name: src
> >Committed By:        ad
> >Date:                Sun Jul 29 13:31:18 UTC 2007
> >
> >Modified Files:
> >     src/sys/arch/amiga/dev: fd.c
> >     src/sys/arch/dreamcast/dev/maple: mmemcard.c
> >     src/sys/dev/ieee1394: firewirereg.h fwdev.c fwmem.c
> >     src/sys/fs/cd9660: cd9660_vnops.c
> >     src/sys/fs/efs: efs_vnops.c
> >     src/sys/fs/ntfs: ntfs_vnops.c
> >     src/sys/fs/puffs: puffs_subr.c puffs_vnops.c
> >     src/sys/fs/smbfs: smbfs_io.c
> >     src/sys/fs/sysvbfs: sysvbfs_vnops.c
> >     src/sys/fs/udf: udf_subr.c
> >     src/sys/miscfs/deadfs: dead_vnops.c
> >     src/sys/miscfs/specfs: spec_vnops.c
> >     src/sys/nfs: nfs_bio.c
> >     src/sys/ufs/ffs: ffs_softdep.c
> >     src/sys/ufs/lfs: lfs_bio.c lfs_segment.c lfs_vfsops.c lfs_vnops.c
> >     src/sys/ufs/mfs: mfs_vnops.c
> >     src/sys/ufs/ufs: ufs_vnops.c
> >     src/sys/uvm: uvm_pager.c uvm_swap.c
> >
> >Log Message:
> >It's not a good idea for device drivers to modify b_flags, as they don't
> >need to understand the locking around that field. Instead of setting
> >B_ERROR, set b_error instead. b_error is 'owned' by whoever completes
> >the I/O request.
[snip]

Same here, reading a tape file yields EIO on end-of-file.  Looks like the
change to physio_done() removed the EOM handling.  Before we got

    mbp->b_error = 0;
    mbp->b_flags |= B_ERROR;

now we get

    mbp->b_error = EIO;

@@ -176,28 +176,16 @@ physio_done(struct work *wk, void *dummy
 
                if (mbp->b_endoffset == -1 || endoffset < mbp->b_endoffset) {
-                       int error;
-
-                       if ((bp->b_flags & B_ERROR) != 0) {
-                               if (bp->b_error == 0) {
-                                       error = EIO; /* XXX */
-                               } else {
-                                       error = bp->b_error;
-                               }
-                       } else {
-                               error = 0; /* EOM */
-                       }
-
                        DPRINTF(("%s: mbp=%p, error %d -> %d, endoff %" PRIu64
                            " -> %" PRIu64 "\n",
                            __func__, mbp,
-                           mbp->b_error, error,
+                           mbp->b_error, bp->b_error,
                            mbp->b_endoffset, endoffset));
 
                        mbp->b_endoffset = endoffset;
-                       mbp->b_error = error;
+                       mbp->b_error = bp->b_error;
                }
-               mbp->b_flags |= B_ERROR;
+               mbp->b_error = EIO;
        } else {
-               KASSERT((bp->b_flags & B_ERROR) == 0);
+               KASSERT(bp->b_error == 0);
        }
 

-- 
Juergen Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)