[sheepdog] [PATCH v3 3/4] sheep: add SD_OP_SYNC_VDI and SD_OP_FLUSH_PEER for writeback cache semantics

[Hitoshi Mitake]

(2012/09/06 16:00), MORITA Kazutaka wrote:
> At Wed,  5 Sep 2012 23:31:04 +0900,
> Hitoshi Mitake wrote:
>> diff --git a/include/internal_proto.h b/include/internal_proto.h
>> index 5288823..06f74fa 100644
>> --- a/include/internal_proto.h
>> +++ b/include/internal_proto.h
>> @@ -65,6 +65,8 @@
>>   #define SD_OP_INFO_RECOVER 0xAA
>>   #define SD_OP_GET_VDI_COPIES 0xAB
>>   #define SD_OP_COMPLETE_RECOVERY 0xAC
>> +#define SD_OP_SYNC_VDI 0xAD
>> +#define SD_OP_FLUSH_PEER 0xAE
>
> Should be SD_OP_SYNC_PEER?
>
> If yes, rename default_flusht to default_sync, peer_flush to
> peer_sync, and so on.

I also think that SD_OP_SYNC_VDI and SD_OP_FLUSH_PEER are confusing names.
But, SD_OP_FLUSH_VDI is used for flushing cache in the protocol between 
qemu and sheepdog.
So I think naming them in either SD_OP_FLUSH_* or SD_OP_SYNC_* is suitable.
I'm thinking good names for them.

>
>
>> diff --git a/sheep/sheep.c b/sheep/sheep.c
>> index e1434cf..f960faf 100644
>> --- a/sheep/sheep.c
>> +++ b/sheep/sheep.c
>> @@ -49,9 +49,9 @@ static struct option const long_options[] = {
>>   	{"stdout", no_argument, NULL, 'o'},
>>   	{"port", required_argument, NULL, 'p'},
>>   	{"disk-space", required_argument, NULL, 's'},
>> -	{"enable-cache", required_argument, NULL, 'w'},
>>   	{"zone", required_argument, NULL, 'z'},
>>   	{"pidfile", required_argument, NULL, 'P'},
>> +	{"cache", required_argument, NULL, 'w'},
>>   	{NULL, 0, NULL, 0},
>>   };
>>
>> @@ -78,9 +78,9 @@ Options:\n\
>>     -p, --port              specify the TCP port on which to listen\n\
>>     -P, --pidfile           create a pid file\n\
>>     -s, --disk-space        specify the free disk space in megabytes\n\
>> -  -w, --enable-cache      enable object cache and specify the max size (M) and mode\n\
>>     -y, --myaddr            specify the address advertised to other sheep\n\
>>     -z, --zone              specify the zone id\n\
>> +  -w, --cache             specify the cache type\n\
>
> "-w, --write-cache" looks better to me.

I agree.

>
>
>> +static void object_cache_set(char *s)
>> +{
>> +	const char *header = "object:";
>> +	int len = strlen(header);
>> +	char *size, *p;
>> +	int64_t cache_size;
>> +
>> +	if (strncmp(s, header, len))
>> +		goto err;
>> +
>> +	size = s + len;
>> +	cache_size = strtol(size, &p, 10);
>> +	if (size == p || cache_size < 0 || UINT64_MAX < cache_size)
>> +		goto err;
>> +
>> +	sys->enable_write_cache = 1;
>> +	sys->cache_size = cache_size * 1024 * 1024;
>
> Should be renamed to 'enable_object_cache' and 'object_cache_size' to
> avoid confusion.
>
>> +
>> +	return;
>> +err:
>> +	fprintf(stderr, "Invalid object cache option '%s': "
>> +		"size must be an integer between 0 and %lu\n",
>> +		s, UINT64_MAX);
>> +	exit(1);
>> +}
>> +
>> +static void disk_cache_set(char *s)
>> +{
>> +	if (strcmp(s, "disk")) {
>> +		fprintf(stderr, "invalid disk cache option: %s\n", s);
>> +		exit(1);
>> +	}
>> +
>> +	sys->store_writeback = 1;
>
> disk_write_cache looks better to me.
>

How about specifying cache mode with bit vector?
e.g.
#define CACHE_OBJECT 0x1
#define CACHE_DISK   0x2

struct cluster_info {
  ...
  int cache_mode;
};

settig:
sys->cache_mode |= CACHE_OBJECT;

conditional branch:
if (sys->cache_mode & CACHE_OBJECT) {

# these can be wrapped with macro

This way will be easily expanded when new cache mode is added.

Thanks,
Hitoshi