kern/52338: fd(4) floppy disk driver & non-blocking I/O

[Bruce Evans]

The following reply was made to PR kern/52338; it has been noted by GNATS.

From: Bruce Evans <bde at zeta.org.au>
To: Yar Tikhiy <yar at freebsd.org>
Cc: FreeBSD-gnats-submit at freebsd.org, freebsd-bugs at freebsd.org,
	joerg at freebsd.org
Subject: Re: kern/52338: fd(4) floppy disk driver & non-blocking I/O
Date: Thu, 22 May 2003 22:19:20 +1000 (EST)

 On Mon, 19 May 2003, Yar Tikhiy wrote:
 
 > On Sun, May 18, 2003 at 12:39:03AM +1000, Bruce Evans wrote:
 > >
 > > It should set bp->bio_resid (to bp->bio_bcount) (bio_resid and bio_bcount
 > > are the same as b_resid and b_bcount here; strategy routines only have
 > > access to a struct bio so they must use the former).
 >
 > What do you think about the following straightforward patch to fd.c?
 > It catches all spots where BIO_ERROR is set, but bio_resid isn't.
 > In fd.c, bio_resid is never set in advance, so the simplest approach
 > should work.
 >
 > OTOH, I wonder if bio_resid could be set equal to bio_bcount at the
 > beginning of fdstrategy() and changed only if needed.  Does this have
 > any obscure implications?
 
 I think that is the right place to set it.  The residual count really
 is the full count initially.  Any obfuscations come later.  The driver
 might prefer to only set bio_resid to its final value at the end of
 normal i/o instead of as every block is finished.  Then the initial
 setting would only used for abnormal i/o.  This is essentially what
 the fd driver does (except it just forgets to set bio_resid for abnormal
 i/o).
 
 > And my other thought is:  What if physio() sets bio_resid equal to
 > bio_bcount before calling DEV_STRATEGY()?  Currently, physio()
 > leaves bio_resid unset.  An obvious drawback of this approach is
 > that it would encourage poor coding in drivers, though.
 
 Rev.1.37 of kern/subr_disk.c sets bio_resid to bio_bcount in the generic
 strategy routine.  This is a better place than physio().  The fd driver
 didn't benefit from this because it never used the disk mini-layer.
 Now geom is used instead of the disk mini-layer.  geom now uses
 essentially the same method as the fd driver for normal i/o -- it keeps
 track of the amount of i/o completed and subtracts this from bio_bcount
 to get bio_resid when i/o completes, and it doesn't set bio_resid
 initially.  The fd driver doesn't benefit from this because it doesn't
 use geom either.
 
 > --- fd.c.dist	Fri Apr 11 15:39:24 2003
 > +++ fd.c	Mon May 19 21:48:11 2003
 > @@ -1668,8 +1668,9 @@ fdstrategy(struct bio *bp)
 >  		      (u_long)major(bp->bio_dev), (u_long)minor(bp->bio_dev));
 >  	fdc = fd->fdc;
 >  	if (fd->type == FDT_NONE || fd->ft == 0) {
 > -		bp->bio_error = ENXIO;
 > +		bp->bio_error = fd->type == FDT_NONE ? ENXIO : EAGAIN;
 >  		bp->bio_flags |= BIO_ERROR;
 > +		bp->bio_resid = bp->bio_bcount;
 >  		goto bad;
 >  	}
 >  	fdblk = 128 << (fd->ft->secsize);
 
 I think this should check FD_NONBLOCK like the next clause does, and only
 return EAGAIN when FD_NONBLOCK is set.  This is clearer even if it is
 equivalent (can we get here with a type but no ft?).
 
 Bruce