From owner-svn-src-head@FreeBSD.ORG Tue Nov 20 12:17:57 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 6C555CD9; Tue, 20 Nov 2012 12:17:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id E8F538FC14; Tue, 20 Nov 2012 12:17:56 +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 mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qAKCHq5V024781 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 20 Nov 2012 23:17:54 +1100 Date: Tue, 20 Nov 2012 23:17:52 +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: <20121120230708.G6016@besplex.bde.org> References: <201211192243.qAJMhjFF055708@svn.freebsd.org> <20121120162708.I924@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=eJBLFwV1 c=1 sm=1 a=m-t6nEIf1F8A:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=t5VDnOqDOncA:10 a=lwsZ13Hph336hBIlgwAA:9 a=CjuIK1q_8ugA:10 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 12:17:57 -0000 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. I didn't point out before that the comment "/* Allocate a new vnode/inode. */" does less than echo the code, since the code obviously allocates a new vnode/inode and only the extent of the part which does that is unclear, and the comment is disorganized so as to make the scope unclear. Bruce