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

[MORITA Kazutaka]

At Thu, 06 Sep 2012 17:01:03 +0900,
Hitoshi Mitake wrote:
> 
> (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.

How about renaming SD_OP_SYNC_VDI to SD_OP_FLUSH_CLUSTER or
SD_OP_FLUSH_NODES?

> >
> >> +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.

Looks good to me.

Thanks,

Kazutaka