qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] Re: [RFC PATCH v2 3/3] block: add sheepdog driver for distr


From: MORITA Kazutaka
Subject: [Qemu-devel] Re: [RFC PATCH v2 3/3] block: add sheepdog driver for distributed storage support
Date: Mon, 17 May 2010 19:34:59 +0900
User-agent: Wanderlust/2.14.0 (Africa) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (Gojō) APEL/10.7 Emacs/22.2 (x86_64-pc-linux-gnu) MULE/5.0 (SAKAKI)

Hi,

Thank you very much for the reviewing!

At Fri, 14 May 2010 13:08:06 +0200,
Kevin Wolf wrote:

> > +
> > +struct sd_req {
> > +   uint8_t         proto_ver;
> > +   uint8_t         opcode;
> > +   uint16_t        flags;
> > +   uint32_t        epoch;
> > +   uint32_t        id;
> > +   uint32_t        data_length;
> > +   uint32_t        opcode_specific[8];
> > +};
> 
> CODING_STYLE says that structs should be typedefed and their names
> should be in CamelCase. So something like this:
> 
> typedef struct SheepdogReq {
>     ...
> } SheepdogReq;
> 
> (Or, if your prefer, SDReq; but with things like SDAIOCB I think it
> becomes hard to read)
> 

I see. I use Sheepdog as a prefix, like SheepdogReq.


> > +/*
> > +
> > +#undef eprintf
> > +#define eprintf(fmt, args...)                                              
> > \
> > +do {                                                                       
> > \
> > +   fprintf(stderr, "%s %d: " fmt, __func__, __LINE__, ##args);     \
> > +} while (0)
> 
> What about using error_report() instead of fprintf? Though it should be
> the same currently.
> 

Yes, using common helper functions is better.  I replaced all the
printf.


> > +
> > +   for (i = 0; i < ARRAY_SIZE(errors); ++i)
> > +           if (errors[i].err == err)
> > +                   return errors[i].desc;
> 
> CODING_STYLE requires braces here.
> 

I fixed all the missing braces.


> > +
> > +   return "Invalid error code";
> > +}
> > +
> > +static inline int before(uint32_t seq1, uint32_t seq2)
> > +{
> > +        return (int32_t)(seq1 - seq2) < 0;
> > +}
> > +
> > +static inline int after(uint32_t seq1, uint32_t seq2)
> > +{
> > +   return (int32_t)(seq2 - seq1) < 0;
> > +}
> 
> These functions look strange... Is the difference to seq1 < seq2 that
> the cast introduces intentional? (after(0x0, 0xabcdefff) == 1)
> 
> If yes, why is this useful? This needs a comment. If no, why even bother
> to have this function instead of directly using < or > ?
> 

These functions are used to compare sequential numbers which can be
wrap-around. For example, linux/net/tcp.h in the linux kernel.

Anyway, sheepdog doesn't use these functions, so I removed them.


> > +   if (snapid)
> > +           dprintf("%" PRIx32 " non current inode was open.\n", vid);
> > +   else
> > +           s->is_current = 1;
> > +
> > +   fd = connect_to_sdog(s->addr);
> 
> I wonder why you need to open another connection here instead of using
> s->fd. This pattern repeats at least in the snapshot functions, so I'm
> sure it's there for a reason. Maybe add a comment?
> 

We can use s->fd only for aio read/write operations.  It is because
the block driver may be during waiting response from the server, so we
cannot send other requests to the discriptor to avoid receiving wrong
data.

I added the comment to get_sheep_fd().


> > +
> > +           iov.iov_base = &s->inode;
> > +           iov.iov_len = sizeof(s->inode);
> > +           aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
> > +                                   data_len, offset, 0, 0, offset);
> > +           if (!aio_req) {
> > +                   eprintf("too many requests\n");
> > +                   acb->ret = -EIO;
> > +                   goto out;
> > +           }
> 
> Randomly failing requests is probably not a good idea. The guest might
> decide that the disk/file system is broken and stop using it. Can't you
> use a list like in AIOPool, so you can dynamically add new requests as
> needed?
> 

I agree.  In the v3 patch, AIO requests are allocated dynamically, and
all the requests are linked to the outstanding_aio_head in the
BDRVSheepdogState.


> > +
> > +static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> > +{
> > +   struct bdrv_sd_state *s = bs->opaque;
> > +   struct bdrv_sd_state *old_s;
> > +   char vdi[SD_MAX_VDI_LEN];
> > +   char *buf = NULL;
> > +   uint32_t vid;
> > +   uint32_t snapid = 0;
> > +   int ret = -ENOENT, fd;
> > +
> > +   old_s = qemu_malloc(sizeof(struct bdrv_sd_state));
> > +   if (!old_s) {
> 
> qemu_malloc never returns NULL.
> 

I removed all the NULL checks.


> > +
> > +BlockDriver bdrv_sheepdog = {
> > +   .format_name = "sheepdog",
> > +   .protocol_name = "sheepdog",
> > +   .instance_size = sizeof(struct bdrv_sd_state),
> > +   .bdrv_file_open = sd_open,
> > +   .bdrv_close = sd_close,
> > +   .bdrv_create = sd_create,
> > +
> > +   .bdrv_aio_readv = sd_aio_readv,
> > +   .bdrv_aio_writev = sd_aio_writev,
> > +
> > +   .bdrv_snapshot_create = sd_snapshot_create,
> > +   .bdrv_snapshot_goto = sd_snapshot_goto,
> > +   .bdrv_snapshot_delete = sd_snapshot_delete,
> > +   .bdrv_snapshot_list = sd_snapshot_list,
> > +
> > +   .bdrv_save_vmstate = sd_save_vmstate,
> > +   .bdrv_load_vmstate = sd_load_vmstate,
> > +
> > +   .create_options = sd_create_options,
> > +};
> 
> Please align the = to the same column, at least in each block.
> 

I have aligned in the v3 patch.


Thanks,

Kazutaka



reply via email to

[Prev in Thread] Current Thread [Next in Thread]