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