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

[Fam Zheng]

于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.

Fam