06.08.2019 16:30, John Snow wrote: > > > On 8/6/19 8:39 AM, Vladimir Sementsov-Ogievskiy wrote: >> 03.08.2019 0:19, Max Reitz wrote: >>> 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 <vsement...@virtuozzo.com> >>>> --- >>>> >>>> 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, >> >> Another thing is migration/block. Should we care of it, is it supported at >> all? >> > > Downstream products always have time and room to get additional fixes; I > think this is supported from an upstream POV so we should investigate this. > > I assume migration/block has the same problem that it fully clears > unaligned blocks? >
Hmm, after closer look, it seems like it's OK. It just a bit more difficult to see than in other places with reset. -- Best regards, Vladimir
