Re: [Qemu-devel] [RFC PATCH 06/14] NBD client: connect to nbd server later

[Wen Congyang]

On 02/24/2015 05:31 AM, Max Reitz wrote:
> On 2015-02-11 at 22:07, Wen Congyang wrote:
>> The secondary qemu starts later than the primary qemu, so we
>> cannot connect to nbd server in bdrv_open().
>>
>> Signed-off-by: Wen Congyang <address@hidden>
>> Signed-off-by: zhanghailiang <address@hidden>
>> Signed-off-by: Gonglei <address@hidden>
>> ---
>>   block/nbd.c | 100 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 87 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index b05d1d0..19b9200 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -44,6 +44,8 @@
>>   typedef struct BDRVNBDState {
>>       NbdClientSession client;
>>       QemuOpts *socket_opts;
>> +    char *export;
>> +    bool connected;
>>   } BDRVNBDState;
>>     static int nbd_parse_uri(const char *filename, QDict *options)
>> @@ -247,20 +249,10 @@ static int nbd_establish_connection(BlockDriverState 
>> *bs, Error **errp)
>>       return sock;
>>   }
>>   -static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
>> -                    Error **errp)
>> +static int nbd_connect_server(BlockDriverState *bs, Error **errp)
>>   {
>>       BDRVNBDState *s = bs->opaque;
>> -    char *export = NULL;
>>       int result, sock;
>> -    Error *local_err = NULL;
>> -
>> -    /* Pop the config into our state object. Exit if invalid. */
>> -    nbd_config(s, options, &export, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        return -EINVAL;
>> -    }
>>         /* establish TCP connection, return error if it fails
>>        * TODO: Configurable retry-until-timeout behaviour.
>> @@ -271,16 +263,57 @@ static int nbd_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>       }
>>         /* NBD handshake */
>> -    result = nbd_client_session_init(&s->client, bs, sock, export, errp);
>> -    g_free(export);
>> +    result = nbd_client_session_init(&s->client, bs, sock, s->export, errp);
>> +    g_free(s->export);
>> +    s->export = NULL;
>> +    if (!result) {
>> +        s->connected = true;
>> +    }
>> +
>>       return result;
>>   }
>>   +static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
>> +                    Error **errp)
>> +{
>> +    BDRVNBDState *s = bs->opaque;
>> +    Error *local_err = NULL;
>> +
>> +    /* Pop the config into our state object. Exit if invalid. */
>> +    nbd_config(s, options, &s->export, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return nbd_connect_server(bs, errp);
>> +}
>> +
>> +static int nbd_open_colo(BlockDriverState *bs, QDict *options, int flags,
>> +                         Error **errp)
>> +{
>> +    BDRVNBDState *s = bs->opaque;
>> +    Error *local_err = NULL;
>> +
>> +    /* Pop the config into our state object. Exit if invalid. */
>> +    nbd_config(s, options, &s->export, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
>>                           int nb_sectors, QEMUIOVector *qiov)
>>   {
>>       BDRVNBDState *s = bs->opaque;
>>   +    if (!s->connected) {
>> +        return -EIO;
>> +    }
>> +
>>       return nbd_client_session_co_readv(&s->client, sector_num,
>>                                          nb_sectors, qiov);
>>   }
>> @@ -290,6 +323,10 @@ static int nbd_co_writev(BlockDriverState *bs, int64_t 
>> sector_num,
>>   {
>>       BDRVNBDState *s = bs->opaque;
>>   +    if (!s->connected) {
>> +        return 0;
>> +    }
> 
> Would it break anything to return -EIO here as well? (And in all the 
> following functions)

1. nbd_co_writev()
   If one child returns error, quorum will report it. There may be many write 
requests before
   we connect to nbd server, so there are too many qapi events...
2. nbd_co_flush()
   If quorum only have two children, and nbd client is the last one, 
quorum_co_flush()
   will return -EIO.
3. nbd_co_discard()
   quorum doens't call bdrv_co_discard(), so it is OK to return -EIO here.

So only nbd_co_discard() can return -EIO.

Thanks
Wen Congyang

> 
>> +
>>       return nbd_client_session_co_writev(&s->client, sector_num,
>>                                           nb_sectors, qiov);
>>   }
>> @@ -298,6 +335,10 @@ static int nbd_co_flush(BlockDriverState *bs)
>>   {
>>       BDRVNBDState *s = bs->opaque;
>>   +    if (!s->connected) {
>> +        return 0;
>> +    }
>> +
>>       return nbd_client_session_co_flush(&s->client);
>>   }
>>   @@ -312,6 +353,10 @@ static int nbd_co_discard(BlockDriverState *bs, 
>> int64_t sector_num,
>>   {
>>       BDRVNBDState *s = bs->opaque;
>>   +    if (!s->connected) {
>> +        return 0;
>> +    }
>> +
>>       return nbd_client_session_co_discard(&s->client, sector_num,
>>                                            nb_sectors);
>>   }
>> @@ -322,6 +367,7 @@ static void nbd_close(BlockDriverState *bs)
>>         qemu_opts_del(s->socket_opts);
>>       nbd_client_session_close(&s->client);
>> +    s->connected = false;
>>   }
>>   
>