On 28.02.2012 15:35, Paolo Bonzini wrote:
> Il 28/02/2012 11:24, Michael Tokarev ha scritto:
>> This removes quite some duplicated code.
[]
>> +static int nbd_co_rwv(BlockDriverState *bs, int64_t sector_num,
>> + int nb_sectors, QEMUIOVector *qiov, int iswrite)
>
> Call this nbd_co_rw, and please pass the whole request.type down.
Originally it is readV and writeV, so why it should not be rwV ?
By passing whole request.type (NBD_CMD_WRITE or NBD_CMD_WRITE|NBD_CMD_FLAG_FUA
or NBD_CMD_READ) the condition (iswrite currently) will be larger (request.type
!= NBD_CMD_READ). Also, if someday we'll have additional flag for READ as we
already do for write, whole thing will be even more difficult to read.
>
>> {
>> BDRVNBDState *s = bs->opaque;
>> struct nbd_request request;
>> struct nbd_reply reply;
>> + int offset = 0;
>>
>> - request.type = NBD_CMD_WRITE;
>> - if (!bdrv_enable_write_cache(bs) && (s->nbdflags & NBD_FLAG_SEND_FUA)) {
>> + request.type = iswrite ? NBD_CMD_WRITE : NBD_CMD_READ;
>> + if (iswrite && !bdrv_enable_write_cache(bs) && (s->nbdflags &
>> NBD_FLAG_SEND_FUA)) {
>> request.type |= NBD_CMD_FLAG_FUA;
>> }
>> + reply.error = 0;
>> +
>> + /* we split the request into pieces of at most NBD_MAX_SECTORS size
>> + * and process them in a loop... */
>> + do {
>> + request.from = sector_num * 512;
>> + request.len = MIN(nb_sectors, NBD_MAX_SECTORS) * 512;
>> +
>> + nbd_coroutine_start(s, &request);
>> + if (nbd_co_send_request(s, &request, iswrite ? qiov->iov : NULL, 0)
>> == -1) {
>
> The last 0 needs to be offset.
Indeed, this is a bug. I think I tested it with large requests
but it looks like only for reads.
[]
> ... but thinking more about it, why don't you leave
> nbd_co_readv_1/nbd_co_writev_1 alone, and create a nbd_split_rw function
> that takes a function pointer?
Because each of these nbd_co_*_1 does the same thing, the diff. is
only quiv->iov vs NULL. While reading the original code it was the
first thing I did - consolidated nbd_co_*_1 into nbd_co_* ;)
Actually it might be a good idea to have single bdrv->bdrv_co_readwritev
method instead of two -- the path of each read and write jumps between
specific read-or-write routine and common readwrite routine several
times.
I see only one correction which needs (really!) to be done - that's
fixing the bug with offset. Do you still not agree?
Thanks,
/mjt
