On 2014-03-18 08:10, Vyacheslav Dubeyko wrote:
> On Mon, 2014-03-17 at 14:49 +0100, Andreas Rohner wrote:
>
>>>
>>>> */
>>>> struct nilfs_vdesc {
>>>> __u64 vd_ino;
>>>> @@ -873,9 +873,55 @@ struct nilfs_vdesc {
>>>> __u64 vd_blocknr;
>>>> __u64 vd_offset;
>>>> __u32 vd_flags;
>>>> - __u32 vd_pad;
>>>> + /* vd_flags2 needed because of backwards compatibility */
>>>
>>> Completely, misunderstand comment. Usually, it keeps old fields for
>>> backward compatibility. But this flag is new.
>>
>> I will rewrite the comment. I need vd_flags2 because I can't use
>> vd_flags because of backwards compatibility.
>>
>>>> + __u32 vd_flags2;
>
> What about vd_blk_state instead of vd_flags2?
Yes sounds good to me.
>>>> };
>>>>
>>>> +/* vdesc flags */
>>>
>>> To be honest, I misunderstand why such number of flags and why namely
>>> such flags? Comments are really necessary.
>>>
>>>> +enum {
>>>> + NILFS_VDESC_DATA,
>>>> + NILFS_VDESC_NODE,
>>>> + /* ... */
>>>
>>> What does it mean?
>>
>> NILFS_VDESC_DATA = 0 and NILFS_VDESC_NODE = 1. This represents the type
>> of block. These two already existed, in the previous version, but they
>> were not explicit. See "[Patch 4/4] nilfs-utils: add extra flags to
>> nilfs_vdesc and update sui_nblocks":
>>
>> @@ -148,17 +149,19 @@ static int nilfs_acc_blocks_file(struct nilfs_file
>> *file,
>> - vdesc->vd_flags = 0; /* data */
>> + nilfs_vdesc_set_data(vdesc);
>> } else {
>> vdesc->vd_vblocknr =
>> le64_to_cpu(*(__le64 *)blk.b_binfo);
>> - vdesc->vd_flags = 1; /* node */
>> + nilfs_vdesc_set_node(vdesc);
>> }
>>
>>>> +};
>>>> +enum {
>>>> + NILFS_VDESC_SNAPSHOT,
>>>> + __NR_NILFS_VDESC_FIELDS,
>>>> + /* ... */
>>>
>>> What does it mean?
>
> I asked here about strange comment. What does it mean?
Sorry for the misunderstanding. I copied the comment from other flags like:
enum {
NILFS_SEGMENT_USAGE_ACTIVE,
NILFS_SEGMENT_USAGE_DIRTY,
NILFS_SEGMENT_USAGE_ERROR,
/* ... */
};
I guess it means "additional flags come here".
But you are right it is confusing it should be like that:
enum {
NILFS_VDESC_SNAPSHOT,
NILFS_VDESC_PROTECTION_PERIOD,
/* ... */
__NR_NILFS_VDESC_FIELDS,
};
> Moreover, I slightly confused by NILFS_VDESC_SNAPSHOT. Is it bit-based
> flag? I mean NILFS_VDESC_SNAPSHOT = (1 << 0). Or am I incorrect?
Yes NILFS_VDESC_SNAPSHOT and NILFS_VDESC_PROTECTION_PERIOD are
bit-based. NILFS_VDESC_DATA and NILFS_VDESC_NODE are not bit-based
because of backwards compatibility.
Please also note, that [PATCH 5/6] adds another flag, namely
NILFS_VDESC_PROTECTION_PERIOD.
Best regards,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
