Re: [PATCH 3/3] qemu-img: Speed up checksum

[Hanna Reitz]

On 30.10.22 18:38, Nir Soffer wrote:
> On Wed, Oct 26, 2022 at 4:54 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 01.09.22 16:32, Nir Soffer wrote:
>     > Add coroutine based loop inspired by `qemu-img convert` design.
>     >
>     > Changes compared to `qemu-img convert`:
>     >
>     > - State for the entire image is kept in ImgChecksumState
>     >
>     > - State for single worker coroutine is kept in ImgChecksumworker.
>     >
>     > - "Writes" are always in-order, ensured using a queue.
>     >
>     > - Calling block status once per image extent, when the current
>     extent is
>     >    consumed by the workers.
>     >
>     > - Using 1m buffer size - testings shows that this gives best read
>     >    performance both with buffered and direct I/O.
>
>     Why does patch 1 then choose to use 2 MB?
>
>
> The first patch uses sync I/O, and in this case 2 MB is a little faster.

Interesting.

>     > - Number of coroutines is not configurable. Testing does not show
>     >    improvement when using more than 8 coroutines.
>     >
>     > - Progress include entire image, not only the allocated state.
>     >
>     > Comparing to the simple read loop shows that this version is up
>     to 4.67
>     > times faster when computing a checksum for an image full of
>     zeroes. For
>     > real images it is 1.59 times faster with direct I/O, and with
>     buffered
>     > I/O there is no difference.
>     >
>     > Test results on Dell PowerEdge R640 in a CentOS Stream 9 container:
>     >
>     > | image    | size | i/o       | before         | after         |
>     change |
>     >
>     |----------|------|-----------|----------------|----------------|--------|
>     > | zero [1] |   6g | buffered  | 1.600s ±0.014s | 0.342s ±0.016s
>     |  x4.67 |
>     > | zero     |   6g | direct    | 4.684s ±0.093s | 2.211s ±0.009s
>     |  x2.12 |
>     > | real [2] |   6g | buffered  | 1.841s ±0.075s | 1.806s ±0.036s
>     |  x1.02 |
>     > | real     |   6g | direct    | 3.094s ±0.079s | 1.947s ±0.017s
>     |  x1.59 |
>     > | nbd  [3] |   6g | buffered  | 2.455s ±0.183s | 1.808s ±0.016s
>     |  x1.36 |
>     > | nbd      |   6g | direct    | 3.540s ±0.020s | 1.749s ±0.018s
>     |  x2.02 |
>     >
>     > [1] raw image full of zeroes
>     > [2] raw fedora 35 image with additional random data, 50% full
>     > [3] image [2] exported by qemu-nbd via unix socket
>     >
>     > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
>     > ---
>     >   qemu-img.c | 343
>     +++++++++++++++++++++++++++++++++++++++++------------
>     >   1 file changed, 270 insertions(+), 73 deletions(-)
>
>     Looks good!
>
>     Just a couple of style comments below.
>
>     > diff --git a/qemu-img.c b/qemu-img.c
>     > index 7edcfe4bc8..bfa8e2862f 100644
>     > --- a/qemu-img.c
>     > +++ b/qemu-img.c
>     > @@ -1613,48 +1613,288 @@ out:
>     >       qemu_vfree(buf2);
>     >       blk_unref(blk2);
>     >   out2:
>     >       blk_unref(blk1);
>     >   out3:
>     >       qemu_progress_end();
>     >       return ret;
>     >   }
>     >
>     >   #ifdef CONFIG_BLKHASH
>     > +
>     > +#define CHECKSUM_COROUTINES 8
>     > +#define CHECKSUM_BUF_SIZE (1 * MiB)
>     > +#define CHECKSUM_ZERO_SIZE MIN(16 * GiB, SIZE_MAX)
>     > +
>     > +typedef struct ImgChecksumState ImgChecksumState;
>     > +
>     > +typedef struct ImgChecksumWorker {
>     > +    QTAILQ_ENTRY(ImgChecksumWorker) entry;
>     > +    ImgChecksumState *state;
>     > +    Coroutine *co;
>     > +    uint8_t *buf;
>     > +
>     > +    /* The current chunk. */
>     > +    int64_t offset;
>     > +    int64_t length;
>     > +    bool zero;
>     > +
>     > +    /* Always true for zero extent, false for data extent. Set
>     to true
>     > +     * when reading the chunk completes. */
>
>     Qemu codestyle requires /* and */ to be on separate lines for
>     multi-line
>     comments (see checkpatch.pl <http://checkpatch.pl>).
>
>
> I'll change that. Do we have a good way to run checkpatch.pl 
> <http://checkpatch.pl/> when using
> git-publish?
>
> Maybe a way to run checkpatch.pl <http://checkpatch.pl> on all patches 
> generated by git publish
> automatically?

I don’t know myself, I don’t use git-publish.  Stefan should know. ;)

>     > +    bool ready;
>     > +} ImgChecksumWorker;
>     > +
>     > +struct ImgChecksumState {
>     > +    const char *filename;
>     > +    BlockBackend *blk;
>     > +    BlockDriverState *bs;
>     > +    int64_t total_size;
>     > +
>     > +    /* Current extent, modified in checksum_co_next. */
>     > +    int64_t offset;
>     > +    int64_t length;
>     > +    bool zero;
>     > +
>     > +    int running_coroutines;
>     > +    CoMutex lock;
>     > +    ImgChecksumWorker workers[CHECKSUM_COROUTINES];
>     > +
>     > +    /* Ensure in-order updates. Update are scheduled at the
>     tail of the
>     > +     * queue and processed from the head of the queue when a
>     worker is
>     > +     * ready. */
>
>     Qemu codestyle requires /* and */ to be on separate lines for
>     multi-line
>     comments.
>
>     > +    QTAILQ_HEAD(, ImgChecksumWorker) update_queue;
>     > +
>     > +    struct blkhash *hash;
>     > +    int ret;
>     > +};
>
>     [...]
>
>     > +static coroutine_fn bool checksum_co_next(ImgChecksumWorker *w)
>     > +{
>     > +    ImgChecksumState *s = w->state;
>     > +
>     > +    qemu_co_mutex_lock(&s->lock);
>     > +
>     > +    if (s->offset == s->total_size || s->ret != -EINPROGRESS) {
>     > +        qemu_co_mutex_unlock(&s->lock);
>     > +        return false;
>     > +    }
>     > +
>     > +    if (s->length == 0 && checksum_block_status(s)) {
>
>     I’d prefer `checksum_block_status(s) < 0` so that it is clear that
>     negative values indicate errors.  Otherwise, based on this code
>     alone,
>     it looks to me more like `checksum_block_status()` returned a
>     boolean,
>     where `false` would generally indicate an error, which is confusing.
>
>
> Sure, will change.
>
>
>     Same in other places below.
>
>     > +        qemu_co_mutex_unlock(&s->lock);
>     > +        return false;
>     > +    }
>
>     [...]
>
>     > +/* Enter the next worker coroutine if the worker is ready. */
>     > +static void coroutine_fn
>     checksum_co_enter_next(ImgChecksumWorker *w)
>     > +{
>     > +    ImgChecksumState *s = w->state;
>     > +    ImgChecksumWorker *next;
>     > +
>     > +    if (!QTAILQ_EMPTY(&s->update_queue)) {
>     > +        next = QTAILQ_FIRST(&s->update_queue);
>     > +        if (next->ready)
>     > +            qemu_coroutine_enter(next->co);
>
>     Qemu codestyle requires braces here.
>
>
> Will add in next version.
>
> Before beautifying the first commit, should I squash this commit into it?
>
> The first commit was an attempt to do the simplest possible 
> implementation,
> but since this commit looks good (with some style issues), do we 
> really need the
> first one?

I wouldn’t mind squashing.  Your choice. :)

Hanna