Re: [Qemu-devel] [PATCH] sheepdog: implement SD_OP_FLUSH_VDI operation

[Liu Yuan <namei.unix@gmail.com>]

On 04/03/2012 04:01 AM, Michael Tokarev wrote:

> On 02.04.2012 21:35, Liu Yuan wrote:
>> From: Liu Yuan <tailai.ly@taobao.com>
>>
>> Flush operation is supposed to flush the write-back cache of
>> sheepdog cluster.
>>
>> By issuing flush operation, we can assure the Guest of data
>> reaching the sheepdog cluster storage.
>>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Reviewd-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
>> Signed-off-by: Liu Yuan <tailai.ly@taobao.com>
>> ---
>>  block/sheepdog.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 50 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 00276f6f..c08c69b 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -32,9 +32,11 @@
>>  #define SD_OP_RELEASE_VDI    0x13
>>  #define SD_OP_GET_VDI_INFO   0x14
>>  #define SD_OP_READ_VDIS      0x15
>> +#define SD_OP_FLUSH_VDI      0x16
>>  
>>  #define SD_FLAG_CMD_WRITE    0x01
>>  #define SD_FLAG_CMD_COW      0x02
>> +#define SD_FLAG_CMD_CACHE    0x04
>>  
>>  #define SD_RES_SUCCESS       0x00 /* Success */
>>  #define SD_RES_UNKNOWN       0x01 /* Unknown error */
>> @@ -179,6 +181,8 @@ typedef struct SheepdogInode {
>>      uint32_t data_vdi_id[MAX_DATA_OBJS];
>>  } SheepdogInode;
>>  
>> +static int cache_enabled;
> 
> Why it is a global property instead of per-device
> (in BDRVSheepdogState) property?
> 
>> @@ -1011,6 +1024,10 @@ static int sd_open(BlockDriverState *bs, const char *filename, int flags)
>>      QLIST_INIT(&s->outstanding_aio_head);
>>      s->fd = -1;
>>  
>> +    if (flags & BDRV_O_CACHE_WB) {
>> +            cache_enabled = 1;
>> +    }
> 
> You test per-device flag here, and set a global variable, why?
> 




Hi Michael,
	Yes, I'd better embed this variable into BDRVSheepdogState.

>> +static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
>> +{
>> +    BDRVSheepdogState *s = bs->opaque;
>> +    SheepdogObjReq hdr = { 0 };
>> +    SheepdogObjRsp *rsp = (SheepdogObjRsp *)&hdr;
>> +    SheepdogInode *inode = &s->inode;
>> +    int fd, ret;
>> +    unsigned int wlen = 0, rlen = 0;
>> +
>> +    fd = connect_to_sdog(s->addr, s->port);
>> +    if (fd < 0) {
>> +        return -1;
>> +    }
> 
> Do you really need _another_ connection here?


I am unsure if I can use bs->fd or not, Kazum, would you please check
this usage? I tried use bs->fd in V2 and everything goes okay.

Thanks,
Yuan