Re: HEADSUP: bioops patch.
> >> Background: > > >> Ideally struct buf should have had a real OO like operations vector > >> like vnodes have it, and struct bioops is the first step towards that. > >> > >struct buf will eventually become merely an iocmd structure, so why do > >we want to complicate things here? > > No, struct buf will remain the cache-handle. the iocmd is called > struct bio. > Even if struct buf stays around, its cache-handle role will be diminished, probably just a translation layer to vm object based caching. > >We already have an OO method for bwrite: VOP_BWRITE(), the problem > >is most of the code are still calling bwrite() directly. Will it > >solve your problem if we change every bwrite() into a VOP_BWRITE()? > > Well, I'm not sure it is correct to go the detour around Vnodes, > if we do, we need to add VOP_BAWRITE, VOP_BDWRITE and all those > other operations as well. > Don't you need bp->b_ops->bawrite(), bp->b_ops->bdwrite() as well? I feel it's better to go through the vnode, because for all the bufs belong to the same vnode, these b_ops are most likely to be the same. And for a stackable filesystem, you may easily implement a passthru call with vnode, how would you deal with it using struct buf? > But what vnode would you use for a buffer associated with a freeblock > bitmap ? > Each buffer belongs to a vnode, a buffer associated with a freeblock bitmap belongs to the device vnode, and that's the vnode we could use. > -- > Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 > [EMAIL PROTECTED] | TCP/IP since RFC 956 > FreeBSD coreteam member | BSD since 4.3-tahoe > Never attribute to malice what can adequately be explained by incompetence. > -lq To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: HEADSUP: bioops patch.
On Fri, 16 Jun 2000, Nick Hibma wrote: > What about using uppercase names for > > buf_complete -> BUF_COMPLETE > > and friends to make it apparent that an indirect call is being made and Ugh. Upper case names for function-like interfaces are for ones that might be implemented as unsafe macros. > that the function might not be supported on that struct buf. Much like > newbus, kobj, and vnode ops. New interfaces like newbus and kobj shouldn't have allowed for being misimplemented as unsafe macros. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: HEADSUP: bioops patch.
What about using uppercase names for buf_complete -> BUF_COMPLETE and friends to make it apparent that an indirect call is being made and that the function might not be supported on that struct buf. Much like newbus, kobj, and vnode ops. Nick On Wed, 14 Jun 2000, Poul-Henning Kamp wrote: > > This patch virtualizes & untangles the bioops operations vector. > > Background: > > The bioops operation vector is a list of OO-like operations which can > be performed on struct buf. They are used by the softupdates code > to handle dependencies. > > Ideally struct buf should have had a real OO like operations vector > like vnodes have it, and struct bioops is the first step towards that. > > One of the reasons we should have OO-like struct buf, is that as > long as bwrite(bp) "knows" that the buffer is backed by a disk > device, we cannot use the UFS layer on top of a storage manager > which isn't based on disk-devices: When UFS modifies a directory > inode, it will call bwrite(bp) on the buffer with the data. This > would not work if the backing were based on malloc(9) or anonymous > swap-backed VM objects for instance. > > In other words: this is the main reason why we don't have a decent > tmpfs in FreeBSD. > > Instead of just assuming that it works on a disk, bwrite(bp) should > do a "bp->b_ops->bwrite(bp)" so that each buffer could have its own > private idea of how to write itself, depending on what backing it has. > > So in order to move bioops closer to become a bp->b_ops, this patch > takes two entries out of bioops: the "sync" and the "fsync" items > and virtualizes the rest of the elements a bit. > > The "sync" item is called only from the syncer, and is a call to the > softupdates code to do what it needs to do for periodic syncing. > > The real way of doing that would be to have an event-handler for this > since other code could need to be part of the sync trafic, raid5 > private parity caches could be one example. I have not done this > yet, since currently softupdates is the only client. > > The fsync item really doesn't belong in the fsync system call, it > belongs in ffs_fsync, and has been moved there. > > To give the right behaviour when SOFTUPDATES is not compiled in, > stubs for both of these functions have been added to ffs_softdep_stub.c > > Finally all the checks to see if the bioops vector is populated > has been centralized in in-line functions in thereby > paving the road for the global bioops to become bp->b_ops. > > Comments, reviews, tests please > > Poul-Henning > > Index: contrib/softupdates/ffs_softdep.c > === > RCS file: /home/ncvs/src/sys/contrib/softupdates/ffs_softdep.c,v > retrieving revision 1.64 > diff -u -r1.64 ffs_softdep.c > --- contrib/softupdates/ffs_softdep.c 2000/05/26 02:01:59 1.64 > +++ contrib/softupdates/ffs_softdep.c 2000/06/14 19:26:46 > @@ -222,8 +222,6 @@ > softdep_disk_io_initiation, /* io_start */ > softdep_disk_write_complete,/* io_complete */ > softdep_deallocate_dependencies,/* io_deallocate */ > - softdep_fsync, /* io_fsync */ > - softdep_process_worklist, /* io_sync */ > softdep_move_dependencies, /* io_movedeps */ > softdep_count_dependencies, /* io_countdeps */ > }; > Index: kern/vfs_bio.c > === > RCS file: /home/ncvs/src/sys/kern/vfs_bio.c,v > retrieving revision 1.258 > diff -u -r1.258 vfs_bio.c > --- kern/vfs_bio.c2000/05/26 02:04:39 1.258 > +++ kern/vfs_bio.c2000/06/14 19:00:56 > @@ -616,8 +616,8 @@ > newbp->b_flags &= ~B_INVAL; > > /* move over the dependencies */ > - if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_movedeps) > - (*bioops.io_movedeps)(bp, newbp); > + if (LIST_FIRST(&bp->b_dep) != NULL) > + buf_movedeps(bp, newbp); > > /* >* Initiate write on the copy, release the original to > @@ -673,10 +673,10 @@ > /* >* Process dependencies then return any unfinished ones. >*/ > - if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_complete) > - (*bioops.io_complete)(bp); > - if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_movedeps) > - (*bioops.io_movedeps)(bp, origbp); > + if (LIST_FIRST(&bp->b_dep) != NULL) > + buf_complete(bp); > + if (LIST_FIRST(&bp->b_dep) != NULL) > + buf_movedeps(bp, origbp); > /* >* Clear the BX_BKGRDINPROG flag in the original buffer >* and awaken it if it is waiting for the write to complete. > @@ -939,8 +939,8 @@ >* cache the buffer. >*/ > bp->b_flags |= B_INVAL; > - if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_deallocate
Re: HEADSUP: bioops patch.
In message <[EMAIL PROTECTED]>, Luoqi Chen write s: >> Background: >> Ideally struct buf should have had a real OO like operations vector >> like vnodes have it, and struct bioops is the first step towards that. >> >struct buf will eventually become merely an iocmd structure, so why do >we want to complicate things here? No, struct buf will remain the cache-handle. the iocmd is called struct bio. >We already have an OO method for bwrite: VOP_BWRITE(), the problem >is most of the code are still calling bwrite() directly. Will it >solve your problem if we change every bwrite() into a VOP_BWRITE()? Well, I'm not sure it is correct to go the detour around Vnodes, if we do, we need to add VOP_BAWRITE, VOP_BDWRITE and all those other operations as well. But what vnode would you use for a buffer associated with a freeblock bitmap ? -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD coreteam member | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: HEADSUP: bioops patch.
> Background: > > The bioops operation vector is a list of OO-like operations which can > be performed on struct buf. They are used by the softupdates code > to handle dependencies. > > Ideally struct buf should have had a real OO like operations vector > like vnodes have it, and struct bioops is the first step towards that. > struct buf will eventually become merely an iocmd structure, so why do we want to complicate things here? > One of the reasons we should have OO-like struct buf, is that as > long as bwrite(bp) "knows" that the buffer is backed by a disk > device, we cannot use the UFS layer on top of a storage manager > which isn't based on disk-devices: When UFS modifies a directory > inode, it will call bwrite(bp) on the buffer with the data. This > would not work if the backing were based on malloc(9) or anonymous > swap-backed VM objects for instance. > We already have an OO method for bwrite: VOP_BWRITE(), the problem is most of the code are still calling bwrite() directly. Will it solve your problem if we change every bwrite() into a VOP_BWRITE()? -lq To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
HEADSUP: bioops patch.
This patch virtualizes & untangles the bioops operations vector.
Background:
The bioops operation vector is a list of OO-like operations which can
be performed on struct buf. They are used by the softupdates code
to handle dependencies.
Ideally struct buf should have had a real OO like operations vector
like vnodes have it, and struct bioops is the first step towards that.
One of the reasons we should have OO-like struct buf, is that as
long as bwrite(bp) "knows" that the buffer is backed by a disk
device, we cannot use the UFS layer on top of a storage manager
which isn't based on disk-devices: When UFS modifies a directory
inode, it will call bwrite(bp) on the buffer with the data. This
would not work if the backing were based on malloc(9) or anonymous
swap-backed VM objects for instance.
In other words: this is the main reason why we don't have a decent
tmpfs in FreeBSD.
Instead of just assuming that it works on a disk, bwrite(bp) should
do a "bp->b_ops->bwrite(bp)" so that each buffer could have its own
private idea of how to write itself, depending on what backing it has.
So in order to move bioops closer to become a bp->b_ops, this patch
takes two entries out of bioops: the "sync" and the "fsync" items
and virtualizes the rest of the elements a bit.
The "sync" item is called only from the syncer, and is a call to the
softupdates code to do what it needs to do for periodic syncing.
The real way of doing that would be to have an event-handler for this
since other code could need to be part of the sync trafic, raid5
private parity caches could be one example. I have not done this
yet, since currently softupdates is the only client.
The fsync item really doesn't belong in the fsync system call, it
belongs in ffs_fsync, and has been moved there.
To give the right behaviour when SOFTUPDATES is not compiled in,
stubs for both of these functions have been added to ffs_softdep_stub.c
Finally all the checks to see if the bioops vector is populated
has been centralized in in-line functions in thereby
paving the road for the global bioops to become bp->b_ops.
Comments, reviews, tests please
Poul-Henning
Index: contrib/softupdates/ffs_softdep.c
===
RCS file: /home/ncvs/src/sys/contrib/softupdates/ffs_softdep.c,v
retrieving revision 1.64
diff -u -r1.64 ffs_softdep.c
--- contrib/softupdates/ffs_softdep.c 2000/05/26 02:01:59 1.64
+++ contrib/softupdates/ffs_softdep.c 2000/06/14 19:26:46
@@ -222,8 +222,6 @@
softdep_disk_io_initiation, /* io_start */
softdep_disk_write_complete,/* io_complete */
softdep_deallocate_dependencies,/* io_deallocate */
- softdep_fsync, /* io_fsync */
- softdep_process_worklist, /* io_sync */
softdep_move_dependencies, /* io_movedeps */
softdep_count_dependencies, /* io_countdeps */
};
Index: kern/vfs_bio.c
===
RCS file: /home/ncvs/src/sys/kern/vfs_bio.c,v
retrieving revision 1.258
diff -u -r1.258 vfs_bio.c
--- kern/vfs_bio.c 2000/05/26 02:04:39 1.258
+++ kern/vfs_bio.c 2000/06/14 19:00:56
@@ -616,8 +616,8 @@
newbp->b_flags &= ~B_INVAL;
/* move over the dependencies */
- if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_movedeps)
- (*bioops.io_movedeps)(bp, newbp);
+ if (LIST_FIRST(&bp->b_dep) != NULL)
+ buf_movedeps(bp, newbp);
/*
* Initiate write on the copy, release the original to
@@ -673,10 +673,10 @@
/*
* Process dependencies then return any unfinished ones.
*/
- if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_complete)
- (*bioops.io_complete)(bp);
- if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_movedeps)
- (*bioops.io_movedeps)(bp, origbp);
+ if (LIST_FIRST(&bp->b_dep) != NULL)
+ buf_complete(bp);
+ if (LIST_FIRST(&bp->b_dep) != NULL)
+ buf_movedeps(bp, origbp);
/*
* Clear the BX_BKGRDINPROG flag in the original buffer
* and awaken it if it is waiting for the write to complete.
@@ -939,8 +939,8 @@
* cache the buffer.
*/
bp->b_flags |= B_INVAL;
- if (LIST_FIRST(&bp->b_dep) != NULL && bioops.io_deallocate)
- (*bioops.io_deallocate)(bp);
+ if (LIST_FIRST(&bp->b_dep) != NULL)
+ buf_deallocate(bp);
if (bp->b_flags & B_DELWRI) {
--numdirtybuffers;
numdirtywakeup();
@@ -1570,8 +1570,8 @@
crfree(bp->b_wcred);
bp->b_wcred = NOCRED;
}
- if (LIST_FIRST(
