Date: Sun, 02 Apr 2000 18:16:27 -0700 From: Peter Wemm <peter@netplex.com.au> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: Poul-Henning Kamp <phk@critter.freebsd.dk>, cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern subr_devstat.c src/sys/ufs/ufs ufs_disksubr.c src/sys/sys buf.h devicestat.h disklabel.h Message-ID: <20000403011627.6E7AE1CD7@overcee.netplex.com.au> In-Reply-To: Message from Matthew Dillon <dillon@apollo.backplane.com> of "Sun, 02 Apr 2000 16:26:37 PDT." <200004022326.QAA51447@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Matthew Dillon wrote:
>
> :
> :In message <200004022223.PAA51020@apollo.backplane.com>, Matthew Dillon writ
es:
> :
> :> b_data is integral to buffer cache KVA management & operation and
> :> should be in the main struct buf structure, not the bio structure.
> :
> :No, b_data is (currently) also a property of the I/O request.
>
> No it isn't. b_data is the property of the buffer cache. If you
> screw with it, you blow up the buffer cache. Any device driver that
> screws with it must restore it prior to reentering the buffer cache
> code.
[..]
> If you want you can duplicate b_data ... that is, have the buffer cache
> managed KVA be a field in struct buf and copy it into a new field in the
> bio which the device drivers use (and get rid of the save field that
> the device drivers currently use to save/restore b_data). But that isn't
> what you've done.
I think this is probably the better thing to do - ie: duplicate it and also
the B_ERROR flag. As you point out, brelse() uses it. However, it's also
part of the driver<->bio interaction.
This is part of the problem with doing this incrementally and trying to keep
disk drivers working with backwards compatability.
Consider: B_ERROR is used by the buffer cache to clean up after transaction
failures (eg: preserving data after write failures for retry) etc and/or
for reporting errno's. But B_ERROR was also a way the driver reported the
error to the bio layer. What I suspect is the "correct" answer is to
duplicate it so that there is an error reporting mechanism between the
IO system and drivers, and any error returns are propagated back up to
b_flags |= B_ERROR for the buffer cache so that it can do the right thing.
But doing that with incremental code changes is a pain because presently
it's all #defined for backwards compatability.
I'm sure I'm not describing my point clearly, but it's worth a try.
Putting it differently (assuming bio != buffer cache terminology)
- bio_data is a property of a 'bio' transaction
- BIO_ERROR is an error return mechanism of a bio <-> driver transaction
- A bio transaction may not necessarily be on a "buffer" - it could be on
anything, including a page, a block of data, etc.
but also:
- b_data is a property of the buffer cache
- B_ERROR is also an attribute of an individual buffer in the buffer cache.
- buffer cache 'buffers' are not part of the IO request subsystem.
I think Poul-Henning is on the right track, and there is plenty of time
to get this "right". Temporarily breaking layering so that this can be
done incrementally is probably OK *providing* that the big picture is kept
in mind and that the layering is fixed. IMHO, redoing the device driver
bio transaction layer to take 'struct bio' instead of 'struct buf' is the
bigger footprint change and is a prerequisite of splitting the bio and buf
structures - at the point that bio and buf split properly then we have
to deal with and restore b_data and B_ERROR and propagate them back up
properly.
Does that make any sense? (It's times like this I wish I hadn't given
up coffee!)
Cheers,
-Peter
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20000403011627.6E7AE1CD7>