[Sheepdog] [RFC PATCH v2 3/3] block: add sheepdog driver for distributed storage support

[MORITA Kazutaka]

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