On Wed, Oct 26, 2022 at 4:54 PM Hanna Reitz <hre...@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.
> > - 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 <nsof...@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).
>
I'll change that. Do we have a good way to run checkpatch.pl when using
git-publish?
Maybe a way to run checkpatch.pl on all patches generated by git publish
automatically?
> > + 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?
Nir
