[Sheepdog] [PATCH v2 2/3] sheep: timestamp the epoch log

[Liu Yuan]

On 10/26/2011 04:59 PM, MORITA Kazutaka wrote:

> At Wed, 26 Oct 2011 15:26:51 +0800,
> Liu Yuan wrote:
>>
>> On 10/26/2011 03:12 PM, MORITA Kazutaka wrote:
>>
>>>>> @@ -147,6 +147,7 @@ struct sheepdog_vnode_list_entry {
>>>>>  
>>>>>  struct epoch_log {
>>>>>  	uint64_t ctime;
>>>>> +	time_t time;
>>>>>  	uint32_t epoch;
>>>>>  	uint32_t nr_nodes;
>>>>>  	struct sheepdog_node_list_entry nodes[SD_MAX_NODES];
>>>
>>> This is not 64 bit aligned.
>>>
>>
>>
>> What do you mean by alignment? So where you suggest to place time in
>> this structure?
> 
> For all structures which can be shared between the different
> architecture, we should use architecture-independent types
> (e.g. uint64_t) and align them to 64 bits.  For example, if you have
> 64 bit machines for Sheepdog cluster and run 'collie cluster info' on
> the 32 bit machine, it will fail.
> 
> Actually, sizeof(struct epoch_log) is 24600 bytes on my 64 bit Linux
> machine, and 24596 bytes on 32 bit machine.
> 


Okay, I'll use uint64_t for time.

>>
>>>
>>>>> diff --git a/sheep/ops.c b/sheep/ops.c
>>>>> index 743eb0f..e2d7fb1 100644
>>>>> --- a/sheep/ops.c
>>>>> +++ b/sheep/ops.c
>>>>> @@ -279,6 +279,7 @@ static int local_stat_cluster(const struct sd_req *req, struct sd_rsp *rsp,
>>>>>  
>>>>>  		rsp->data_length += sizeof(*log);
>>>>>  		log->nr_nodes /= sizeof(log->nodes[0]);
>>>>> +		log->time = *(time_t *)(&log->nodes[log->nr_nodes]);
>>>
>>> Looks a bit hacky to me.  In addition, this doesn't work if
>>> sizeof(log->nodes[0]) < sizeof(time_t).
>>
>>
>>
>> Yes, tricky, but simplest. even sizeof(log->nodes[0]) < sizeof(time_t), I think this works out, cause
>>
>> we just need to get the address of end of the nodes array. Dereference
>> is okay since it has converted into the right type(time_t *).
> 
> If sizeof(log->nodes[0]) < sizeof(time_t),
> log->nr_nodes /= sizeof(log->nodes[0]) doesn't set the correct number
> of nodes.
> 
>>
>> any other more suitable means?
> 
> I think we should define the epoch log layout and include followings:
> 
>  - the number of nodes
>  - the array of node
>  - timestamp
> 
> 


ah, thanks for the explanation. you are right.

I'll cook a v3 patch.

Thanks,
Yuan