From owner-svn-src-all@FreeBSD.ORG Tue Nov 20 12:21:56 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 42D9025C; Tue, 20 Nov 2012 12:21:56 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 53E608FC08; Tue, 20 Nov 2012 12:21:55 +0000 (UTC) Received: by mail-lb0-f182.google.com with SMTP id go10so2988995lbb.13 for ; Tue, 20 Nov 2012 04:21:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=8jL/NDw86VkSf7BxO9C3s17WjO6QGRnj81Gp0RZQvjQ=; b=r9JyLrGlDLxhQRqhixI31QWNTkXl65ONYqLV+3jIk1qDpoaqPqcnN/F7yRou3AlPke Z8qTJikrE/R9LpZdbC9JRD6mlVfjnh2cFUYtd+N3jwCsX0PnBrbhHGIVbVtE5R/5xYS8 /JBrS9a26XrmJugGPphaAoGFlJoDf715qlbzAjQDBPmZd/0y+zxMIMB2gF/8VWu7+/xl DmdHLxmbMAH4t1OA5CtTpge5JWebtQI7rCcdHuV+e5evrsKvv3Y1HmbrWchIuufsiios tepQfQ7SVQeTTCgx8WvqXSj29QyKo+Zg3UfMQ1KfFCCiEtGlzPPBpOB0f9q4rMEtfupz LPew== MIME-Version: 1.0 Received: by 10.152.129.197 with SMTP id ny5mr14574259lab.43.1353414114087; Tue, 20 Nov 2012 04:21:54 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.112.134.5 with HTTP; Tue, 20 Nov 2012 04:21:54 -0800 (PST) In-Reply-To: References: <201211192243.qAJMhjFF055708@svn.freebsd.org> <20121120162708.I924@besplex.bde.org> <20121120230708.G6016@besplex.bde.org> Date: Tue, 20 Nov 2012 12:21:54 +0000 X-Google-Sender-Auth: KP6DZmwmE6X5rXanQtkcEIjqi6E Message-ID: Subject: Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs From: Attilio Rao To: Bruce Evans Content-Type: text/plain; charset=UTF-8 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Nov 2012 12:21:56 -0000 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. Attilio -- Peace can only be achieved by understanding - A. Einstein