From owner-svn-src-head@FreeBSD.ORG Tue Nov 20 13:07:48 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id C95C46C7; Tue, 20 Nov 2012 13:07:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 539898FC0C; Tue, 20 Nov 2012 13:07:47 +0000 (UTC) Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qAKD7c2u028790 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 21 Nov 2012 00:07:39 +1100 Date: Wed, 21 Nov 2012 00:07:38 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Attilio Rao Subject: Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs In-Reply-To: Message-ID: <20121120234822.J6178@besplex.bde.org> References: <201211192243.qAJMhjFF055708@svn.freebsd.org> <20121120162708.I924@besplex.bde.org> <20121120230708.G6016@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-Cloudmark-Score: 0 X-Optus-Cloudmark-Analysis: v=2.0 cv=XbrRV/F5 c=1 sm=1 a=m-t6nEIf1F8A:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=t5VDnOqDOncA:10 a=6I5d2MoRAAAA:8 a=wMkmLcVF5XD_M-86SkUA:9 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 a=kIvj0PshPjRlJRXl:21 a=aRJuftRBnZ_gBOK1:21 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Nov 2012 13:07:48 -0000 On Tue, 20 Nov 2012, Attilio Rao wrote: > On 11/20/12, Attilio Rao wrote: >> On 11/20/12, Bruce Evans wrote: >>> On Tue, 20 Nov 2012, Attilio Rao wrote: >>> >>>> On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans >>>> wrote: >>>>> On Mon, 19 Nov 2012, Attilio Rao wrote: >>>>> >>>>>> Log: >>>>>> r16312 is not any longer real since many years (likely since when VFS >>>>>> received granular locking) but the comment present in UFS has been >>>>>> copied all over other filesystems code incorrectly for several times. >>>>>> >>>>>> Removes comments that makes no sense now. >>>>> >>>>> >>>>> It still made sense (except for bitrot in the function name), but might >>>>> not >>>>> be true). The code made sense with it. Now the code makes no sense. >>>>> >>>>> >>>>>> Modified: head/sys/ufs/ffs/ffs_vfsops.c >>>>>> >>>>>> ============================================================================== >>>>>> --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 >>>>>> (r243310) >>>>>> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 >>>>>> (r243311) >>>>>> @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags >>>>>> ump = VFSTOUFS(mp); >>>>>> dev = ump->um_dev; >>>>>> fs = ump->um_fs; >>>>>> - >>>>>> - /* >>>>>> - * If this malloc() is performed after the getnewvnode() >>>>> >>>>> >>>>> This malloc() didn't match the code, which uses uma_zalloc(). Old >>>>> versions used MALLOC() in both the comment and the code. ffs's comment >>>>> was updated to say malloc() when the code was changed to use malloc(), >>>>> then rotted when the code was changed to use uma_zalloc(). In some >>>>> other file systems, the comment still said MALLOC(). >>>>> >>>>> >>>>>> - * it might block, leaving a vnode with a NULL v_data to be >>>>>> - * found by ffs_sync() if a sync happens to fire right then, >>>>>> - * which will cause a panic because ffs_sync() blindly >>>>>> - * dereferences vp->v_data (as well it should). >>>>>> - */ >>>>>> ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); >>>>>> >>>>>> /* Allocate a new vnode/inode. */ >>>>>> >>>>> >>>>> The code makes no sense now. The comment explains why ip is allocated >>>>> before vp, instead of in the natural, opposite order like it used to >>>>> be. Allocating things in an unnatural order requires extra code to >>>>> free ip when the allocation of vp fails. >>>> >>>> "Used to be" is very arguably. The code has been like its current form >>>> many more years than the opposite (16 against 3 I think). >>>> And the code makes perfectly sense if you know the history. So I don't >>>> agree with you. >>> >>> But it shouldn't be necessary to know the history of the code to >>> understand it. The code only makes sense if its comment is not removed, >>> or if you know the history of the code so that you can restore the >>> removed comment. However, if the comment makes no sense as you claim, >>> then the code that it it describes makes no sense. >> >> The "code that makes no sense" is basically the justification to have >> the allocation before the getnewvnode(). It makes no sense because the >> order makes no sense (you can allocate before or after getnewvnode(), >> you won't have v_data corruption as the comment claims). >> >> Hence the code makes no sense. > > Herm, s/code/comment. I think you are right that the comment makes no sense. A preemptible kernel may be preempted without it calling malloc() where it may block. Thus ffs_fsync() and anything else that looks at the inode must be doing something to avoid dereferencing v_data if the vnode is not fully constructed. This seems to be done by iterating over vnodes using MNT_VNODE_FOREACH_ACTIVE*() and not making incomplete vnodes active. ffs_fsync() still just blindly dereferences the inode. I think I am right that the code makes no sense. It is ordered like it is because placing the allocation of ip before the allocation of vp used to be enough to prevent v_data being dereferenced. This makes no sense when it isn't enough. Bruce