[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] util/hbitmap: fix unaligned reset
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH] util/hbitmap: fix unaligned reset |
Date: |
Mon, 5 Aug 2019 11:56:10 +0200 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
Am 02.08.2019 um 23:19 hat Max Reitz geschrieben:
> On 02.08.19 20:58, Vladimir Sementsov-Ogievskiy wrote:
> > hbitmap_reset is broken: it rounds up the requested region. It leads to
> > the following bug, which is shown by fixed test:
> >
> > assume granularity = 2
> > set(0, 3) # count becomes 4
> > reset(0, 1) # count becomes 2
> >
> > But user of the interface assume that virtual bit 1 should be still
> > dirty, so hbitmap should report count to be 4!
> >
> > In other words, because of granularity, when we set one "virtual" bit,
> > yes, we make all "virtual" bits in same chunk to be dirty. But this
> > should not be so for reset.
> >
> > Fix this, aligning bound correctly.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >
> > Hi all!
> >
> > Hmm, is it a bug or feature? :)
> > I don't have a test for mirror yet, but I think that sync mirror may be
> > broken
> > because of this, as do_sync_target_write() seems to be using unaligned
> > reset.
>
> Crap.
>
>
> Yes, you’re right. This would fix it, and it wouldn’t fix it in the
> worst way.
>
> But I don’t know whether this patch is the best way forward still. I
> think call hbitmap_reset() with unaligned boundaries generally calls for
> trouble, as John has laid out. If mirror’s do_sync_target_write() is
> the only offender right now, I’d prefer for hbitmap_reset() to assert
> that the boundaries are aligned (for 4.2), and for
> do_sync_target_write() to be fixed (for 4.1? :-/).
>
> (A practical problem with this patch is that do_sync_target_write() will
> still do the write, but it won’t change anything in the bitmap, so the
> copy operation was effectively useless.)
>
> I don’t know how to fix mirror exactly, though. I have four ideas:
>
> (A) Quick fix 1: do_sync_target_write() should shrink [offset, offset +
> bytes) such that it is aligned. This would make it skip writes that
> don’t fill one whole chunk.
>
> +: Simple fix. Could go into 4.1.
> -: Makes copy-mode=write-blocking equal to copy-mode=background unless
> you set the granularity to like 512. (Still beats just being
> completely broken.)
write-blocking promises that the guest receives request completion only
when the request has also been written to the target. If you completely
skip the write, this promise is broken.
So I think you'd have to keep the write and only align the range for the
purpose of clearing bits in the dirty bitmap. This would result in some
duplicated I/O, which is an efficiency problem, but at least it
shouldn't come with a correctness problem.
Kevin
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] util/hbitmap: fix unaligned reset, Max Reitz, 2019/08/05