Re: [PATCH 4/6] nilfs2: add ioctl() to clean snapshot flags from dat entries

[Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html