svn commit: r252467 - head/sys/ufs/ffs

[Pedro Giffuni]

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.