Date: Mon, 01 Jul 2013 14:17:00 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r252467 - head/sys/ufs/ffs Message-ID: <51D1D5AC.3000805@FreeBSD.org> In-Reply-To: <20130702031244.Q19968@besplex.bde.org> References: <201307011449.r61EnOjP043954@svn.freebsd.org> <20130702031244.Q19968@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?51D1D5AC.3000805>