From owner-svn-src-head@FreeBSD.ORG Mon Jul 1 19:17:05 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id A8896192 for ; Mon, 1 Jul 2013 19:17:05 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from nm7.bullet.mail.bf1.yahoo.com (nm7.bullet.mail.bf1.yahoo.com [98.139.212.166]) by mx1.freebsd.org (Postfix) with ESMTP id 60C8E1CDC for ; Mon, 1 Jul 2013 19:17:05 +0000 (UTC) Received: from [66.196.81.170] by nm7.bullet.mail.bf1.yahoo.com with NNFMP; 01 Jul 2013 19:17:02 -0000 Received: from [98.139.211.202] by tm16.bullet.mail.bf1.yahoo.com with NNFMP; 01 Jul 2013 19:17:02 -0000 Received: from [127.0.0.1] by smtp211.mail.bf1.yahoo.com with NNFMP; 01 Jul 2013 19:17:02 -0000 X-Yahoo-Newman-Id: 347875.32894.bm@smtp211.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: Sm2P37cVM1mbEaks4DIiew2.vw2Y9ogmVj2t_Mcnlil5Y6F A7gk6mwFssDNOYGw7rzyroaOxFycuv7FClBVXH1eKzmUEB7HYdHb17euKm6j KALxa.zBxq5nXmrQhLKSGw0UZCXV5FN7yBc5fV7CUmodgx981gUR5b5wi.xb usOdOmg_LZTeEwAKFprKuLvUME5Ezhfc7hpnqfLwMhQA4HecTNVuGKrk.RMU kQA7dqVEk4j.PiZ1UE8nI13sM.8wN4wjsh6plVW197aY45FTpj9x6T730U6q IoiqYeW_Vzjt9aXSYfCv6BdHhXvuSGaec0RhwKzEK91Iq9Vgn3vD7NmiVFfv QVwtscDpp3_UsbxzwVtT8rqhtF3RE0FfK0okvjS9TU_9SeqJektTDleavomL OcpefgIpYiQpKc1NYY9PE8Zm2dPnBY0.xlCbKF6RFMYQHcuajplsH6a0PnRm GI5sP2KRHgvtxaLxlxwADQicwB3PX_hItPXzLWawhhY8571i7JL1kH7YZqPg fPQ1csrh_6Rqpi8gIBXSMFHUDtsDK2Yt5XFqNdBfodT3AXg8ykVvg8A-- X-Yahoo-SMTP: xcjD0guswBAZaPPIbxpWwLcp9Unf X-Rocket-Received: from [192.168.0.102] (pfg@190.157.126.109 with ) by smtp211.mail.bf1.yahoo.com with SMTP; 01 Jul 2013 12:17:02 -0700 PDT Message-ID: <51D1D5AC.3000805@FreeBSD.org> Date: Mon, 01 Jul 2013 14:17:00 -0500 From: Pedro Giffuni Organization: FreeBSD User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 MIME-Version: 1.0 To: Bruce Evans Subject: Re: svn commit: r252467 - head/sys/ufs/ffs References: <201307011449.r61EnOjP043954@svn.freebsd.org> <20130702031244.Q19968@besplex.bde.org> In-Reply-To: <20130702031244.Q19968@besplex.bde.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org 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: Mon, 01 Jul 2013 19:17:05 -0000 Hi Bruce; El 01/07/2013 1:27 p. m., Bruce Evans escribió: > 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. > This can happen but the effect is, I think, negligible. > 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. > This is similar to the code that I changed originally. What I will do is revert that line to it's original state: on older disks it is unlikely that we will need the extra bit the original change is attempting to rescue. > I think ffs_valloc() still has the (i_gen == 0 || ++i_gen == 0) logic, > but it doesn't work without the division and addition. > I hadnt notice the use in ffs_valloc(). Lucky for me ;). Pedro.