Date: Wed, 28 May 2003 19:30:40 +0400 From: Yar Tikhiy <yar@freebsd.org> To: Bruce Evans <bde@zeta.org.au> Cc: joerg@freebsd.org Subject: Re: kern/52338: fd(4) floppy disk driver & non-blocking I/O Message-ID: <20030528153040.GA34472@comp.chem.msu.su> In-Reply-To: <20030522220631.P43165@gamplex.bde.org> References: <200305161646.h4GGkdDS000677@stylish.chem.msu.su> <20030517165718.B15076@gamplex.bde.org> <20030517091047.GA82314@comp.chem.msu.su> <20030517235738.U16304@gamplex.bde.org> <20030519182357.GA99588@comp.chem.msu.su> <20030522220631.P43165@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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;
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030528153040.GA34472>