Re: [RFC patch v1 1/3] qemu-file: introduce current buffer

[Eric Blake]

On 4/13/20 6:12 AM, Denis Plotnikov wrote:
> To approach async wrtiting in the further commits, the buffer

writing

> allocated in QEMUFile struct is replaced with the link to the
> current buffer. We're going to use many buffers to write the
> qemu file stream to the unerlying storage asynchronously. The

underlying

> current buffer points out to the buffer is currently filled
> with data.

This sentence is confusing.  I'd suggest: The current_buf pointer tracks 
which buffer is currently filled with data.

> This patch doesn't add any features to qemu-file and doesn't
> change any qemu-file behavior.
>
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>   include/qemu/typedefs.h |   1 +
>   migration/qemu-file.c   | 156 +++++++++++++++++++++++++++++-------------------
>   2 files changed, 95 insertions(+), 62 deletions(-)
>
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 375770a..88dce54 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -97,6 +97,7 @@ typedef struct QDict QDict;
>   typedef struct QEMUBH QEMUBH;
>   typedef struct QemuConsole QemuConsole;
>   typedef struct QEMUFile QEMUFile;
> +typedef struct QEMUFileBuffer QEMUFileBuffer;
>   typedef struct QemuLockable QemuLockable;
>   typedef struct QemuMutex QemuMutex;
>   typedef struct QemuOpt QemuOpt;
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 1c3a358..285c6ef 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -33,6 +33,17 @@
>   #define IO_BUF_SIZE 32768
>   #define MAX_IOV_SIZE MIN(IOV_MAX, 64)
>   
> +QEMU_BUILD_BUG_ON(!QEMU_IS_ALIGNED(IO_BUF_SIZE, 512));
> +
> +struct QEMUFileBuffer {
> +    int buf_index;
> +    int buf_size; /* 0 when writing */
> +    uint8_t *buf;
> +    unsigned long *may_free;

Do we really want something that compiles differently on 32- vs. 64-bit?
/me reads ahead...

> +    struct iovec *iov;
> +    unsigned int iovcnt;
> +};
> +
>   struct QEMUFile {
>       const QEMUFileOps *ops;
>       const QEMUFileHooks *hooks;
> @@ -43,18 +54,12 @@ struct QEMUFile {
>   
>       int64_t pos; /* start of buffer when writing, end of buffer
>                       when reading */
> -    int buf_index;
> -    int buf_size; /* 0 when writing */
> -    uint8_t buf[IO_BUF_SIZE];
> -
> -    DECLARE_BITMAP(may_free, MAX_IOV_SIZE);

...ah, you're replacing a bitmap, and our bitmap code _does_ use 'long' 
as its core for optimum speed (which overcomes the fact that it does 
mean annoying differences on 32- vs. 64-bit).

> -    struct iovec iov[MAX_IOV_SIZE];
> -    unsigned int iovcnt;
> -
>       int last_error;
>       Error *last_error_obj;
>       /* has the file has been shutdown */
>       bool shutdown;
> +    /* currently used buffer */
> +    QEMUFileBuffer *current_buf;
>   };
>   

Most of the patch is mechanical conversion to the rearranged struct.

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org