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

[Yar Tikhiy]

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

From: Yar Tikhiy <yar at freebsd.org>
To: Bruce Evans <bde at zeta.org.au>
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: Wed, 28 May 2003 19:30:40 +0400

 On Thu, May 22, 2003 at 10:19:20PM +1000, Bruce Evans wrote:
 > 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).
 [...] 
 > > --- 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?).
 
 Right!  Setting bio_resid at the beginning of fdstrategy()
 and checking for FD_NONBLOCK makes the change smaller and cleaner.
 I've attached the new version below.
 
 However, I'm unsure if I should push this into 5.1-RELEASE.  As a
 matter of fact, this fixes an architectural bug that has (luckily)
 no impact on the system's functionality, unlike the related problem
 with panic on ioctl().
 
 -- 
 Yar
 
 --- fd.c.dist	Fri Apr 11 15:39:24 2003
 +++ fd.c	Fri May 23 20:42:24 2003
 @@ -1667,8 +1667,12 @@ fdstrategy(struct bio *bp)
  		panic("fdstrategy: buf for nonexistent device (%#lx, %#lx)",
  		      (u_long)major(bp->bio_dev), (u_long)minor(bp->bio_dev));
  	fdc = fd->fdc;
 +	bp->bio_resid = bp->bio_bcount;
  	if (fd->type == FDT_NONE || fd->ft == 0) {
 -		bp->bio_error = ENXIO;
 +		if (fd->type != FDT_NONE && (fd->flags & FD_NONBLOCK))
 +			bp->bio_error = EAGAIN;
 +		else
 +			bp->bio_error = ENXIO;
  		bp->bio_flags |= BIO_ERROR;
  		goto bad;
  	}
 @@ -1710,9 +1714,7 @@ fdstrategy(struct bio *bp)
   	nblocks = fd->ft->size;
  	if (blknum + bp->bio_bcount / fdblk > nblocks) {
  		if (blknum >= nblocks) {
 -			if (bp->bio_cmd == BIO_READ)
 -				bp->bio_resid = bp->bio_bcount;
 -			else {
 +			if (bp->bio_cmd != BIO_READ) {
  				bp->bio_error = ENOSPC;
  				bp->bio_flags |= BIO_ERROR;
  			}