Hello,
> On Fri, 12 Apr 2013 16:39:55 +0800, Wang Shilong wrote:
> [...]
>>> + if (ritem && !is_v0) {
>>> + rinfo->cgen = btrfs_root_ctransid(ritem);
>>> + rinfo->ogen = btrfs_root_otransid(ritem);
>>> + rinfo->sgen = btrfs_root_stransid(ritem);
>>> + rinfo->rgen = btrfs_root_rtransid(ritem);
>>> + rinfo->ctime = btrfs_stack_timespec_sec(&ritem->ctime);
>>> + rinfo->otime = btrfs_stack_timespec_sec(&ritem->otime);
>>> + rinfo->stime = btrfs_stack_timespec_sec(&ritem->stime);
>>> + rinfo->rtime = btrfs_stack_timespec_sec(&ritem->rtime);
>>> + memcpy(rinfo->uuid, ritem->uuid, BTRFS_UUID_SIZE);
>>> + memcpy(rinfo->puuid, ritem->parent_uuid, BTRFS_UUID_SIZE);
>>> + memcpy(rinfo->ruuid, ritem->received_uuid, BTRFS_UUID_SIZE);
>>> + } else if (ritem && is_v0 && root_offset) {
>>> + /*
>>> + * old style (v0) root items don't contain an otransid field.
>>> + * But for snapshots, root_offset equals to its original
>>> + * generation.
>>> + */
>>> + rinfo->ogen = root_offset;
>>> + }
>>
>> We set it rinfo->ogen = root_offset only if:
>> 1> for root_item_v0
>> 2> it is a snapshot.
>>
>> Besides for a snapshot it's root_offset is always none zero.
>> so we do not need (is_v0 && root_offset) both.
>> Actually, Patch V2 doses the correct thing.
>>
>
> Patch V2 was accessing the otransid field also for root_item_v0 which
> does not have this field. This was not correct.
>
> That root_offset != 0 thing is because add_root() and therefore
> set_root_info() is called twice, once for BTRFS_ROOT_BACKREF_KEY and
> once for BTRFS_ROOT_ITEM_KEY. In both cases, the arguments to add_root()
> are only partially supplied and those values that are not available are
> set to zero. The old code everywhere had this ... != 0 else don't set
> the value, to handle this double call to add_root(), and I replaced most
> of it by passing a root_item pointer of NULL in the BACKREF case (where
> the old code just set gen=0, time=0, uuid=0 ...), and reading the values
> of the root_item down in set_root_info() in the ROOT_ITEM case. Only
> root_offset remains which is set to 0 in the BACKREF case and to the
> key's offset value in the ROOT_ITEM case. One could now argue that in
> the first case where root_offset is not valid, ritem is set to NULL and
> therefore the equation (ritem && is_v0 && root_offset) is equal to
> (ritem && is_v0), but IMHO a deep subfunction should not make use of too
> much information that is part of the functions that call the subfunction.
>
> Summary: Patch V3 does the correct thing.
>
After reading carefully, i agree patch V3 is correct~~, thanks
for so detailed illustration^_^
Thanks,
Wang
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
