From owner-svn-src-all@FreeBSD.ORG Mon Jul 1 18:45:27 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id A386454D; Mon, 1 Jul 2013 18:45:27 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 37E401B2E; Mon, 1 Jul 2013 18:45:27 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id 7725F1252D2; Tue, 2 Jul 2013 04:27:27 +1000 (EST) Date: Tue, 2 Jul 2013 04:27:25 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Pedro F. Giffuni" Subject: Re: svn commit: r252467 - head/sys/ufs/ffs In-Reply-To: <201307011449.r61EnOjP043954@svn.freebsd.org> Message-ID: <20130702031244.Q19968@besplex.bde.org> References: <201307011449.r61EnOjP043954@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=K8x6hFqI c=1 sm=1 a=pHRvX3JBB04A:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=Nw6HGA-A-RMA:10 a=deVyh3sjnY2ZYR7BRNkA:9 a=CjuIK1q_8ugA:10 a=h29Nqe6eIYPUJYRs:21 a=vF3kbaQWrGgMy4Ok:21 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 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 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: Mon, 01 Jul 2013 18:45:27 -0000 On Mon, 1 Jul 2013, Pedro F. Giffuni wrote: > Log: > Change i_gen in UFS to an unsigned type. > > Further simplify the i_gen calculation for older disks. > Having a zero here is not really a problem and this is more > similar to what is done in newfs_random(). This seems to be a minor problem. > Modified: head/sys/ufs/ffs/ffs_vfsops.c > ============================================================================== > --- head/sys/ufs/ffs/ffs_vfsops.c Mon Jul 1 14:45:03 2013 (r252466) > +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Jul 1 14:49:23 2013 (r252467) > @@ -1791,7 +1791,7 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags > * already have one. This should only happen on old filesystems. > */ The comment is wronger than before. This should (really shouldn't, but does) happen happens on new file systems under the following conditions: - if newfs initialized i_gen (other than by bzeroing it) and arc4random() returned 0 in newfs. We used to fix up this case. In the previous version, we failed to fix up this case when arc4random() returns UINT32_MAX. Now we fail to fix it up when arc4random() returns 0. The combined chance of failure is 1 in 2**64. - if newfs didn't initialize i_gen. We used to fix up this case. Now we fail to fix it up, similarly to the above, except the chance of failure when newfs didn't do the initialization is 1 in 2**32. > if (ip->i_gen == 0) { > - ip->i_gen = arc4random() + 1; > + ip->i_gen = arc4random(); > if ((vp->v_mount->mnt_flag & MNT_RDONLY) == 0) { > ip->i_flag |= IN_MODIFIED; > DIP_SET(ip, i_gen, ip->i_gen); When we fail to fix up i_gen, we keep using i_gen = 0 until the vnode is recycled. When the vnode is reused again after recycling it, we execute the above code again. Normally it doesn't fail again. So the inode gets a new generation number even though the inode wasn't reallocated. The MNT_RDONLY case gives a similar bug (when ip->i_gen needs fixing). Now i_gen is normally "fixed" to nonzero, but only for the in-core inode since the disk inode is read-only. After recycling, the inode again usually gets a new inode number although it wasn't reallocated. ffs_valloc() is more careful to never allocate i_gen = 0 (unless it was changed recently), but is/was still buggy. The old version was: % if (ip->i_gen == 0 || ++ip->i_gen == 0) % ip->i_gen = arc4random() / 2 + 1; It prefers to just add 1, without ever producing 0. When it wraps around to 0, it could advance to 1, but it uses arc4random(), apparently just to share code with the initially-zero case. It was careful to actually change i_gen in the wrapping case. Then i_gen is initially 0xFFFFFFFF; arc4random() might give that value again, so the value returned by arc4random() was divided by 2 to make it quite different (at most 0x7FFFFFFF); then 1 was added to keep it away from 0 (this also depends on dividing by 2). This had nothing to do with the type being signed -- arc4random() / 2 + 1 can still overflow the signed type with a probability of 1 in 2**31, and incrementing i_gen overflows with a probability of up to 1/2. I think ffs_valloc() still has the (i_gen == 0 || ++i_gen == 0) logic, but it doesn't work without the division and addition. Bruce