Re: specfs lock plumbing broken
On Sun, 5 Jan 2003, Nate Lawson wrote: > On Mon, 6 Jan 2003, Bruce Evans wrote: > > - spec_print() is of low quality: it doesn't print the device name or number. > > - devfs_print() would be reachable but doesn't exist, so vprint() prints > > even lower quality output for devfs since there nothing prints an inode > > number either. > > I was the one who left vprint in a not-so-desirable state. I plan to fix > it very soon if you can tell me what info should be printed at what > layer. For instance, several fs's print the device but this is probably > unnecessary since specfs could do this. Care to elaborate? You didn't break this :-). Printing \n\t before VOP_PRINT() in vprint() works poorly for printing the output in log files. It would be better to have everything on one line for grepping on the string in the vprint() call. ufs only prints the device of the file system. Most file systems need to do that for themself since even having a device for the file system is fs-dependent. OTOH, v_rdev is in the vnode and there are fs-independent ways to get its name[s], so it can be printed directly by vprint() However, I prefer to let lower layers handle it. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: specfs lock plumbing broken
In message <[EMAIL PROTECTED]>, Bruce Evans writes: >No. Also no INVARIANTS and the like. A profiling kernel (with profiling >not running) seemed to panic faster. Ok, in this case listening to KASSERTS would probably have helped you. Please try 1.351 of vfs_bio.c -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | 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: specfs lock plumbing broken
On Mon, 6 Jan 2003, Bruce Evans wrote: > - spec_print() is of low quality: it doesn't print the device name or number. > - devfs_print() would be reachable but doesn't exist, so vprint() prints > even lower quality output for devfs since there nothing prints an inode > number either. I was the one who left vprint in a not-so-desirable state. I plan to fix it very soon if you can tell me what info should be printed at what layer. For instance, several fs's print the device but this is probably unnecessary since specfs could do this. Care to elaborate? -Nate To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: specfs lock plumbing broken
On Sun, 5 Jan 2003 [EMAIL PROTECTED] wrote: > In message <[EMAIL PROTECTED]>, Bruce Evans writes: > >On Sun, 5 Jan 2003 [EMAIL PROTECTED] wrote: > >> This is not tested with DEVFS I take it ? > > > >It doesn't affect devfs because devfs doesn't go through ufs. It goes > >straight to the default vnodeop table so it gets std* since it doesn't > >override them. > > Uhm, no. DEVFS only goes to the default vector for directories, for > devices it goes to spec_vnoperate. Hmm, that means that the spec_vnoperate() overrides actually worked, so devfs has never had locking and fixing specfs might break devfs :-). But devfs didn't have the bug either. This is presumably ufs stays out of its way in another way: it doesn't use ffs_vget(), so the vnode is not bogusly locked initially. > >I'm getting some other panics. One while writing this was > >"bwrite: buffer is not busy???". > > Yes, I'm hunting that one atm, but havn't found a way to reproduce. > > Did you get any complaints about the wrong strategy for the wrong > type of node before this panic ? I didn't notice one for that, but there is a panic for running wine which seems to be easy to reproduce and the message occurred just before that for at least the second of 2 panics in 2 attempts to run wine here. It was something simple involving vop_stdgetpages ...-> ffsext_strategy ... ffs presumably avoids this path by having a specialized getpages. > Do you have DEBUG_VFS_LOCKS in your kernel ? No. Also no INVARIANTS and the like. A profiling kernel (with profiling not running) seemed to panic faster. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: specfs lock plumbing broken
In message <[EMAIL PROTECTED]>, Bruce Evans writes: >On Sun, 5 Jan 2003 [EMAIL PROTECTED] wrote: > >> I always wondered why specfs would insist on no locking, but I never >> had much ambition for finding out. > >Me too. It seems to be mostly a mistake. > >> >Fixing specfs is simple: >> >> This is not tested with DEVFS I take it ? > >It doesn't affect devfs because devfs doesn't go through ufs. It goes >straight to the default vnodeop table so it gets std* since it doesn't >override them. Uhm, no. DEVFS only goes to the default vector for directories, for devices it goes to spec_vnoperate. >I'm getting some other panics. One while writing this was >"bwrite: buffer is not busy???". Yes, I'm hunting that one atm, but havn't found a way to reproduce. Did you get any complaints about the wrong strategy for the wrong type of node before this panic ? Do you have DEBUG_VFS_LOCKS in your kernel ? -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | 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: specfs lock plumbing broken
On Sun, 5 Jan 2003 [EMAIL PROTECTED] wrote: > In message <[EMAIL PROTECTED]>, Bruce Evans writes: > > >The following change uncovers bugs in specfs locking and other places: > > Wow, that was fun! :-/ It took a while, yes %-). > I always wondered why specfs would insist on no locking, but I never > had much ambition for finding out. Me too. It seems to be mostly a mistake. > >Fixing specfs is simple: > > This is not tested with DEVFS I take it ? It doesn't affect devfs because devfs doesn't go through ufs. It goes straight to the default vnodeop table so it gets std* since it doesn't override them. > >Bugs found while investigating this: > >- spec_print() is unreachable because ufs_vnops.c overrides it. > >- spec_print() is of low quality: it doesn't print the device name or number. > > spec_print should probably just be retired, after all specfs is only > a set of common helper functions and not a filesystem as such. It can remove some knowledge of devices from ufs. There are similar problems for vprinting fifos. > >- the vop tables work even worse than might first appear. > > I agree, but I have no ambition to fiddle with the mechanics of them. > > >- other entries in specfs's vop table seem to be unreachable or unnecessary > > (because the default is better). > > Suggestions ? Surely the table doesn't need so many vop_panic's? Panicing for unsupported things should be the default. I can't see where some of the others are called. I'm getting some other panics. One while writing this was "bwrite: buffer is not busy???". Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: specfs lock plumbing broken
In message <[EMAIL PROTECTED]>, Bruce Evans writes: >The following change uncovers bugs in specfs locking and other places: Wow, that was fun! :-/ I always wondered why specfs would insist on no locking, but I never had much ambition for finding out. >Fixing specfs is simple: This is not tested with DEVFS I take it ? > >Bugs found while investigating this: >- spec_print() is unreachable because ufs_vnops.c overrides it. >- spec_print() is of low quality: it doesn't print the device name or number. spec_print should probably just be retired, after all specfs is only a set of common helper functions and not a filesystem as such. >- the vop tables work even worse than might first appear. I agree, but I have no ambition to fiddle with the mechanics of them. >- other entries in specfs's vop table seem to be unreachable or unnecessary > (because the default is better). Suggestions ? >- deadfs is also missing an override for vop_islocked. This is OK provided > it is never passed locked vnodes. I have no opinion on this one. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | 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
specfs lock plumbing broken
The following change uncovers bugs in specfs locking and other places:
% RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
% Working file: ufs_vnops.c
% head: 1.222
% ...
%
% revision 1.221
% date: 2003/01/04 08:47:19; author: phk; state: Exp; lines: +0 -9
% Since Jeffr made the std* functions the default in rev 1.63 of
% kern/vfs_defaults.c it is wrong for the individual filesystems to use
% the std* functions as that prevents override of the default.
%
% Found by: src/tools/tools/vop_table
%
specfs has always attempted to override the default to get no locking,
but having the std* in ufs_vnops.c overrode the override so specfs got
locking after all. This turns out to be essential for avoiding fatally
inconsistent lock states and not just for avoiding races. Without it,
the following bugs in ffs_vget() and deadfs are fatal:
- ffs_vget() returns a locked vnode ... according to ffs's idea of locking.
It calls lockmgr() directly, which gives the same result as ufs's
vop_lock which is vop_stdlock(). The vnode never gets unlocked if it
is handled by a file system whose vop_lock is vop_nolock (like specfs
with the above change).
- deadfs overrides the default for vop_lock but not for vop_unlock. So
when a vnode that was left bogusly locked by the above bugs is
revoked, deadfs_lock() is happy with it (it does nothing much), but
deadfs's vop_unlock (== vop_stdunlock) passes it to lockmgr() and
lockmgr() is often unhappy with it. For me, the bug was usually fatal
at reboot time because lockmgr() was unhappy with the shell unlocking
a vnode that was exclusively locked by a long-dead process. The
exclusive lock had no effect while the vnode was handled by specfs
because lockmgr() didn't get a chance to check it.
Fixing specfs is simple:
%%%
Index: spec_vnops.c
===
RCS file: /home/ncvs/src/sys/fs/specfs/spec_vnops.c,v
retrieving revision 1.193
diff -u -2 -r1.193 spec_vnops.c
--- spec_vnops.c5 Jan 2003 10:03:57 - 1.193
+++ spec_vnops.c5 Jan 2003 15:58:55 -
@@ -85,9 +89,7 @@
{ &vop_getwritemount_desc, (vop_t *) vop_stdgetwritemount },
{ &vop_ioctl_desc, (vop_t *) spec_ioctl },
- { &vop_islocked_desc, (vop_t *) vop_noislocked },
{ &vop_kqfilter_desc, (vop_t *) spec_kqfilter },
{ &vop_lease_desc, (vop_t *) vop_null },
{ &vop_link_desc, (vop_t *) vop_panic },
- { &vop_lock_desc, (vop_t *) vop_nolock },
{ &vop_mkdir_desc, (vop_t *) vop_panic },
{ &vop_mknod_desc, (vop_t *) vop_panic },
@@ -108,5 +110,4 @@
{ &vop_strategy_desc, (vop_t *) spec_strategy },
{ &vop_symlink_desc,(vop_t *) vop_panic },
- { &vop_unlock_desc, (vop_t *) vop_nounlock },
{ &vop_write_desc, (vop_t *) spec_write },
{ NULL, NULL }
%%%
Bugs found while investigating this:
- spec_print() is unreachable because ufs_vnops.c overrides it.
- spec_print() is of low quality: it doesn't print the device name or number.
- devfs_print() would be reachable but doesn't exist, so vprint() prints
even lower quality output for devfs since there nothing prints an inode
number either.
- the vop tables work even worse than might first appear.
- other entries in specfs's vop table seem to be unreachable or unnecessary
(because the default is better).
- deadfs is also missing an override for vop_islocked. This is OK provided
it is never passed locked vnodes.
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
