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

[Max Reitz <mreitz@redhat.com>]

On 2015-02-24 at 21:23, Wen Congyang wrote:
> 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 <wency@cn.fujitsu.com>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>>    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.

Hm, okay. How about adding an option to quorum for ignoring errors from 
a specific child? It's probably not possible to do something like 
"children.1.ignore-errors=true", but maybe you can just ignore errors in 
quorum from any but the first child if the read pattern is set to 
"first", that would make sense to me.

But if you don't want to do that, I guess just making NBD some kind of 
/dev/null before it's connected should be fine.

Max

>>> +
>>>        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;
>>>    }
>>>