Date: Wed, 30 Jul 2014 23:51:14 +0200 From: Joerg Wunsch <freebsd-scsi@uriah.heep.sax.de> To: Alexander Motin <mav@FreeBSD.org> Cc: freebsd-scsi@freebsd.org, ken@freebsd.org Subject: Re: Bacula fails on FreeBSD 10.x / "mt fsf" infinitely proceeds Message-ID: <20140730215113.GA3564@uriah.heep.sax.de> In-Reply-To: <53D95F61.4080701@FreeBSD.org> References: <20140729191829.GK3121@uriah.heep.sax.de> <20140729204354.GA78616@nargothrond.kdm.org> <20140729224414.E2AAE276@uriah.heep.sax.de> <20140730035230.GA81800@nargothrond.kdm.org> <20140730060330.GA3272@uriah.heep.sax.de> <20140730153229.GA86368@nargothrond.kdm.org> <20140730191915.9B944267B@uriah.heep.sax.de> <20140730203315.0EED1295B@uriah.heep.sax.de> <20140730204200.4645729B8@uriah.heep.sax.de> <53D95F61.4080701@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--gKMricLos+KVdGMg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline As Alexander Motin wrote: > Yes, that was me who removed that second type handling from sastart(). > The same was done for all other drivers as part of CAM simplification > efforts. Probably I missed the fact that sa(4) uses that type for > something else. Well, a quick glance at the remainder of the driver shows that the typemask is just a single bit, so if you remove one of its possible values, you should have questioned the entire type field/mask in turn, as it became useless then. #define SA_CCB_BUFFER_IO 0x0 #define SA_CCB_TYPEMASK 0x1 #define SA_POSITION_UPDATED 0x2 #define Set_CCB_Type(x, type) \ x->ccb_h.ccb_pflags &= ~SA_CCB_TYPEMASK; \ x->ccb_h.ccb_pflags |= type SA_CCB_BUFFER_WAITING was indeed not tested anywhere else, but there are decisions based on SA_CCB_BUFFER_IO (which used to be the opposite of SA_CCB_BUFFER_WAITING). These decisions now became random decisions. :-( > So while type may indeed be restored (I have no idea > about that aspect of sa(4) driver, neither have hardware to test it) Not having the hardware doesn't seem to be a good base for modifying the driver in the first place to me. Ken, I think that entire CCB_Type stuff can go away then as it is not used anymore. The only side-effect I see is that parts of MTIOCERRSTAT are rendered useless now, as the distinction between control and IO transfers is gone. Unfortunately, the new Bugzilla doesn't seem to accept me (I tried Peter Wemm's description about kinit / kpasswd), so I cannot open a bug report for it. Attached is a suggested patch. So far, I only compile-tested it. -- cheers, Joerg .-.-. --... ...-- -.. . DL8DTL http://www.sax.de/~joerg/ Never trust an operating system you don't have sources for. ;-) --gKMricLos+KVdGMg Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="scsi_sa.c.diff" Index: scsi/scsi_sa.c =================================================================== --- scsi/scsi_sa.c (Revision 269317) +++ scsi/scsi_sa.c (Arbeitskopie) @@ -113,18 +113,8 @@ #define ccb_pflags ppriv_field0 #define ccb_bp ppriv_ptr1 -#define SA_CCB_BUFFER_IO 0x0 -#define SA_CCB_TYPEMASK 0x1 #define SA_POSITION_UPDATED 0x2 -#define Set_CCB_Type(x, type) \ - x->ccb_h.ccb_pflags &= ~SA_CCB_TYPEMASK; \ - x->ccb_h.ccb_pflags |= type - -#define CCB_Type(x) (x->ccb_h.ccb_pflags & SA_CCB_TYPEMASK) - - - typedef enum { SA_FLAG_OPEN = 0x0001, SA_FLAG_FIXED = 0x0002, @@ -250,18 +240,12 @@ * Latched Error Info */ struct { - struct scsi_sense_data _last_io_sense; - u_int64_t _last_io_resid; - u_int8_t _last_io_cdb[CAM_MAX_CDBLEN]; - struct scsi_sense_data _last_ctl_sense; - u_int64_t _last_ctl_resid; - u_int8_t _last_ctl_cdb[CAM_MAX_CDBLEN]; -#define last_io_sense errinfo._last_io_sense -#define last_io_resid errinfo._last_io_resid -#define last_io_cdb errinfo._last_io_cdb -#define last_ctl_sense errinfo._last_ctl_sense -#define last_ctl_resid errinfo._last_ctl_resid -#define last_ctl_cdb errinfo._last_ctl_cdb + struct scsi_sense_data _last_sense; + u_int64_t _last_resid; + u_int8_t _last_cdb[CAM_MAX_CDBLEN]; +#define last_sense errinfo._last_sense +#define last_resid errinfo._last_resid +#define last_cdb errinfo._last_cdb } errinfo; /* * Misc other flags/state @@ -1013,18 +997,10 @@ /* * Yes, we know that this is likely to overflow */ - if (softc->last_resid_was_io) { - if ((g->mt_resid = (short) softc->last_io_resid) != 0) { - if (SA_IS_CTRL(dev) == 0 || didlockperiph) { - softc->last_io_resid = 0; - } + if ((g->mt_resid = (short) softc->last_resid) != 0) { + if (SA_IS_CTRL(dev) == 0 || didlockperiph) { + softc->last_resid = 0; } - } else { - if ((g->mt_resid = (short)softc->last_ctl_resid) != 0) { - if (SA_IS_CTRL(dev) == 0 || didlockperiph) { - softc->last_ctl_resid = 0; - } - } } error = 0; break; @@ -1038,16 +1014,11 @@ ("saioctl: MTIOCERRSTAT\n")); bzero(sep, sizeof(*sep)); - sep->io_resid = softc->last_io_resid; - bcopy((caddr_t) &softc->last_io_sense, sep->io_sense, + sep->io_resid = softc->last_resid; + bcopy((caddr_t) &softc->last_sense, sep->io_sense, sizeof (sep->io_sense)); - bcopy((caddr_t) &softc->last_io_cdb, sep->io_cdb, + bcopy((caddr_t) &softc->last_cdb, sep->io_cdb, sizeof (sep->io_cdb)); - sep->ctl_resid = softc->last_ctl_resid; - bcopy((caddr_t) &softc->last_ctl_sense, sep->ctl_sense, - sizeof (sep->ctl_sense)); - bcopy((caddr_t) &softc->last_ctl_cdb, sep->ctl_cdb, - sizeof (sep->ctl_cdb)); if ((SA_IS_CTRL(dev) == 0 && softc->open_pending_mount) || didlockperiph) @@ -1835,7 +1806,6 @@ bp->bio_data, bp->bio_bcount, SSD_FULL_SIZE, IO_TIMEOUT); start_ccb->ccb_h.ccb_pflags &= ~SA_POSITION_UPDATED; - Set_CCB_Type(start_ccb, SA_CCB_BUFFER_IO); start_ccb->ccb_h.ccb_bp = bp; bp = bioq_first(&softc->bio_queue); xpt_action(start_ccb); @@ -1863,8 +1833,6 @@ softc = (struct sa_softc *)periph->softc; csio = &done_ccb->csio; - switch (CCB_Type(csio)) { - case SA_CCB_BUFFER_IO: { struct bio *bp; int error; @@ -1943,9 +1911,7 @@ bp->bio_resid, bp->bio_bcount)); } biofinish(bp, softc->device_stats, 0); - break; } - } xpt_release_ccb(done_ccb); } @@ -2401,8 +2367,7 @@ /* * Clear I/O residual. */ - softc->last_io_resid = 0; - softc->last_ctl_resid = 0; + softc->last_resid = 0; } return (error); } @@ -2498,21 +2463,11 @@ info /= softc->media_blksize; } } - if (CCB_Type(csio) == SA_CCB_BUFFER_IO) { - bcopy((caddr_t) sense, (caddr_t) &softc->last_io_sense, - sizeof (struct scsi_sense_data)); - bcopy(csio->cdb_io.cdb_bytes, softc->last_io_cdb, - (int) csio->cdb_len); - softc->last_io_resid = resid; - softc->last_resid_was_io = 1; - } else { - bcopy((caddr_t) sense, (caddr_t) &softc->last_ctl_sense, - sizeof (struct scsi_sense_data)); - bcopy(csio->cdb_io.cdb_bytes, softc->last_ctl_cdb, - (int) csio->cdb_len); - softc->last_ctl_resid = resid; - softc->last_resid_was_io = 0; - } + bcopy((caddr_t) sense, (caddr_t) &softc->last_sense, + sizeof (struct scsi_sense_data)); + bcopy(csio->cdb_io.cdb_bytes, softc->last_cdb, + (int) csio->cdb_len); + softc->last_resid = resid; CAM_DEBUG(periph->path, CAM_DEBUG_INFO, ("CDB[0]=0x%x Key 0x%x " "ASC/ASCQ 0x%x/0x%x CAM STATUS 0x%x flags 0x%x resid %jd " "dxfer_len %d\n", csio->cdb_io.cdb_bytes[0] & 0xff, @@ -3248,7 +3203,7 @@ /* * Clear residual because we will be using it. */ - softc->last_ctl_resid = 0; + softc->last_resid = 0; softc->dsreg = (count < 0)? MTIO_DSREG_REV : MTIO_DSREG_FWD; error = cam_periph_runccb(ccb, saerror, 0, 0, softc->device_stats); @@ -3280,14 +3235,14 @@ } else if (code == SS_SETMARKS || code == SS_EOD) { softc->fileno = softc->blkno = (daddr_t) -1; } else if (code == SS_FILEMARKS && softc->fileno != (daddr_t) -1) { - softc->fileno += (count - softc->last_ctl_resid); + softc->fileno += (count - softc->last_resid); if (softc->fileno < 0) /* we must of hit BOT */ softc->fileno = 0; softc->blkno = 0; } else if (code == SS_BLOCKS && softc->blkno != (daddr_t) -1) { - softc->blkno += (count - softc->last_ctl_resid); + softc->blkno += (count - softc->last_resid); if (count < 0) { - if (softc->last_ctl_resid || softc->blkno < 0) { + if (softc->last_resid || softc->blkno < 0) { if (softc->fileno == 0) { softc->blkno = 0; } else { @@ -3314,7 +3269,7 @@ /* * Clear residual because we will be using it. */ - softc->last_ctl_resid = 0; + softc->last_resid = 0; softc->dsreg = MTIO_DSREG_FMK; /* this *must* not be retried */ @@ -3327,7 +3282,7 @@ if (error == 0 && nmarks) { struct sa_softc *softc = (struct sa_softc *)periph->softc; - nwm = nmarks - softc->last_ctl_resid; + nwm = nmarks - softc->last_resid; softc->filemarks += nwm; } --gKMricLos+KVdGMg--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140730215113.GA3564>