QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] bitmaps branch rebase
@ 2019-08-07 21:45 John Snow
  2019-08-14 13:26 ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 4+ messages in thread
From: John Snow @ 2019-08-07 21:45 UTC (permalink / raw)
  To: Qemu-block; +Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz

FYI: I rebased jsnow/bitmaps on top of kwolf/block-next, itself based on
top of v4.1.0-rc4.

I'll post this along with the eventual pull request, but here's the
diffstat against the published patches:

011/33:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
016/33:[----] [-C] 'iotests: Add virtio-scsi device helper'
017/33:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
030/33:[0001] [FC] 'block/backup: teach TOP to never copy unallocated
regions'
032/33:[0018] [FC] 'iotests/257: test traditional sync modes'

11: A new hbitmap call was added upstream, changed to
bdrv_dirty_bitmap_next_zero.
16: Context-only (self.has_quit is new context in 040)
17: Removed 'auto' to follow upstream trends in iotest fashion
30: Remove ret = -ECANCELED as agreed on-list;
    Context changes for dirty_end patches
32: Fix capitalization in test, as mentioned on list.

I think the changes are actually fairly minimal and translate fairly
directly; let's review the rebase on-list in response to the PULL mails
when I send them.

Thanks,
--js


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] bitmaps branch rebase
  2019-08-07 21:45 [Qemu-devel] bitmaps branch rebase John Snow
@ 2019-08-14 13:26 ` Vladimir Sementsov-Ogievskiy
  2019-08-14 19:03   ` John Snow
  0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-14 13:26 UTC (permalink / raw)
  To: John Snow, Qemu-block; +Cc: qemu-devel, Max Reitz

08.08.2019 0:45, John Snow wrote:
> FYI: I rebased jsnow/bitmaps on top of kwolf/block-next, itself based on
> top of v4.1.0-rc4.
> 
> I'll post this along with the eventual pull request, but here's the
> diffstat against the published patches:
> 
> 011/33:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
> 016/33:[----] [-C] 'iotests: Add virtio-scsi device helper'
> 017/33:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
> 030/33:[0001] [FC] 'block/backup: teach TOP to never copy unallocated
> regions'
> 032/33:[0018] [FC] 'iotests/257: test traditional sync modes'
> 
> 11: A new hbitmap call was added upstream, changed to
> bdrv_dirty_bitmap_next_zero.
> 16: Context-only (self.has_quit is new context in 040)
> 17: Removed 'auto' to follow upstream trends in iotest fashion
> 30: Remove ret = -ECANCELED as agreed on-list;
>      Context changes for dirty_end patches
> 32: Fix capitalization in test, as mentioned on list.
> 
> I think the changes are actually fairly minimal and translate fairly
> directly; let's review the rebase on-list in response to the PULL mails
> when I send them.
> 


There is a bug in "block/backup: teach TOP to never copy unallocated regions":


 > @@ -256,6 +287,15 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
 >              continue; /* already copied */
 >          }
 >
 > +        if (job->initializing_bitmap) {
 > +            ret = backup_bitmap_reset_unallocated(job, start, &skip_bytes);
 > +            if (ret == 0) {
 > +                trace_backup_do_cow_skip_range(job, start, skip_bytes);
 > +                start += skip_bytes;
 > +                continue;
 > +            }

assume ret == 1, so we see skip_bytes of allocated bytes

 > +        }
 > +
 >          dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
 >                                                  (end - start));
 >          if (dirty_end < 0) {
 >

but then, we may copy more than skip_bytes, i.e. touch following possibly unallocated area.

===

Also, if want to fix it anyway, I think it's better to make additional while loop before this one
and reset all unallocated from start to end, otherwise we may call block_status for every cluster
on each loop iteration, even if the first call returns skip_bytes >= (end - start).

-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] bitmaps branch rebase
  2019-08-14 13:26 ` Vladimir Sementsov-Ogievskiy
@ 2019-08-14 19:03   ` John Snow
  2019-08-14 19:15     ` John Snow
  0 siblings, 1 reply; 4+ messages in thread
From: John Snow @ 2019-08-14 19:03 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, Qemu-block; +Cc: qemu-devel, Max Reitz



On 8/14/19 9:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.08.2019 0:45, John Snow wrote:
>> FYI: I rebased jsnow/bitmaps on top of kwolf/block-next, itself based on
>> top of v4.1.0-rc4.
>>
>> I'll post this along with the eventual pull request, but here's the
>> diffstat against the published patches:
>>
>> 011/33:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
>> 016/33:[----] [-C] 'iotests: Add virtio-scsi device helper'
>> 017/33:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
>> 030/33:[0001] [FC] 'block/backup: teach TOP to never copy unallocated
>> regions'
>> 032/33:[0018] [FC] 'iotests/257: test traditional sync modes'
>>
>> 11: A new hbitmap call was added upstream, changed to
>> bdrv_dirty_bitmap_next_zero.
>> 16: Context-only (self.has_quit is new context in 040)
>> 17: Removed 'auto' to follow upstream trends in iotest fashion
>> 30: Remove ret = -ECANCELED as agreed on-list;
>>      Context changes for dirty_end patches
>> 32: Fix capitalization in test, as mentioned on list.
>>
>> I think the changes are actually fairly minimal and translate fairly
>> directly; let's review the rebase on-list in response to the PULL mails
>> when I send them.
>>
> 
> 
> There is a bug in "block/backup: teach TOP to never copy unallocated regions":
> 
> 
>  > @@ -256,6 +287,15 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>  >              continue; /* already copied */
>  >          }
>  >
>  > +        if (job->initializing_bitmap) {
>  > +            ret = backup_bitmap_reset_unallocated(job, start, &skip_bytes);
>  > +            if (ret == 0) {
>  > +                trace_backup_do_cow_skip_range(job, start, skip_bytes);
>  > +                start += skip_bytes;
>  > +                continue;
>  > +            }
> 
> assume ret == 1, so we see skip_bytes of allocated bytes
> 
>  > +        }
>  > +
>  >          dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
>  >                                                  (end - start));
>  >          if (dirty_end < 0) {
>  >
> 
> but then, we may copy more than skip_bytes, i.e. touch following possibly unallocated area.
> 

Yes, Max pointed this out to me. He fixed it in his rebase. I will
probably send his fix as a patch, but then squash it in.

> ===
> 
> Also, if want to fix it anyway, I think it's better to make additional while loop before this one
> and reset all unallocated from start to end, otherwise we may call block_status for every cluster
> on each loop iteration, even if the first call returns skip_bytes >= (end - start).
> 

Are you worried about the case where backup_bitmap_reset_unallocated
returns as soon as it knows at least one cluster is unallocated, and
thus might re-query regions in the unaligned "tails"?

(That is, if it finds 0 - 92K unallocated, so it confirms [0-64K] as
unallocated, but then needs to spend time re-querying for [64-128K].)

If you'd like to optimize this, I'll invite you to, as a patch.

In practice I wonder if you're often going to run into the case where
block_status wants to return to you information segmented to less than
the cluster size such that you'd be spending a considerable portion of
time re-querying.

I suppose if the job size was e.g. 128K but the native qcow2 size was
64K and you had very perfectly fragmented allocations that you might see
the worst case for re-querying regions.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] bitmaps branch rebase
  2019-08-14 19:03   ` John Snow
@ 2019-08-14 19:15     ` John Snow
  0 siblings, 0 replies; 4+ messages in thread
From: John Snow @ 2019-08-14 19:15 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, Qemu-block; +Cc: qemu-devel, Max Reitz


On 8/14/19 3:03 PM, John Snow wrote:
> 
> If you'd like to optimize this, I'll invite you to, as a patch.
> 

Ah, looks like you're a step ahead of me :)

--js


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-07 21:45 [Qemu-devel] bitmaps branch rebase John Snow
2019-08-14 13:26 ` Vladimir Sementsov-Ogievskiy
2019-08-14 19:03   ` John Snow
2019-08-14 19:15     ` John Snow

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox