Re: [Qemu-devel] [PATCH] block: add native support for NFS

[Fam Zheng <famz@redhat.com>]

On 2013年12月17日 17:07, Peter Lieven wrote:
> On 17.12.2013 10:01, Fam Zheng wrote:
>> On 2013年12月17日 16:55, Peter Lieven wrote:
>>> On 17.12.2013 09:51, Fam Zheng wrote:
>>>> 于2013年 12月17日 星期二 16时46分50秒,Peter Lieven写到:
>>>>> On 17.12.2013 09:29, Fam Zheng wrote:
>>>>>> On 2013年12月17日 15:53, Peter Lieven wrote:
>>>>>>> Hi Fam,
>>>>>>>
>>>>>>> On 17.12.2013 05:07, Fam Zheng wrote:
>>>>>>>> On 2013年12月16日 23:34, Peter Lieven wrote:
>>>>>>>>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
>>>>>>>>> +                                        int64_t sector_num, int
>>>>>>>>> nb_sectors,
>>>>>>>>> + QEMUIOVector *iov)
>>>>>>>>> +{
>>>>>>>>> +    nfsclient *client = bs->opaque;
>>>>>>>>> +    struct NFSTask Task;
>>>>>>>>> +    char *buf = NULL;
>>>>>>>>> +
>>>>>>>>> +    nfs_co_init_task(client, &Task);
>>>>>>>>> +
>>>>>>>>> +    buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
>>>>>>>>> +    qemu_iovec_to_buf(iov, 0, buf, nb_sectors *
>>>>>>>>> BDRV_SECTOR_SIZE);
>>>>>>>>> +
>>>>>>>>> +    if (nfs_pwrite_async(client->context, client->fh,
>>>>>>>>> +                         sector_num * BDRV_SECTOR_SIZE,
>>>>>>>>> +                         nb_sectors * BDRV_SECTOR_SIZE,
>>>>>>>>> +                         buf, nfs_co_generic_cb, &Task) != 0) {
>>>>>>>>> +        g_free(buf);
>>>>>>>>> +        return -EIO;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    while (!Task.complete) {
>>>>>>>>> +        nfs_set_events(client);
>>>>>>>>> +        qemu_coroutine_yield();
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    g_free(buf);
>>>>>>>>> +
>>>>>>>>> +    if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
>>>>>>>>> +        return -EIO;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    bs->total_sectors = MAX(bs->total_sectors, sector_num +
>>>>>>>>> nb_sectors);
>>>>>>>>> +    client->allocated_file_size = -ENOTSUP;
>>>>>>>>
>>>>>>>> Why does allocated_file_size become not supported after a write?
>>>>>>> I thought that someone would ask this ;-) bdrv_allocated_file_size
>>>>>>> is only
>>>>>>> used in image info. I saved some code here implementing an async
>>>>>>> call.
>>>>>>> On open I fstat anyway and store that value. For qemu-img info
>>>>>>> this is
>>>>>>> sufficient, but the allocated size likely changes after a write.
>>>>>>> -ENOTSUP
>>>>>>> is the default if bdrv_allocated_file_size is not implemented.
>>>>>>
>>>>>> OK. Please add some comment here.
>>>>>>
>>>>>>>>> +static int nfs_file_open_common(BlockDriverState *bs, QDict
>>>>>>>>> *options, int flags,
>>>>>>>>> +                         int open_flags, Error **errp)
>>>>>>>>> +{
>>>>>>>>> +    nfsclient *client = bs->opaque;
>>>>>>>>> +    const char *filename;
>>>>>>>>> +    int ret = 0;
>>>>>>>>> +    QemuOpts *opts;
>>>>>>>>> +    Error *local_err = NULL;
>>>>>>>>> +    char *server = NULL, *path = NULL, *file = NULL, *strp;
>>>>>>>>> +    struct stat st;
>>>>>>>>> +
>>>>>>>>> +    opts = qemu_opts_create_nofail(&runtime_opts);
>>>>>>>>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>>>>>>>>> +    if (error_is_set(&local_err)) {
>>>>>>>>> +        qerror_report_err(local_err);
>>>>>>>>> +        error_free(local_err);
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    filename = qemu_opt_get(opts, "filename");
>>>>>>>>> +
>>>>>>>>> +    client->context = nfs_init_context();
>>>>>>>>> +
>>>>>>>>> +    if (client->context == NULL) {
>>>>>>>>> +        error_setg(errp, "Failed to init NFS context");
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    server = g_strdup(filename + 6);
>>>>>>>>
>>>>>>>> Please check the length of filename is longer than 6 before
>>>>>>>> accessing
>>>>>>>> filename[6].
>>>>>>> Good point. I will check for this, but in fact I think it can't
>>>>>>> happen
>>>>>>> because we will
>>>>>>> never end up there if filename does not start with nfs://
>>>>>>
>>>>>> True, at least for now, but it doesn't hurt to be defensive,
>>>>>> especially with strings.
>>>>>>
>>>>>>>>
>>>>>>>>> +    if (server[0] == '/' || server[0] == '\0') {
>>>>>>>>> +        error_setg(errp, "Invalid server in URL");
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +    strp = strchr(server, '/');
>>>>>>>>> +    if (strp == NULL) {
>>>>>>>>> +        error_setg(errp, "Invalid URL specified.\n");
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +    path = g_strdup(strp);
>>>>>>>>> +    *strp = 0;
>>>>>>>>> +    strp = strrchr(path, '/');
>>>>>>>>> +    if (strp == NULL) {
>>>>>>>>> +        error_setg(errp, "Invalid URL specified.\n");
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +    file = g_strdup(strp);
>>>>>>>>> +    *strp = 0;
>>>>>>>>> +
>>>>>>>>> +    if (nfs_mount(client->context, server, path) != 0) {
>>>>>>>>> +        error_setg(errp, "Failed to mount nfs share: %s",
>>>>>>>>> + nfs_get_error(client->context));
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    if (open_flags & O_CREAT) {
>>>>>>>>> +        if (nfs_creat(client->context, file, 0600, &client->fh)
>>>>>>>>> != 0) {
>>>>>>>>> +            error_setg(errp, "Failed to create file: %s",
>>>>>>>>> + nfs_get_error(client->context));
>>>>>>>>> +            ret = -EINVAL;
>>>>>>>>> +            goto fail;
>>>>>>>>> +        }
>>>>>>>>> +    } else {
>>>>>>>>> +        open_flags = (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
>>>>>>>>> +        if (nfs_open(client->context, file, open_flags,
>>>>>>>>> &client->fh)
>>>>>>>>> != 0) {
>>>>>>>>> +            error_setg(errp, "Failed to open file : %s",
>>>>>>>>> + nfs_get_error(client->context));
>>>>>>>>> +            ret = -EINVAL;
>>>>>>>>> +            goto fail;
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    if (nfs_fstat(client->context, client->fh, &st) != 0) {
>>>>>>>>> +        error_setg(errp, "Failed to fstat file: %s",
>>>>>>>>> + nfs_get_error(client->context));
>>>>>>>>> +        ret = -EIO;
>>>>>>>>> +        goto fail;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    bs->total_sectors = st.st_size / BDRV_SECTOR_SIZE;
>>>>>>>>
>>>>>>>> Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
>>>>>>>> couldn't be read.
>>>>>>> Will do. Can't it happen that we end up reading unallocated sectors?
>>>>>>
>>>>>> Hmm, maybe. Not missing last bytes of unaligned sector is essential
>>>>>> for VMDK description file. But you are right, please add check code
>>>>>> and make sure that we don't read beyond EOF as well.
>>>>> Actually it would like to keep bs->total_sectors = st.st_size /
>>>>> BDRV_SECTOR_SIZE; for now until I have checked how libnfs and the
>>>>> whole stack react on beyond EOF reads.
>>>>>
>>>>
>>>> You don't need to care about libnfs' EOF behavior, nfs_pread_async is
>>>> in bytes granularity, you could just read the last partial sector till
>>>> EOF and zero padding the buffer.
>>> isn't this something that is already in bdrv_co_do_readv?
>>>
>>> do you have an example of an VMDK with descriptor file for me. I would
>>> try the implementation then.
>>>
>>
>> You can create a VMDK with description file with:
>>
>>> $ qemu-img create -f vmdk -o subformat=monolithicFlat foo.vmdk 1G
>>> Formatting 'foo.vmdk', fmt=vmdk size=1073741824 compat6=off
>>> subformat='monolithicFlat' zeroed_grain=off
>>
>>> $ cat foo.vmdk
>>> # Disk DescriptorFile
>>> version=1
>>> CID=52b01245
>>> parentCID=ffffffff
>>> createType="monolithicFlat"
>>>
>>> # Extent description
>>> RW 2097152 FLAT "foo-flat.vmdk" 0
>>>
>>> # The Disk Data Base
>>> #DDB
>>>
>>> ddb.virtualHWVersion = "4"
>>> ddb.geometry.cylinders = "2080"
>>> ddb.geometry.heads = "16"
>>> ddb.geometry.sectors = "63"
>>> ddb.adapterType = "ide"
>>
>> It's a 313 bytes text file which I assume might not work with nfs
>> backend here. Please have a try.
>>
>> Thanks :)
> It works better than expected. Just creating does not work for obvious
> reasons, but if I have those files on NFS it works *g*
>
> image: nfs://172.21.200.61/vcore-dev-cdrom/foo.vmdk
> file format: vmdk
> virtual size: 1.0G (1073741824 bytes)
> disk size: 1.0G
> Format specific information:
>      cid: 1387271102
>      parent cid: 4294967295
>      create type: monolithicFlat
>      extents:
>          [0]:
>              virtual size: 1073741824
>              filename: nfs://172.21.200.61/vcore-dev-cdrom/foo-flat.vmdk
>              format: FLAT
>

Without DIV_ROUND_UP? That would be conditional on zero_beyond_eof. I 
tried this as well, it's not working if total_sectors is not rounded up:

> $ sudo ./qemu-img info nfs://127.0.0.1/stor/vmdk/foo.vmdk
> image: nfs://127.0.0.1/stor/vmdk/foo.vmdk
> file format: raw
> virtual size: 0 (0 bytes)
> disk size: 0

Fam