From owner-freebsd-fs Fri Feb 27 07:49:26 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id HAA05689 for freebsd-fs-outgoing; Fri, 27 Feb 1998 07:49:26 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from ns1.yes.no (ns1.yes.no [195.119.24.10]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id HAA05669 for ; Fri, 27 Feb 1998 07:49:22 -0800 (PST) (envelope-from eivind@bitbox.follo.net) Received: from bitbox.follo.net (bitbox.follo.net [194.198.43.36]) by ns1.yes.no (8.8.7/8.8.7) with ESMTP id PAA01491 for ; Fri, 27 Feb 1998 15:49:20 GMT Received: (from eivind@localhost) by bitbox.follo.net (8.8.6/8.8.6) id QAA00374; Fri, 27 Feb 1998 16:48:59 +0100 (MET) Message-ID: <19980227164859.25557@follo.net> Date: Fri, 27 Feb 1998 16:48:59 +0100 From: Eivind Eklund To: fs@FreeBSD.ORG Subject: syncer / SMP question Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.89.1i Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org I was looking at implementing an incremental syncer for UFS, as a sort of "let's get to know the FS-code" project, and noticed something that looked like really strange code in the sync() syscall. Is there any reason why the below wouldn't be a benign change? The extra simplelock-call looks especially weird - it looks like either the lock is released somewhere else, or we'll have the mountlist locked many times. If it looks like a benign change, what would I do to test it properly? I can allocate a scratch machine to do this if there is no need for really long-running tests. I can probably even do a scratchable SMP-box. cvs diff: Diffing . Index: vfs_syscalls.c =================================================================== RCS file: /home/ncvs/src/sys/kern/vfs_syscalls.c,v retrieving revision 1.93 diff -u -u -5 -r1.93 vfs_syscalls.c --- vfs_syscalls.c 1998/02/15 04:17:09 1.93 +++ vfs_syscalls.c 1998/02/27 15:37:16 @@ -480,24 +480,21 @@ register struct mount *mp, *nmp; int asyncflag; simple_lock(&mountlist_slock); for (mp = mountlist.cqh_first; mp != (void *)&mountlist; mp = nmp) { + nmp = mp->mnt_list.cqe_next; if (vfs_busy(mp, LK_NOWAIT, &mountlist_slock, p)) { - nmp = mp->mnt_list.cqe_next; continue; } if ((mp->mnt_flag & MNT_RDONLY) == 0) { asyncflag = mp->mnt_flag & MNT_ASYNC; mp->mnt_flag &= ~MNT_ASYNC; vfs_msync(mp, MNT_NOWAIT); VFS_SYNC(mp, MNT_NOWAIT, p != NULL ? p->p_ucred : NOCRED, p); - if (asyncflag) - mp->mnt_flag |= MNT_ASYNC; + mp->mnt_flag |= asyncflag; } - simple_lock(&mountlist_slock); - nmp = mp->mnt_list.cqe_next; vfs_unbusy(mp, p); } simple_unlock(&mountlist_slock); #if 0 /* Eivind. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 07:57:16 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id HAA08031 for freebsd-fs-outgoing; Fri, 27 Feb 1998 07:57:16 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from ns1.yes.no (ns1.yes.no [195.119.24.10]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id HAA07993 for ; Fri, 27 Feb 1998 07:57:05 -0800 (PST) (envelope-from eivind@bitbox.follo.net) Received: from bitbox.follo.net (bitbox.follo.net [194.198.43.36]) by ns1.yes.no (8.8.7/8.8.7) with ESMTP id PAA01659 for ; Fri, 27 Feb 1998 15:56:59 GMT Received: (from eivind@localhost) by bitbox.follo.net (8.8.6/8.8.6) id QAA00456; Fri, 27 Feb 1998 16:56:38 +0100 (MET) Message-ID: <19980227165638.57458@follo.net> Date: Fri, 27 Feb 1998 16:56:38 +0100 From: Eivind Eklund To: fs@FreeBSD.ORG Subject: Re: syncer / SMP question References: <19980227164859.25557@follo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.89.1i In-Reply-To: <19980227164859.25557@follo.net>; from Eivind Eklund on Fri, Feb 27, 1998 at 04:48:59PM +0100 Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Fri, Feb 27, 1998 at 04:48:59PM +0100, Eivind Eklund wrote: > I was looking at implementing an incremental syncer for UFS, as a sort > of "let's get to know the FS-code" project, and noticed something that > looked like really strange code in the sync() syscall. Is there any > reason why the below wouldn't be a benign change? The extra > simplelock-call looks especially weird - it looks like either the lock > is released somewhere else, or we'll have the mountlist locked many > times. I looked a bit further. If the sync patch is incorrect (ie, the lock can't be moved that way), is there any reason why this simple_lock(&mountlist_slock); for (mp = mountlist.cqh_first; mp != (void *)&mountlist; mp = nmp) { if (vfs_busy(mp, LK_NOWAIT, &mountlist_slock, p)) { >>>>>>> nmp = mp->mnt_list.cqe_next; continue; >>>>>>> } is not a race-condition? I'm not certain I get all of this. Eivind. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 08:14:37 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id IAA10783 for freebsd-fs-outgoing; Fri, 27 Feb 1998 08:14:37 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from critter.freebsd.dk (critter.freebsd.dk [195.8.129.14]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id IAA10740 for ; Fri, 27 Feb 1998 08:14:22 -0800 (PST) (envelope-from phk@critter.freebsd.dk) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.8.7/8.8.5) with ESMTP id RAA03418; Fri, 27 Feb 1998 17:07:05 +0100 (CET) To: Eivind Eklund cc: fs@FreeBSD.ORG Subject: Re: syncer / SMP question In-reply-to: Your message of "Fri, 27 Feb 1998 16:48:59 +0100." <19980227164859.25557@follo.net> Date: Fri, 27 Feb 1998 17:07:05 +0100 Message-ID: <3416.888595625@critter.freebsd.dk> From: Poul-Henning Kamp Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Uhm, this is part&parcel of Kirks softupdate code, so I wouldn't bother if I were you... Poul-Henning In message <19980227164859.25557@follo.net>, Eivind Eklund writes: >I was looking at implementing an incremental syncer for UFS, as a sort >of "let's get to know the FS-code" project, and noticed something that >looked like really strange code in the sync() syscall. Is there any >reason why the below wouldn't be a benign change? The extra >simplelock-call looks especially weird - it looks like either the lock >is released somewhere else, or we'll have the mountlist locked many >times. > >If it looks like a benign change, what would I do to test it properly? >I can allocate a scratch machine to do this if there is no need for >really long-running tests. I can probably even do a scratchable >SMP-box. > >cvs diff: Diffing . >Index: vfs_syscalls.c >=================================================================== >RCS file: /home/ncvs/src/sys/kern/vfs_syscalls.c,v >retrieving revision 1.93 >diff -u -u -5 -r1.93 vfs_syscalls.c >--- vfs_syscalls.c 1998/02/15 04:17:09 1.93 >+++ vfs_syscalls.c 1998/02/27 15:37:16 >@@ -480,24 +480,21 @@ > register struct mount *mp, *nmp; > int asyncflag; > > simple_lock(&mountlist_slock); > for (mp = mountlist.cqh_first; mp != (void *)&mountlist; mp = nmp) { >+ nmp = mp->mnt_list.cqe_next; > if (vfs_busy(mp, LK_NOWAIT, &mountlist_slock, p)) { >- nmp = mp->mnt_list.cqe_next; > continue; > } > if ((mp->mnt_flag & MNT_RDONLY) == 0) { > asyncflag = mp->mnt_flag & MNT_ASYNC; > mp->mnt_flag &= ~MNT_ASYNC; > vfs_msync(mp, MNT_NOWAIT); > VFS_SYNC(mp, MNT_NOWAIT, p != NULL ? p->p_ucred : NOC >RED, p); >- if (asyncflag) >- mp->mnt_flag |= MNT_ASYNC; >+ mp->mnt_flag |= asyncflag; > } >- simple_lock(&mountlist_slock); >- nmp = mp->mnt_list.cqe_next; > vfs_unbusy(mp, p); > } > simple_unlock(&mountlist_slock); > #if 0 > /* > >Eivind. >To Unsubscribe: send mail to majordomo@FreeBSD.org >with "unsubscribe freebsd-fs" in the body of the message > -- Poul-Henning Kamp FreeBSD coreteam member phk@FreeBSD.ORG "Real hackers run -current on their laptop." "Drink MONO-tonic, it goes down but it will NEVER come back up!" To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 08:26:33 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id IAA13525 for freebsd-fs-outgoing; Fri, 27 Feb 1998 08:26:33 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from sumatra.americantv.com (sumatra.americantv.com [207.170.17.37]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id IAA13500 for ; Fri, 27 Feb 1998 08:26:29 -0800 (PST) (envelope-from jlemon@americantv.com) Received: from right.PCS (right.PCS [148.105.10.31]) by sumatra.americantv.com (8.8.5/8.8.5) with ESMTP id KAA22559; Fri, 27 Feb 1998 10:26:27 -0600 (CST) Received: (from jlemon@localhost) by right.PCS (8.6.13/8.6.4) id KAA23915; Fri, 27 Feb 1998 10:25:56 -0600 Message-ID: <19980227102555.05064@right.PCS> Date: Fri, 27 Feb 1998 10:25:55 -0600 From: Jonathan Lemon To: Eivind Eklund Cc: fs@FreeBSD.ORG Subject: Re: syncer / SMP question References: <19980227164859.25557@follo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.61.1 In-Reply-To: <19980227164859.25557@follo.net>; from Eivind Eklund on Feb 02, 1998 at 04:48:59PM +0100 Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Feb 02, 1998 at 04:48:59PM +0100, Eivind Eklund wrote: > I was looking at implementing an incremental syncer for UFS, as a sort > of "let's get to know the FS-code" project, and noticed something that > looked like really strange code in the sync() syscall. Is there any > reason why the below wouldn't be a benign change? The extra > simplelock-call looks especially weird - it looks like either the lock > is released somewhere else, or we'll have the mountlist locked many > times. It appears to be released. The vfs_busy() routine makes a call to lockmgr(), which sets a lock on mountpoint and releases the lock on the mountlist. I agree that the lock calls look weird. Perhaps someone could explain the purpose of the various locks? It appears that you need to get a lock on the mountlist in order to iterate the mountpoints. But why does vfs_busy() (which operates on a mountpoint) release your mountlist lock? Convenience? (Yes, I know that simple_lock is only for SMP, but still...) As for hoisting up the calculation of nmp, I don't think you can do that, since the inner portion releases it's lock on the mountlist, meaning that the another processor can change the mountlist out from underneath you, invalidating your (saved) nmp. -- Jonathan To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 10:02:04 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id KAA28136 for freebsd-fs-outgoing; Fri, 27 Feb 1998 10:02:04 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from ns1.yes.no (ns1.yes.no [195.119.24.10]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id KAA28084 for ; Fri, 27 Feb 1998 10:01:59 -0800 (PST) (envelope-from eivind@bitbox.follo.net) Received: from bitbox.follo.net (bitbox.follo.net [194.198.43.36]) by ns1.yes.no (8.8.7/8.8.7) with ESMTP id SAA06032; Fri, 27 Feb 1998 18:01:56 GMT Received: (from eivind@localhost) by bitbox.follo.net (8.8.6/8.8.6) id TAA00976; Fri, 27 Feb 1998 19:01:33 +0100 (MET) Message-ID: <19980227190132.53798@follo.net> Date: Fri, 27 Feb 1998 19:01:32 +0100 From: Eivind Eklund To: Jonathan Lemon , Eivind Eklund Cc: fs@FreeBSD.ORG Subject: Re: syncer / SMP question References: <19980227164859.25557@follo.net> <19980227102555.05064@right.PCS> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.89.1i In-Reply-To: <19980227102555.05064@right.PCS>; from Jonathan Lemon on Fri, Feb 27, 1998 at 10:25:55AM -0600 Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Fri, Feb 27, 1998 at 10:25:55AM -0600, Jonathan Lemon wrote: > On Feb 02, 1998 at 04:48:59PM +0100, Eivind Eklund wrote: > > I was looking at implementing an incremental syncer for UFS, as a sort > > of "let's get to know the FS-code" project, and noticed something that > > looked like really strange code in the sync() syscall. Is there any > > reason why the below wouldn't be a benign change? The extra > > simplelock-call looks especially weird - it looks like either the lock > > is released somewhere else, or we'll have the mountlist locked many > > times. > > It appears to be released. The vfs_busy() routine makes a call to > lockmgr(), which sets a lock on mountpoint and releases the lock on > the mountlist. But... But... What about the fetch of the next-pointer? Looks like a potential race-condition to me, but this includes some lock-calls I don't really understand what do (interlocks? What are they?). > I agree that the lock calls look weird. Perhaps someone could explain > the purpose of the various locks? It appears that you need to get a lock > on the mountlist in order to iterate the mountpoints. But why does > vfs_busy() (which operates on a mountpoint) release your mountlist lock? > Convenience? Looks likely. The functionality is actually only used five places: sync(), getfsstat(), printlockedvnodes(), sysctl_vnode(), and nqnfs_lease_updatetime(), all in equal-looking loops. By comparison, vfs_busy itself is used in 15 places. Perhaps a WALK_MOUNTLIST macro would be better? I use #define LIST_WALK_FORWARD(list, node, extra_node) \ for ((node) = (list)->pHead; \ (node) = (extra_node); \ (extra_node) = (node)->pNext) for my list walking. This could be done for mountlists, too, and abstract away all knowledge of the mountlist/lock relation from the actual consumer. The below could handle everything in a simple macro #define WALK_MOUNT_LIST(ml, mp, nmp, p) \ for((mp) = _vfs_getfirstmountp((ml), (p)), \ (mp) || (vfs_unbusy((mp), (p)),0); \ (mp) = _vfs_getnextmountp((ml), (mp), (p))) and two functions struct mount * _vfs_firstmountp(ml, p) struct mntlist *ml; struct proc *p; { struct mount *mp; simple_lock(&mountlist_slock); mp = ml->cqh_first; while ((mp != (void*)ml) && vfs_busy(mp, LK_NOWAIT, &mountlist_slock, p)) { mp = mp->mnt_list.cqe_next; } if (mp == (void*)ml) mp = NULL; simple_unlock(&mountlist_slock); return mp; } struct mount * _vfs_getnextmountp(ml, mp, p) struct mntlist *ml; struct mount *mp; struct proc *p; { struct mount *nmp; simple_lock(&mountlist_slock); nmp = mp->mnt_list.cqe_next; vfs_unbusy(mp); mp = nmp; while (vfs_busy(mp, LK_NOWAIT, &mountlist_slock, p)) { mp = mp->mnt_list.cqe_next; if (mp == (void*)ml) { simple_unlock(&mountlist_slock); return NULL; } } simple_unlock(&mountlist_slock); return mp; } Yes, it is probably more code than presently (at least if you count it as lines of C-code), but it abstract away all the complexity, and make the interfaces clean. Locking and unlocking at different layers is IMHO not clean; it 'smells of trouble'. > (Yes, I know that simple_lock is only for SMP, but still...) > > As for hoisting up the calculation of nmp, I don't think you can do that, > since the inner portion releases it's lock on the mountlist, meaning that > the another processor can change the mountlist out from underneath you, > invalidating your (saved) nmp. Yes, I later noticed. What do you think of the above? Eivind. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 11:20:25 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id LAA06498 for freebsd-fs-outgoing; Fri, 27 Feb 1998 11:20:25 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from smtp01.primenet.com (smtp01.primenet.com [206.165.6.131]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id LAA06471 for ; Fri, 27 Feb 1998 11:20:20 -0800 (PST) (envelope-from tlambert@usr05.primenet.com) Received: (from daemon@localhost) by smtp01.primenet.com (8.8.8/8.8.8) id MAA15243; Fri, 27 Feb 1998 12:20:19 -0700 (MST) Received: from usr05.primenet.com(206.165.6.205) via SMTP by smtp01.primenet.com, id smtpd015207; Fri Feb 27 12:20:16 1998 Received: (from tlambert@localhost) by usr05.primenet.com (8.8.5/8.8.5) id MAA17452; Fri, 27 Feb 1998 12:20:08 -0700 (MST) From: Terry Lambert Message-Id: <199802271920.MAA17452@usr05.primenet.com> Subject: Re: syncer / SMP question To: eivind@yes.no (Eivind Eklund) Date: Fri, 27 Feb 1998 19:20:08 +0000 (GMT) Cc: jlemon@americantv.com, eivind@yes.no, fs@FreeBSD.ORG In-Reply-To: <19980227190132.53798@follo.net> from "Eivind Eklund" at Feb 27, 98 07:01:32 pm X-Mailer: ELM [version 2.4 PL25] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org > > It appears to be released. The vfs_busy() routine makes a call to > > lockmgr(), which sets a lock on mountpoint and releases the lock on > > the mountlist. > > But... But... What about the fetch of the next-pointer? Looks like > a potential race-condition to me, but this includes some lock-calls I > don't really understand what do (interlocks? What are they?). The placement of the nmp assignment is correct. This allows the nmp to *not* be advanced through several loop iterations, if necessary, apart from the other benefits noted. > > I agree that the lock calls look weird. Perhaps someone could explain > > the purpose of the various locks? It appears that you need to get a lock > > on the mountlist in order to iterate the mountpoints. But why does > > vfs_busy() (which operates on a mountpoint) release your mountlist lock? > > Convenience? > > Looks likely. The functionality is actually only used five places: > sync(), getfsstat(), printlockedvnodes(), sysctl_vnode(), and > nqnfs_lease_updatetime(), all in equal-looking loops. Almost all of the lock code is non-reflexive. If you consider memory allocations as pointer/region locks, then even more of it is screwed (my favorite example here is "namei" allocating the path buffer, and each FS being expected to free it). > Yes, it is probably more code than presently (at least if you count it > as lines of C-code), but it abstract away all the complexity, and make > the interfaces clean. Locking and unlocking at different layers is > IMHO not clean; it 'smells of trouble'. Heh... "Welcome to my world". I'm also in favor of documenting locks held in and out of all functions in the function comments, and making all functions single-entry/single-exit to facilitate state tracking and enforcement in debug kernels (using assertions). > > (Yes, I know that simple_lock is only for SMP, but still...) > > > > As for hoisting up the calculation of nmp, I don't think you can do that, > > since the inner portion releases it's lock on the mountlist, meaning that > > the another processor can change the mountlist out from underneath you, > > invalidating your (saved) nmp. > > Yes, I later noticed. What do you think of the above? I think you are bailing the ocean with a bucket. This whole issue needs a top level design, and then style enforcement, not just a onesey-twosey fix where it's obvious. You will need to drag in the core team for a style change discussion. Terry Lambert terry@lambert.org --- Any opinions in this posting are my own and not those of my present or previous employers. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 11:58:23 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id LAA17469 for freebsd-fs-outgoing; Fri, 27 Feb 1998 11:58:23 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from sumatra.americantv.com (sumatra.americantv.com [207.170.17.37]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id LAA17464 for ; Fri, 27 Feb 1998 11:58:18 -0800 (PST) (envelope-from jlemon@americantv.com) Received: from right.PCS (right.PCS [148.105.10.31]) by sumatra.americantv.com (8.8.5/8.8.5) with ESMTP id NAA22981; Fri, 27 Feb 1998 13:58:05 -0600 (CST) Received: (from jlemon@localhost) by right.PCS (8.6.13/8.6.4) id NAA12914; Fri, 27 Feb 1998 13:57:33 -0600 Message-ID: <19980227135733.19894@right.PCS> Date: Fri, 27 Feb 1998 13:57:33 -0600 From: Jonathan Lemon To: Eivind Eklund Cc: fs@FreeBSD.ORG, jlemon@americantv.com Subject: Re: syncer / SMP question References: <19980227164859.25557@follo.net> <19980227102555.05064@right.PCS> <19980227190132.53798@follo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.61.1 In-Reply-To: <19980227190132.53798@follo.net>; from Eivind Eklund on Feb 02, 1998 at 07:01:32PM +0100 Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Feb 02, 1998 at 07:01:32PM +0100, Eivind Eklund wrote: > Yes, I later noticed. What do you think of the above? I think I'll take the original code. :-) While layering is nice, it imposes a performance hit. Also, I think that the above is overkill, and just serves to obfuscate the code. It took me longer to understand the macros above than it did to read the original implementation. IMHO, all the original code really needs is a nice little comment stating that vfs_busy() releases the lock on the mountlist if it succeeds. I'll also note that it appears possible to pass a NULL third argument to vfs_busy() and thus have it not release the lock on the mountlist. Then simple_unlock would be called explicitly in the sync() function, also making things clearer. I'm unsure if the lock release done by vfs_busy/lockmgr was for 1) convenience, or 2) some form of atomic unlocking. Perhaps someone else could explain? Terry? Poul? -- Jonathan To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 13:31:02 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id NAA02455 for freebsd-fs-outgoing; Fri, 27 Feb 1998 13:31:02 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from smtp01.primenet.com (smtp01.primenet.com [206.165.6.131]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id NAA02446 for ; Fri, 27 Feb 1998 13:30:53 -0800 (PST) (envelope-from tlambert@usr05.primenet.com) Received: (from daemon@localhost) by smtp01.primenet.com (8.8.8/8.8.8) id OAA29982; Fri, 27 Feb 1998 14:30:41 -0700 (MST) Received: from usr05.primenet.com(206.165.6.205) via SMTP by smtp01.primenet.com, id smtpdb29671; Fri Feb 27 14:30:37 1998 Received: (from tlambert@localhost) by usr05.primenet.com (8.8.5/8.8.5) id OAA24114; Fri, 27 Feb 1998 14:26:44 -0700 (MST) From: Terry Lambert Message-Id: <199802272126.OAA24114@usr05.primenet.com> Subject: Re: syncer / SMP question To: jlemon@americantv.com (Jonathan Lemon) Date: Fri, 27 Feb 1998 21:26:43 +0000 (GMT) Cc: eivind@yes.no, fs@FreeBSD.ORG, jlemon@americantv.com In-Reply-To: <19980227135733.19894@right.PCS> from "Jonathan Lemon" at Feb 27, 98 01:57:33 pm X-Mailer: ELM [version 2.4 PL25] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org > IMHO, all the original code really needs is a nice little comment stating > that vfs_busy() releases the lock on the mountlist if it succeeds. Yes; for now that's what should be done, instead. I found the macros obfuscatory as well. My suggestion was a style change, if you'll remember... > I'll also note that it appears possible to pass a NULL third argument to > vfs_busy() and thus have it not release the lock on the mountlist. Then > simple_unlock would be called explicitly in the sync() function, also making > things clearer. I don't know if this would be correct. The way it's written now allows interleaved access to work. The mountlist isn't reordered, generally. You'd have to pretend, in your mind, that someone is rotoring through mounts of three FS's, two at a time, before you'd have a nice model of a situation where a conflict could occur; even then, I think you'd have to be at a specific place in walking the list while this was happening, since if you weren't where the links were actively bouncing, it's a non-problem. > I'm unsure if the lock release done by vfs_busy/lockmgr was for > 1) convenience, or 2) some form of atomic unlocking. Perhaps someone > else could explain? Terry? Poul? I can't explain. I didn't write that code, and I pretty much hate a lot of it. For me to be able to explain, I'd have to "play computer" and walk it looking for cycles given the suggested changes. This would be a lot easier for me with: o A call graph in hand o Header block comments on the function that I can sed out into my call graph so I can watch the lock states transiting I can get the first, but I have to do the second in my head; it's not really worth doing this to code that is relatively inoffensive compared to some of the more glaringly offensive code that's there for me to stare at instead (NFS, anyone?). Frankly, it's just as easy for someone else to "play computer" as it is for me, but I think that doing so in this case would be beside the point. If you fix the big problems, the small ones take care of themselves. A friend of mine likes to say this another way: "Obstacles are what you see when you take your eyes off your goals". Terry Lambert terry@lambert.org --- Any opinions in this posting are my own and not those of my present or previous employers. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 13:52:11 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id NAA06301 for freebsd-fs-outgoing; Fri, 27 Feb 1998 13:52:11 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from sumatra.americantv.com (sumatra.americantv.com [207.170.17.37]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id NAA06288 for ; Fri, 27 Feb 1998 13:52:06 -0800 (PST) (envelope-from jlemon@americantv.com) Received: from right.PCS (right.PCS [148.105.10.31]) by sumatra.americantv.com (8.8.5/8.8.5) with ESMTP id PAA23212; Fri, 27 Feb 1998 15:51:55 -0600 (CST) Received: (from jlemon@localhost) by right.PCS (8.6.13/8.6.4) id PAA09288; Fri, 27 Feb 1998 15:51:23 -0600 Message-ID: <19980227155123.46514@right.PCS> Date: Fri, 27 Feb 1998 15:51:23 -0600 From: Jonathan Lemon To: Terry Lambert Cc: eivind@yes.no, fs@FreeBSD.ORG Subject: Re: syncer / SMP question References: <19980227135733.19894@right.PCS> <199802272126.OAA24114@usr05.primenet.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.61.1 In-Reply-To: <199802272126.OAA24114@usr05.primenet.com>; from Terry Lambert on Feb 02, 1998 at 09:26:43PM +0000 Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Feb 02, 1998 at 09:26:43PM +0000, Terry Lambert wrote: > > I'll also note that it appears possible to pass a NULL third argument to > > vfs_busy() and thus have it not release the lock on the mountlist. Then > > simple_unlock would be called explicitly in the sync() function, also making > > things clearer. > > I don't know if this would be correct. The way it's written now allows > interleaved access to work. The mountlist isn't reordered, generally. I "played computer", as you put it, and realized that a NULL argument isn't possible, since the routine could call tsleep, and it releases/reaquires the lock around the sleep call. > I can get the first, but I have to do the second in my head; it's not > really worth doing this to code that is relatively inoffensive compared > to some of the more glaringly offensive code that's there for me to > stare at instead (NFS, anyone?). Little steps first, baby steps... that's it. :-) Some of us are still trying to understand the basics here. > If you fix the big problems, the small ones take care of themselves. > A friend of mine likes to say this another way: "Obstacles are what > you see when you take your eyes off your goals". And I think you tend to fall flat on your face when you don't watch where you are going. -- Jonathan To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 18:01:10 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id SAA21003 for freebsd-fs-outgoing; Fri, 27 Feb 1998 18:01:10 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from parkplace.cet.co.jp (parkplace.cet.co.jp [202.32.64.1]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id SAA20996 for ; Fri, 27 Feb 1998 18:01:08 -0800 (PST) (envelope-from michaelh@cet.co.jp) Received: from localhost (michaelh@localhost) by parkplace.cet.co.jp (8.8.8/CET-v2.2) with SMTP id CAA10158; Sat, 28 Feb 1998 02:00:07 GMT Date: Sat, 28 Feb 1998 11:00:07 +0900 (JST) From: Michael Hancock To: Jonathan Lemon cc: Terry Lambert , eivind@yes.no, fs@FreeBSD.ORG Subject: Re: syncer / SMP question In-Reply-To: <19980227155123.46514@right.PCS> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Fri, 27 Feb 1998, Jonathan Lemon wrote: > > If you fix the big problems, the small ones take care of themselves. > > A friend of mine likes to say this another way: "Obstacles are what > > you see when you take your eyes off your goals". > > And I think you tend to fall flat on your face when you don't > watch where you are going. Both are valid points, which is why it'sgood to have both kinds of people around. The problem is striking the right balance. Regards, Mike To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 20:56:10 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id UAA20167 for freebsd-fs-outgoing; Fri, 27 Feb 1998 20:56:10 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.19]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id UAA20151 for ; Fri, 27 Feb 1998 20:56:03 -0800 (PST) (envelope-from bde@godzilla.zeta.org.au) Received: (from bde@localhost) by godzilla.zeta.org.au (8.8.7/8.8.7) id PAA20951; Sat, 28 Feb 1998 15:54:10 +1100 Date: Sat, 28 Feb 1998 15:54:10 +1100 From: Bruce Evans Message-Id: <199802280454.PAA20951@godzilla.zeta.org.au> To: eivind@yes.no, jlemon@americantv.com Subject: Re: syncer / SMP question Cc: fs@FreeBSD.ORG Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org >> It appears to be released. The vfs_busy() routine makes a call to >> lockmgr(), which sets a lock on mountpoint and releases the lock on >> the mountlist. > >But... But... What about the fetch of the next-pointer? Looks like >a potential race-condition to me, but this includes some lock-calls I >don't really understand what do (interlocks? What are they?). Interlocks are for very short-term locks and for locking the aquisition of full locks. There is no race, because vfs_busy(), unlike lockmgr(), never releases the interlock in the LK_NOWAIT case. This non-release is surprising and is actually commented on at the beginning of vfs_busy() (bug: the comment says that the interlock is not released on failure, but it always temporarily released on failure in the !LK_NOWAIT case). Release of the interlock on success is normal and is not commented on. >Perhaps a WALK_MOUNTLIST macro would be better? I use >#define LIST_WALK_FORWARD(list, node, extra_node) \ > for ((node) = (list)->pHead; \ > (node) = (extra_node); \ > (extra_node) = (node)->pNext) No. Macros are never better :-). Here they would obfuscate the locking. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 21:13:45 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id VAA21355 for freebsd-fs-outgoing; Fri, 27 Feb 1998 21:13:45 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from dyson.iquest.net (dyson.iquest.net [198.70.144.127]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id VAA21345 for ; Fri, 27 Feb 1998 21:13:40 -0800 (PST) (envelope-from toor@dyson.iquest.net) Received: (from root@localhost) by dyson.iquest.net (8.8.8/8.8.8) id AAA00289; Sat, 28 Feb 1998 00:13:06 -0500 (EST) (envelope-from toor) From: "John S. Dyson" Message-Id: <199802280513.AAA00289@dyson.iquest.net> Subject: Re: syncer / SMP question In-Reply-To: <199802280454.PAA20951@godzilla.zeta.org.au> from Bruce Evans at "Feb 28, 98 03:54:10 pm" To: bde@zeta.org.au (Bruce Evans) Date: Sat, 28 Feb 1998 00:13:06 -0500 (EST) Cc: eivind@yes.no, jlemon@americantv.com, fs@FreeBSD.ORG X-Mailer: ELM [version 2.4ME+ PL32 (25)] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Bruce Evans said: > >> It appears to be released. The vfs_busy() routine makes a call to > >> lockmgr(), which sets a lock on mountpoint and releases the lock on > >> the mountlist. > > > >But... But... What about the fetch of the next-pointer? Looks like > >a potential race-condition to me, but this includes some lock-calls I > >don't really understand what do (interlocks? What are they?). > > Interlocks are for very short-term locks and for locking the aquisition > of full locks. > One rule-of-thumb is never to block (tsleep) when you have an interlock. -- John | Never try to teach a pig to sing, dyson@freebsd.org | it just makes you look stupid, jdyson@nc.com | and it irritates the pig. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 22:04:29 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id WAA26299 for freebsd-fs-outgoing; Fri, 27 Feb 1998 22:04:29 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from smtp03.primenet.com (smtp03.primenet.com [206.165.6.133]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id WAA26294 for ; Fri, 27 Feb 1998 22:04:26 -0800 (PST) (envelope-from tlambert@usr05.primenet.com) Received: (from daemon@localhost) by smtp03.primenet.com (8.8.8/8.8.8) id XAA09583; Fri, 27 Feb 1998 23:04:22 -0700 (MST) Received: from usr05.primenet.com(206.165.6.205) via SMTP by smtp03.primenet.com, id smtpd009558; Fri Feb 27 23:04:13 1998 Received: (from tlambert@localhost) by usr05.primenet.com (8.8.5/8.8.5) id XAA20809; Fri, 27 Feb 1998 23:04:10 -0700 (MST) From: Terry Lambert Message-Id: <199802280604.XAA20809@usr05.primenet.com> Subject: Re: syncer / SMP question To: toor@dyson.iquest.net (John S. Dyson) Date: Sat, 28 Feb 1998 06:04:10 +0000 (GMT) Cc: bde@zeta.org.au, eivind@yes.no, jlemon@americantv.com, fs@FreeBSD.ORG In-Reply-To: <199802280513.AAA00289@dyson.iquest.net> from "John S. Dyson" at Feb 28, 98 00:13:06 am X-Mailer: ELM [version 2.4 PL25] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org > > Interlocks are for very short-term locks and for locking the aquisition > > of full locks. > > > One rule-of-thumb is never to block (tsleep) when you have an interlock. This would make a good assert for a kernel compiled with debugging turned on... 8-). Terry Lambert terry@lambert.org --- Any opinions in this posting are my own and not those of my present or previous employers. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 22:19:56 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id WAA27891 for freebsd-fs-outgoing; Fri, 27 Feb 1998 22:19:56 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from dyson.iquest.net (dyson.iquest.net [198.70.144.127]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id WAA27886 for ; Fri, 27 Feb 1998 22:19:54 -0800 (PST) (envelope-from toor@dyson.iquest.net) Received: (from root@localhost) by dyson.iquest.net (8.8.8/8.8.8) id BAA03691; Sat, 28 Feb 1998 01:19:31 -0500 (EST) (envelope-from toor) Message-Id: <199802280619.BAA03691@dyson.iquest.net> Subject: Re: syncer / SMP question In-Reply-To: <199802280604.XAA20809@usr05.primenet.com> from Terry Lambert at "Feb 28, 98 06:04:10 am" To: tlambert@primenet.com (Terry Lambert) Date: Sat, 28 Feb 1998 01:19:31 -0500 (EST) Cc: bde@zeta.org.au, eivind@yes.no, jlemon@americantv.com, fs@FreeBSD.ORG From: "John S. Dyson" Reply-To: dyson@FreeBSD.ORG X-Mailer: ELM [version 2.4ME+ PL32 (25)] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Terry Lambert said: > > > Interlocks are for very short-term locks and for locking the aquisition > > > of full locks. > > > > > One rule-of-thumb is never to block (tsleep) when you have an interlock. > > This would make a good assert for a kernel compiled with debugging > turned on... 8-). > Yep, except, the low level lockmgr still has to, but that can be hidden. I can imagine the possibility of blocking, but that could greatly complicate things. The usage of locks has to be very disciplined until we can agree on a schema. The scheme that you outlined to me is very reasonable. -- John | Never try to teach a pig to sing, dyson@freebsd.org | it just makes you look stupid, jdyson@nc.com | and it irritates the pig. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Fri Feb 27 22:53:38 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id WAA02384 for freebsd-fs-outgoing; Fri, 27 Feb 1998 22:53:38 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from parkplace.cet.co.jp (parkplace.cet.co.jp [202.32.64.1]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id WAA02377; Fri, 27 Feb 1998 22:53:36 -0800 (PST) (envelope-from michaelh@cet.co.jp) Received: from localhost (michaelh@localhost) by parkplace.cet.co.jp (8.8.8/CET-v2.2) with SMTP id GAA11337; Sat, 28 Feb 1998 06:52:57 GMT Date: Sat, 28 Feb 1998 15:52:57 +0900 (JST) From: Michael Hancock To: freebsd-current@FreeBSD.ORG, freebsd-fs@FreeBSD.ORG Subject: VFS_VRELE: a couple more that I missed ... Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Index: fdesc_vfsops.c =================================================================== RCS file: /usr/cvs/src/sys/miscfs/fdesc/fdesc_vfsops.c,v retrieving revision 1.13 diff -u -r1.13 fdesc_vfsops.c --- fdesc_vfsops.c 1997/10/12 20:24:39 1.13 +++ fdesc_vfsops.c 1998/02/28 05:30:37 @@ -71,6 +71,7 @@ struct ucred *cred, struct proc *p)); static int fdesc_vget __P((struct mount *mp, ino_t ino, struct vnode **vpp)); +static int fdesc_vrele __P((struct mount *mp, struct vnode *vp)); static int fdesc_vptofh __P((struct vnode *vp, struct fid *fhp)); /* @@ -253,6 +254,8 @@ size_t, struct proc *)))eopnotsupp) #define fdesc_vget ((int (*) __P((struct mount *, ino_t, struct vnode **))) \ eopnotsupp) +#define fdesc_vrele ((int (*) __P((struct mount *, struct vnode *))) \ + eopnotsupp) #define fdesc_vptofh ((int (*) __P((struct vnode *, struct fid *)))eopnotsupp) static struct vfsops fdesc_vfsops = { @@ -264,6 +267,7 @@ fdesc_statfs, fdesc_sync, fdesc_vget, + fdesc_vrele, fdesc_fhtovp, fdesc_vptofh, fdesc_init, Index: portal_vfsops.c =================================================================== RCS file: /usr/cvs/src/sys/miscfs/portal/portal_vfsops.c,v retrieving revision 1.19 diff -u -r1.19 portal_vfsops.c --- portal_vfsops.c 1998/01/01 08:28:11 1.19 +++ portal_vfsops.c 1998/02/28 05:42:03 @@ -273,6 +273,8 @@ size_t, struct proc *)))eopnotsupp) #define portal_vget ((int (*) __P((struct mount *, ino_t, struct vnode **))) \ eopnotsupp) +#define portal_vrele ((int (*) __P((struct mount *, struct vnode *))) \ + eopnotsupp) #define portal_vptofh ((int (*) __P((struct vnode *, struct fid *)))eopnotsupp) static struct vfsops portal_vfsops = { @@ -284,6 +286,7 @@ portal_statfs, portal_sync, portal_vget, + portal_vrele, portal_fhtovp, portal_vptofh, portal_init, To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Sat Feb 28 01:33:48 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id BAA18098 for freebsd-fs-outgoing; Sat, 28 Feb 1998 01:33:48 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from ns1.yes.no (ns1.yes.no [195.119.24.10]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id BAA18087 for ; Sat, 28 Feb 1998 01:33:46 -0800 (PST) (envelope-from eivind@bitbox.follo.net) Received: from bitbox.follo.net (bitbox.follo.net [194.198.43.36]) by ns1.yes.no (8.8.7/8.8.7) with ESMTP id JAA22100; Sat, 28 Feb 1998 09:33:38 GMT Received: (from eivind@localhost) by bitbox.follo.net (8.8.6/8.8.6) id KAA06485; Sat, 28 Feb 1998 10:33:12 +0100 (MET) Message-ID: <19980228103312.01592@follo.net> Date: Sat, 28 Feb 1998 10:33:12 +0100 From: Eivind Eklund To: Bruce Evans , eivind@yes.no, jlemon@americantv.com Cc: fs@FreeBSD.ORG Subject: Re: syncer / SMP question References: <199802280454.PAA20951@godzilla.zeta.org.au> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Mailer: Mutt 0.89.1i In-Reply-To: <199802280454.PAA20951@godzilla.zeta.org.au>; from Bruce Evans on Sat, Feb 28, 1998 at 03:54:10PM +1100 Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Sat, Feb 28, 1998 at 03:54:10PM +1100, Bruce Evans wrote: > >> It appears to be released. The vfs_busy() routine makes a call to > >> lockmgr(), which sets a lock on mountpoint and releases the lock on > >> the mountlist. > > > >But... But... What about the fetch of the next-pointer? Looks like > >a potential race-condition to me, but this includes some lock-calls I > >don't really understand what do (interlocks? What are they?). > > Interlocks are for very short-term locks and for locking the aquisition > of full locks. Thanks. > There is no race, because vfs_busy(), unlike lockmgr(), never releases > the interlock in the LK_NOWAIT case. This non-release is surprising and > is actually commented on at the beginning of vfs_busy() (bug: the comment > says that the interlock is not released on failure, but it always > temporarily released on failure in the !LK_NOWAIT case). Release of the > interlock on success is normal and is not commented on. Are you saying the release of the mountlist lock on success normal? It looks very much abnormal to me; it is at the wrong layer. If this is normal, it means I'll have to remember 200 locking calls instead of 5, as any call that mess with the locks must be considered a locking call. Ouch. > >Perhaps a WALK_MOUNTLIST macro would be better? I use > >#define LIST_WALK_FORWARD(list, node, extra_node) \ > > for ((node) = (list)->pHead; \ > > (node) = (extra_node); \ > > (extra_node) = (node)->pNext) > > No. Macros are never better :-). Here they would obfuscate the locking. The macro above is for walking 'normal lists' (actually, tail-node terminated lists with the tail-node marked by having a NULL next-pointer). They don't contain locking. The other macro I showed would hide the locking, as you say. This is intentional, as it is part of abstracting the datastructure. The interface for manipulating the datastructure is assumed to take care of locking as necessary; 'obfuscating' the locking by hiding it from the client. IMO, macros are better if they can consistently provide a set of functionality. Due to C lacking late binding, anynoymous functions, and user-definable control-structures, we have to resort to textual trickery to reach the same goals ;-) Using a macro allow me to take a fairly complex piece of code that need to use user variables and centralize it. Afterwards, I don't have to read things that carefully; if I see that macro, I know exactly what code it is. Readability improved. Now, here is what sync() would look like if I had free reign (I've cut the comment at the end here, for readbility): int sync(p, uap) struct proc *p; struct sync_args *uap; { struct mount *mp; /* mountpoint - walked through the list */ struct mount *nmp; /* next-pointer (internal use in WALK_M_L) */ int asyncflag; WALK_MOUNT_LIST(&mountlist, mp, nmp, p) { if ((mp->mnt_flag & MNT_RDONLY) == 0) { asyncflag = mp->mnt_flag & MNT_ASYNC; mp->mnt_flag &= ~MNT_ASYNC; vfs_msync(mp, MNT_NOWAIT); VFS_SYNC(mp, MNT_NOWAIT, p != NULL ? p->p_ucred : NOCRED, p); mp->mnt_flag |= asyncflag; } } } This is about half the code of the previous version, and to me it is much clearer than before. Eivind. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message From owner-freebsd-fs Sat Feb 28 05:51:01 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id FAA10955 for freebsd-fs-outgoing; Sat, 28 Feb 1998 05:51:01 -0800 (PST) (envelope-from owner-freebsd-fs@FreeBSD.ORG) Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.19]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id FAA10941 for ; Sat, 28 Feb 1998 05:50:58 -0800 (PST) (envelope-from bde@godzilla.zeta.org.au) Received: (from bde@localhost) by godzilla.zeta.org.au (8.8.7/8.8.7) id AAA03879; Sun, 1 Mar 1998 00:49:43 +1100 Date: Sun, 1 Mar 1998 00:49:43 +1100 From: Bruce Evans Message-Id: <199802281349.AAA03879@godzilla.zeta.org.au> To: bde@zeta.org.au, eivind@yes.no, jlemon@americantv.com Subject: Re: syncer / SMP question Cc: fs@FreeBSD.ORG Sender: owner-freebsd-fs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org >Are you saying the release of the mountlist lock on success normal? Yes, whatever interlock is passed to lockmgr() is always released, and for vfs_busy(), that lock is mountlist_slock. >It looks very much abnormal to me; it is at the wrong layer. If this >is normal, it means I'll have to remember 200 locking calls instead of >5, as any call that mess with the locks must be considered a locking >call. Ouch. There aren't quite that many. vfs_busy() is apparently supposed to simplify one common case. >Now, here is what sync() would look like if I had free reign (I've cut >the comment at the end here, for readbility): > >int >sync(p, uap) > struct proc *p; > struct sync_args *uap; >{ > struct mount *mp; /* mountpoint - walked through the list */ > struct mount *nmp; /* next-pointer (internal use in WALK_M_L) */ > int asyncflag; > > WALK_MOUNT_LIST(&mountlist, mp, nmp, p) { > if ((mp->mnt_flag & MNT_RDONLY) == 0) { > asyncflag = mp->mnt_flag & MNT_ASYNC; > mp->mnt_flag &= ~MNT_ASYNC; > vfs_msync(mp, MNT_NOWAIT); > VFS_SYNC(mp, MNT_NOWAIT, > p != NULL ? p->p_ucred : NOCRED, p); > mp->mnt_flag |= asyncflag; > } > } >} > >This is about half the code of the previous version, and to me it is >much clearer than before. It seems to make one case simpler. How would you handle a return from inside the loop, like the one in getfsstat()? Requiring a single exit point is not an option :-). Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message