Re: [PATCH] ide: Get rid of IDEDrive struct

[Markus Armbruster]

Eduardo Habkost  writes:

> On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
>> On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
>> >
>> > The struct had a single field (IDEDevice dev), and is only used
>> > in the QOM type declarations and property lists.  We can simply
>> > use the IDEDevice struct directly instead.
>> >
>> > Signed-off-by: Eduardo Habkost 
>> > @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
>> > *data)
>> >  static const TypeInfo ide_hd_info = {
>> >  .name  = "ide-hd",
>> >  .parent= TYPE_IDE_DEVICE,
>> > -.instance_size = sizeof(IDEDrive),
>> >  .class_init= ide_hd_class_init,
>> >  };
>> 
>> This is one of those areas where this change works and reduces
>> amount of code, but on the other hand it means the QOM type
>> doesn't follow the common pattern for a leaf type of:
>>  * it has a struct
>>  * it has cast macros that cast to that struct
>>  * the typeinfo instance_size is the size of that struct
>> (it wasn't exactly following this pattern before, of course).
>
> Is this really a pattern that exists and we want to follow?
> I don't see why that pattern would be useful for simple leaf
> types.

I think the pattern exists, but we deviate from it in quite a few
places, probably just because it's so much boilerplate.

Related: Daniel's "[PATCH 0/4] qom: reduce boilerplate required for
declaring and defining objects".  Perhaps Daniel has an opinion on
taking shortcuts with leaf types.

> Also, in this case the code wasn't even following that pattern:
> it was using the same IDEDrive struct for all TYPE_IDE_DEVICE
> subtypes.

Rule of thumb: hw/ide/ is a bad example.  I don't mean to belittle the
efforts of quite a few people over the years.  It used to be worse.

>> We define in https://wiki.qemu.org/Documentation/QOMConventions
>> (in the 'When to create class types and macros' bit at the bottom)
>> what we expect for whether to provide class cast macros/a
>> class struct/class_size in the TypeInfo, essentially recommending
>> that types follow one of two patterns (simple leaf class with no
>> methods or class members, vs everything else) even if in a
>> particular case you could take a short-cut and not define
>> everything. We haven't really defined similar "this is the
>> standard pattern, provide it all even if you don't strictly
>> need it" rules for the instance struct/macros. Maybe we should?
>
> I think we should include the instance struct/macros in the
> recommendations there, but I would expect those recommendations
> to apply only to non-leaf types.

I'm fine with having a separate convention for leaf types if that helps,
but please let's have a convention.  I like my QOM boilerplate
uncreative.

>> Just a thought, not a nak; I know we have quite a number
>> of types that take this kind of "we don't really need to
>> provide all the standard QOM macros/structs/etc" approach
>> (some of which I wrote!).
>> 
>> thanks
>> -- PMM
>> 

Re: cleanups with long-term benefits

[Markus Armbruster]

Eduardo Habkost  writes:

> On Wed, Aug 05, 2020 at 06:23:23PM +0200, Kevin Wolf wrote:
>> We're basically weighing "git blame" against syntax highlighting
>> defaults. I don't think the latter has an obviously higher weight.
>
> I think "syntax highlight defaults" is far from being an accurate
> description of the motivations behind this discussion.

The motivation and the expected benefits are far from clear to me,
because all I have so far is a meandering e-mail thread.

For a change proposal as massive as "throw out working code and touch
basically every line in the QAPI schema", I'd like to see a concise memo
stating the goals, their benefits, the possible ways to get there, and
their cost.  I don't think that's too much to ask.

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Markus Armbruster]

John Snow  writes:

> On 8/5/20 3:36 AM, Markus Armbruster wrote:
>> John Snow  writes:
>>
>>> On 8/4/20 4:03 AM, Markus Armbruster wrote:
 The pain of tweaking the parser is likely dwarved several times over by
 the pain of the flag day.
>>>
>>> You mention this often; I wonder if I misunderstand the critique,
>>> because the pain of a "flag day" for a new file format seems
>>> negligible to me.
>>>
>>> I don't think we edit these .json files very often. Generally, we add
>>> a new command when we need one. The edits are usually one or two lines
>>> plus docstrings.
>>>
>>> If anyone has patches in-flight, I genuinely doubt it will take more
>>> than a few minutes to rewrite for the new file format.
>>>
>>> No?
>>
>> You describe the the flag day's one-time pain.
>>
>> There's also the longer term pain of having to work around git-blame
>> unable to see beyond the flag day.
>>
>
> So it's not really the "flag day" we're worried about anymore, it's
> the ongoing ease-of-use for vcs history.

Feel free to call that pain however you want.  I'm going to call it
"lasting aftereffects of the flag day" :)

>> I'm not claiming the pain is prohibitive (if I thought it was, I
>> would've tried to strange this thread in its crib), I am claiming it'll
>> be much more painful (read: expensive) than a parser tweak.
>>
>
> I do use `git blame` quite a lot, but with a project as old as QEMU,
> most of my trips through history do involve jumping across a few
> refactor gaps as a normal part of that process.
>
> As Dan points out, I often have to back out and add refactorSHA^ to my
> invocation, and just keep hopping backwards until I find what I am
> truly after. It just feels like a fact of programmer life for me at
> this point.

The fact that we all need to cope with this class of issue doesn't mean
we should create more instances unthinkingly.

We should only when we believe the benefits are worth it, and can't find
a cheaper way to get them.

We've discussed "is it really that bad" at some length.  What I'm
missing so far is a clear writeup of the benefits beyond "editor works
out of the box" (which is quite a desirable benefit, but can also be had
without a flag day).

> I've not used Paolo's invocation before, but it looks like it might be
> useful. I'll try to remember to try it out.

Re: [PATCH] ide: Get rid of IDEDrive struct

[Eduardo Habkost]

On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
> On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
> >
> > The struct had a single field (IDEDevice dev), and is only used
> > in the QOM type declarations and property lists.  We can simply
> > use the IDEDevice struct directly instead.
> >
> > Signed-off-by: Eduardo Habkost 
> > @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
> > *data)
> >  static const TypeInfo ide_hd_info = {
> >  .name  = "ide-hd",
> >  .parent= TYPE_IDE_DEVICE,
> > -.instance_size = sizeof(IDEDrive),
> >  .class_init= ide_hd_class_init,
> >  };
> 
> This is one of those areas where this change works and reduces
> amount of code, but on the other hand it means the QOM type
> doesn't follow the common pattern for a leaf type of:
>  * it has a struct
>  * it has cast macros that cast to that struct
>  * the typeinfo instance_size is the size of that struct
> (it wasn't exactly following this pattern before, of course).

Is this really a pattern that exists and we want to follow?
I don't see why that pattern would be useful for simple leaf
types.

Also, in this case the code wasn't even following that pattern:
it was using the same IDEDrive struct for all TYPE_IDE_DEVICE
subtypes.

> 
> We define in https://wiki.qemu.org/Documentation/QOMConventions
> (in the 'When to create class types and macros' bit at the bottom)
> what we expect for whether to provide class cast macros/a
> class struct/class_size in the TypeInfo, essentially recommending
> that types follow one of two patterns (simple leaf class with no
> methods or class members, vs everything else) even if in a
> particular case you could take a short-cut and not define
> everything. We haven't really defined similar "this is the
> standard pattern, provide it all even if you don't strictly
> need it" rules for the instance struct/macros. Maybe we should?

I think we should include the instance struct/macros in the
recommendations there, but I would expect those recommendations
to apply only to non-leaf types.

> 
> Just a thought, not a nak; I know we have quite a number
> of types that take this kind of "we don't really need to
> provide all the standard QOM macros/structs/etc" approach
> (some of which I wrote!).
> 
> thanks
> -- PMM
> 

-- 
Eduardo

[PATCH] block/vhdx: Support vhdx image only with 512 bytes logical sector size

[Swapnil Ingle]

block/vhdx uses qemu block layer where sector size is always 512 byte.
This may have issues  with 4K logical sector sized vhdx image.

For e.g qemu-img convert on such images fails with following assert:

$qemu-img convert -f vhdx -O raw 4KTest1.vhdx test.raw
qemu-img: util/iov.c:388: qiov_slice: Assertion `offset + len <=
qiov->size' failed.
Aborted

This patch adds an check to return ENOTSUP for vhdx images which
has logical sector size other than 512 bytes.

Signed-off-by: Swapnil Ingle 
---
 block/vhdx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 791eb90..356ec4c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -816,9 +816,9 @@ static int vhdx_parse_metadata(BlockDriverState *bs, 
BDRVVHDXState *s)
 goto exit;
 }
 
-/* only 2 supported sector sizes */
-if (s->logical_sector_size != 512 && s->logical_sector_size != 4096) {
-ret = -EINVAL;
+/* Currently we only support 512 */
+if (s->logical_sector_size != 512) {
+ret = -ENOTSUP;
 goto exit;
 }
 
-- 
1.8.3.1

Re: [PATCH] ide: Get rid of IDEDrive struct

[Peter Maydell]

On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost  wrote:
>
> The struct had a single field (IDEDevice dev), and is only used
> in the QOM type declarations and property lists.  We can simply
> use the IDEDevice struct directly instead.
>
> Signed-off-by: Eduardo Habkost 
> @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
> *data)
>  static const TypeInfo ide_hd_info = {
>  .name  = "ide-hd",
>  .parent= TYPE_IDE_DEVICE,
> -.instance_size = sizeof(IDEDrive),
>  .class_init= ide_hd_class_init,
>  };

This is one of those areas where this change works and reduces
amount of code, but on the other hand it means the QOM type
doesn't follow the common pattern for a leaf type of:
 * it has a struct
 * it has cast macros that cast to that struct
 * the typeinfo instance_size is the size of that struct
(it wasn't exactly following this pattern before, of course).

We define in https://wiki.qemu.org/Documentation/QOMConventions
(in the 'When to create class types and macros' bit at the bottom)
what we expect for whether to provide class cast macros/a
class struct/class_size in the TypeInfo, essentially recommending
that types follow one of two patterns (simple leaf class with no
methods or class members, vs everything else) even if in a
particular case you could take a short-cut and not define
everything. We haven't really defined similar "this is the
standard pattern, provide it all even if you don't strictly
need it" rules for the instance struct/macros. Maybe we should?

Just a thought, not a nak; I know we have quite a number
of types that take this kind of "we don't really need to
provide all the standard QOM macros/structs/etc" approach
(some of which I wrote!).

thanks
-- PMM

[PATCH] ide: Get rid of IDEDrive struct

[Eduardo Habkost]

The struct had a single field (IDEDevice dev), and is only used
in the QOM type declarations and property lists.  We can simply
use the IDEDevice struct directly instead.

Signed-off-by: Eduardo Habkost 
---
 hw/ide/qdev.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 27ff1f7f66..dd3867d8b3 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -157,10 +157,6 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
 
 /* - */
 
-typedef struct IDEDrive {
-IDEDevice dev;
-} IDEDrive;
-
 static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
 IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
@@ -297,19 +293,19 @@ static void ide_drive_realize(IDEDevice *dev, Error 
**errp)
 }
 
 #define DEFINE_IDE_DEV_PROPERTIES() \
-DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
-DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
-DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
-DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
-DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
-DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+DEFINE_BLOCK_PROPERTIES(IDEDevice, conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDevice, conf),  \
+DEFINE_PROP_STRING("ver",  IDEDevice, version),  \
+DEFINE_PROP_UINT64("wwn",  IDEDevice, wwn, 0),   \
+DEFINE_PROP_STRING("serial",  IDEDevice, serial),\
+DEFINE_PROP_STRING("model", IDEDevice, model)
 
 static Property ide_hd_properties[] = {
 DEFINE_IDE_DEV_PROPERTIES(),
-DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+DEFINE_BLOCK_CHS_PROPERTIES(IDEDevice, conf),
 DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
-IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
-DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
+IDEDevice, chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+DEFINE_PROP_UINT16("rotation_rate", IDEDevice, rotation_rate, 0),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo ide_hd_info = {
 .name  = "ide-hd",
 .parent= TYPE_IDE_DEVICE,
-.instance_size = sizeof(IDEDrive),
 .class_init= ide_hd_class_init,
 };
 
@@ -350,7 +345,6 @@ static void ide_cd_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo ide_cd_info = {
 .name  = "ide-cd",
 .parent= TYPE_IDE_DEVICE,
-.instance_size = sizeof(IDEDrive),
 .class_init= ide_cd_class_init,
 };
 
@@ -373,7 +367,6 @@ static void ide_drive_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo ide_drive_info = {
 .name  = "ide-drive",
 .parent= TYPE_IDE_DEVICE,
-.instance_size = sizeof(IDEDrive),
 .class_init= ide_drive_class_init,
 };
 
-- 
2.26.2

Re: [PATCH v12 11/11] iotests: dump QCOW2 header in JSON in #303

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Extend the test case #303 by dumping QCOW2 image metadata in JSON
format.

Signed-off-by: Andrey Shinkevich


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v12 10/11] qcow2_format.py: support dumping metadata in JSON format

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Implementation of dumping QCOW2 image metadata.
The sample output:
{
 "Header_extensions": [
 {
 "name": "Feature table",
 "magic": 1745090647,
 "length": 192,
 "data_str": ""
 },
 {
 "name": "Bitmaps",
 "magic": 595929205,
 "length": 24,
 "data": {
 "nb_bitmaps": 2,
 "reserved32": 0,
 "bitmap_directory_size": 64,
 "bitmap_directory_offset": 1048576,
 "bitmap_directory": [
 {
 "name": "bitmap-1",
 "bitmap_table_offset": 589824,
 "bitmap_table_size": 1,
 "flags": 2,
 "type": 1,
 "granularity_bits": 15,
 "name_size": 8,
 "extra_data_size": 0,
 "bitmap_table": [
 {
 "type": "serialized",
 "offset": 655360
 },
 ...

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 17 +
  1 file changed, 17 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index a4114cb..7487720 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -19,6 +19,15 @@
  
  import struct

  import string
+import json
+
+
+class ComplexEncoder(json.JSONEncoder):
+def default(self, obj):
+if hasattr(obj, 'to_dict'):
+return obj.to_dict()
+else:
+return json.JSONEncoder.default(self, obj)
  
  
  class Qcow2Field:

@@ -110,6 +119,10 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
   for i, field in enumerate(self.fields))
  
  def dump(self, is_json=False):

+if is_json:
+print(json.dumps(self.to_dict(), indent=4, cls=ComplexEncoder))
+return
+
  for f in self.fields:
  value = self.__dict__[f[2]]
  if isinstance(f[1], str):
@@ -440,6 +453,10 @@ class QcowHeader(Qcow2Struct):
  fd.write(buf)
  
  def dump_extensions(self, is_json=False):

+if is_json:
+print(json.dumps(self.extensions, indent=4, cls=ComplexEncoder))
+return
+
  for ex in self.extensions:
  print('Header extension:')
  ex.dump()



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: qemu-img convert asserts while converting from vhdx to raw

[Swapnil Ingle]

Hi Max,

Thanks for the response.

I checked internally and looks like it always fails.
Also in the code I see comment saying "We only support 512 currently"
at block/vhdx.c: vhdx_parse_metadata()

As you suggested we can just refuse to open images with 4K logical sector size, 
I will send an patch fixing this.

Thanks and Regards,
-Swapnil

On 29.07.20, 12:56, "Max Reitz"  wrote:

On 28.07.20 18:53, Swapnil Ingle wrote:
> Hey Guys,
> 
>  
> 
> We are seeing following assert when trying to convert disk image from
> vhdx to raw. 
> 
> This issue is seen only for disk with 4k logical sector size.

Honestly, looking at the vhdx code, it just can’t work for 4k logical
sectors without a major rework.  As far as I can see, for some reason it
likes to assume that qemu block layer sectors (which are always 512
bytes) are always the same as vhdx sectors (which aren’t).

Did you ever get any vhdx image with 4k logical sectors to work?

The problem I see is that we don’t have an active maintainer for vhdx,
so as unfortunate as it is, if it’s really never worked, the only
realistic solution I see would be to outright refuse to open images with
4k sectors...

Max

> $ qemu-img convert -f vhdx -O raw 4KTest1.vhdx test.raw
> 
> qemu-img: util/iov.c:388: qiov_slice: Assertion `offset + len <=
> qiov->size' failed.
> 
> Aborted
> 
>  
> 
> $ qemu-img --version
> 
> qemu-img version 5.0.91 (v5.1.0-rc1-2-g3cbc897-dirty)
> 
> Copyright (c) 2003-2020 Fabrice Bellard and the QEMU Project developers
> 
>  
> 
>  $ qemu-img check -r all 4KTest1.vhdx
> 
> No errors were found on the image.
> 
>  
> 
> $ qemu-img info 4KTest1.vhdx
> 
> image: 4KTest1.vhdx
> 
> file format: vhdx
> 
> virtual size: 10 GiB (10737418240 bytes)
> 
> disk size: 35.7 GiB
> 
> cluster_size: 33554432
> 
>  
> 
> The vhdx disk metadata is following, 
> 
>  
> 
> VhdFormat : VHDX
> 
> VhdType : Dynamic
> 
> LogicalSectorSize : 4096 
> 
> PhysicalSectorSize : 4096
> 
> BlockSize : 33554432
> 
>  
> 
> Following is the backtrace of the assert, 
> 
>  
> 
> #0  0x764cf387 in raise () from /lib64/libc.so.6
> 
> #1  0x764d0a78 in abort () from /lib64/libc.so.6
> 
> #2  0x764c81a6 in __assert_fail_base () from /lib64/libc.so.6
> 
> #3  0x764c8252 in __assert_fail () from /lib64/libc.so.6
> 
> #4  0x556abf5a in qiov_slice (qiov=0x74122a20, offset=0,
> len=2096640, head=0x74122648, tail=0x74122650,
> 
> niov=0x74122640) at util/iov.c:388
> 
> #5  0x556ac0f6 in qemu_iovec_init_extended (qiov=0x74122730,
> head_buf=0x0, head_len=0, mid_qiov=0x74122a20, mid_offset=0,
> 
> mid_len=2096640, tail_buf=0x0, tail_len=0) at util/iov.c:429
> 
> #6  0x556ac438 in qemu_iovec_init_slice (qiov=0x74122730,
> source=0x74122a20, offset=0, len=2096640) at util/iov.c:495
> 
> #7  0x55609bd6 in bdrv_driver_preadv (bs=0x55982a80,
> offset=15841886208, bytes=2096640, qiov=0x74122a20, qiov_offset=0,
> 
> flags=0) at block/io.c:1134
> 
> #8  0x5560ad55 in bdrv_aligned_preadv (child=0x559891f0,
> req=0x74122900, offset=15841886208, bytes=2096640, align=1,
> 
> qiov=0x74122a20, qiov_offset=0, flags=0) at block/io.c:1515
> 
> #9  0x5560b67b in bdrv_co_preadv_part (child=0x559891f0,
> offset=15841886208, bytes=2096640, qiov=0x74122a20, qiov_offset=0,
> 
> flags=0) at block/io.c:1756
> 
> #10 0x5560b4b4 in bdrv_co_preadv (child=0x559891f0,
> offset=15841886208, bytes=2096640, qiov=0x74122a20, flags=0)
> 
> at block/io.c:1714
> 
> #11 0x555e3266 in vhdx_co_readv (bs=0x5597b370,
> sector_num=4194304, nb_sectors=4095, qiov=0x74122e10) at
> block/vhdx.c:1208
> 
> #12 0x55609da1 in bdrv_driver_preadv (bs=0x5597b370,
> offset=2147483136, bytes=2097152, qiov=0x74122e10, qiov_offset=0,
> 
> flags=0) at block/io.c:1169
> 
> #13 0x5560ad55 in bdrv_aligned_preadv (child=0x55989150,
> req=0x74122cb0, offset=2147483136, bytes=2097152, align=512,
> 
> qiov=0x74122e10, qiov_offset=0, flags=0) at block/io.c:1515
> 
> #14 0x5560b67b in bdrv_co_preadv_part (child=0x55989150,
> offset=2147483136, bytes=2097152, qiov=0x74122e10, qiov_offset=0,
> 
> flags=0) at block/io.c:1756
> 
> #15 0x5560b4b4 in bdrv_co_preadv (child=0x55989150,
> offset=2147483136, bytes=2097152, qiov=0x74122e10, flags=0)
> 
   

Re: [PATCH v12 09/11] qcow2_format.py: collect fields to dump in JSON format

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

As __dict__ is being extended with class members we do not want to
print, add the to_dict() method to classes that returns a dictionary
with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 34 ++
  1 file changed, 34 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2000de3..a4114cb 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -119,6 +119,9 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
  
  print('{:<25} {}'.format(f[2], value_str))
  
+def to_dict(self):

+return dict((f[2], self.__dict__[f[2]]) for f in self.fields)
+
  
  class Qcow2BitmapExt(Qcow2Struct):
  
@@ -151,6 +154,11 @@ class Qcow2BitmapExt(Qcow2Struct):

  print()
  entry.dump()
  
+def to_dict(self):

+fields_dict = super().to_dict()
+fields_dict['bitmap_directory'] = self.bitmap_directory
+return fields_dict
+
  
  class Qcow2BitmapDirEntry(Qcow2Struct):
  
@@ -189,6 +197,14 @@ class Qcow2BitmapDirEntry(Qcow2Struct):

  super(Qcow2BitmapDirEntry, self).dump()
  self.bitmap_table.dump()
  
+def to_dict(self):

+fields_dict = super().to_dict()
+fields_dict['bitmap_table'] = self.bitmap_table.entries


the fact that we have to access internals of bitmap_table is not nice, but 
let's refactor it later.


+bmp_name = dict(name=self.name)
+# Put the name ahead of the dict


Aha. I don't think that ordering is necessary for json, but why not to order it 
a bit.


+bme_dict = {**bmp_name, **fields_dict}



strange to create bmp_name dict just to unpack it. You may as well do

bme_dict = {'name': self.name, **fields_dict}


+return bme_dict


bme_dict is extra variable: it's created just to return it, so, how about just

return {'name': self.name, **fields_dict}


or, maybe

return {
   'name': self.name,
   **super().to_dict(),
   'bitmap_table': self.bitmap_table.entries
   }


+
  
  class Qcow2BitmapTableEntry(Qcow2Struct):
  
@@ -214,6 +230,9 @@ class Qcow2BitmapTableEntry(Qcow2Struct):

  else:
  self.type = 'all-zeroes'
  
+def to_dict(self):

+return dict(type=self.type, offset=self.offset, reserved=self.reserved)
+


Python has a special syntax for creating dicts :), let's use:

return {'type': self.type, 'offset': self.offset, 'reserved': self.reserved}


  
  class Qcow2BitmapTable:
  
@@ -246,6 +265,9 @@ class QcowHeaderExtension(Qcow2Struct):

  0x44415441: 'Data file'
  }
  
+def to_dict(self):

+return self.mapping.get(self.value, "")


aha, so, actually, to_dict() returns not dict, but string. So it should better 
be named to_json() (and return any json-dumpable object, like string or dict)

and then, we can add to_json() method to Qcow2BitmapTable as well, to return 
list.



+
  fields = (
  ('u32', Magic, 'magic'),
  ('u32', '{}', 'length')
@@ -308,6 +330,18 @@ class QcowHeaderExtension(Qcow2Struct):
  else:
  self.obj.dump()
  
+def to_dict(self):

+fields_dict = super().to_dict()
+ext_name = dict(name=self.Magic(self.magic))


strange that we have to create Magic object here... Ok, let's refactor it later


+# Put the name ahead of the dict
+he_dict = {**ext_name, **fields_dict}
+if self.obj is not None:
+he_dict['data'] = self.obj
+else:
+he_dict['data_str'] = self.data_str
+
+return he_dict


again, let's avoid extra dict variables:

res = {'name': self.Magic(self.magic), **super().to_dict()}
if ...



+
  @classmethod
  def create(cls, magic, data):
  return QcowHeaderExtension(magic, len(data), data)




--
Best regards,
Vladimir

Re: cleanups with long-term benefits

[Eduardo Habkost]

On Wed, Aug 05, 2020 at 06:23:23PM +0200, Kevin Wolf wrote:
> Am 05.08.2020 um 12:08 hat Daniel P. Berrangé geschrieben:
> > On Wed, Aug 05, 2020 at 11:11:55AM +0200, Cornelia Huck wrote:
> > > On Wed, 5 Aug 2020 10:05:40 +0100
> > > Daniel P. Berrangé  wrote:
> > > 
> > > > On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> > > > > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:  
> > > > > >> Do you really use "git blame" that much?  "git log -S" does more 
> > > > > >> or less
> > > > > >> the same function (in a different way) and is not affected as much 
> > > > > >> by
> > > > > >> large code movement and transformation patches.  
> > > > > >
> > > > > > I use it a lot!   Following stuff back to find where a change came
> > > > > > from and then asking people.  
> > > > > 
> > > > > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > > > > just do "git log -p" and search for a relevant line of the code I'm
> > > > > "blaming".  
> > > > 
> > > > I used git blame alot too, but I don't think its a reason to not do the
> > > > cleanups. It is easy enough to just tell blame to use an earlier commit
> > > > if you see it displaying a refactor. I don't think such mild 
> > > > inconvenience
> > > > should stop us making otherwise desirable code changes
> > > 
> > > I don't think people argue that it should block changes; it it simply
> > > another thing to consider when weighing benefits vs. drawbacks.
> > 
> > Actually, I'm saying that including "git blame" in such a weighing is
> > the mechanism for blocking otherwise beneficial change to the code. Or
> > to put it another way, I believe the value assigned to "git blame" in
> > such a comparison is miniscule / effectively zero. The only time
> > "git blame" should win is if the change in question is purely the
> > bike shed colour and has no technical benefits at all.  If there's any
> > technical benefit to making the change it should always win.  We
> > shouldn't preserve technical debt in the code merely to avoid an impact
> > on "git blame".
> 
> We're basically weighing "git blame" against syntax highlighting
> defaults. I don't think the latter has an obviously higher weight.

I think "syntax highlight defaults" is far from being an accurate
description of the motivations behind this discussion.

-- 
Eduardo

Re: [PATCH v2 3/3] aio-posix: keep aio_notify_me disabled during polling

[Paolo Bonzini]

On 05/08/20 12:00, Stefan Hajnoczi wrote:
> +
> +/*
> + * aio_notify can avoid the expensive event_notifier_set if
> + * everything (file descriptors, bottom halves, timers) will
> + * be re-evaluated before the next blocking poll().  This is
> + * already true when aio_poll is called with blocking == false;
> + * if blocking == true, it is only true after poll() returns,
> + * so disable the optimization now.
> + */
> +if (use_notify_me) {
> +atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
> +/*
> + * Write ctx->notify_me before reading ctx->notified.  Pairs with
> + * smp_mb in aio_notify().
> + */
> +smp_mb();
> +
> +/* Don't block if aio_notify() was called */
> +if (atomic_read(&ctx->notified)) {
> +timeout = 0;
> +}

Aha, this is the trick: "timeout = 0" also applies if a timer was moved 
early.  In this case you uselessly keep notify_me set for a bit, but 
it's okay. Nice!

The code can be simplified a bit more, since the use_notify_me variable 
is just "timeout":

use_notify_me = (timeout != 0);
if (use_notify_me) {
 /*
  * aio_notify can avoid the expensive event_notifier_set if
  * everything (file descriptors, bottom halves, timers) will
  * be re-evaluated before the next blocking poll().  This is
  * already true when aio_poll is called with blocking == false;
  * if blocking == true, it is only true after poll() returns,
  * so disable the optimization now.
  */
 atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
 /*
  * Write ctx->notify_me before reading ctx->notified.  Pairs with
  * smp_mb in aio_notify().
  */
 smp_mb();
 
 /* Don't block if aio_notify() was called */
 if (atomic_read(&ctx->notified)) {
 timeout = 0;
 }
 }
 if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
 ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
 }
 if (use_notify_me) {
 /* Finish the poll before clearing the flag.  */
 atomic_store_release(&ctx->notify_me,
  atomic_read(&ctx->notify_me) - 2);
 }

Paolo

Re: [PATCH v2 2/3] async: always set ctx->notified in aio_notify()

[Paolo Bonzini]

On 05/08/20 12:00, Stefan Hajnoczi wrote:
> aio_notify() does not set ctx->notified when called with
> ctx->aio_notify_me disabled. Therefore aio_notify_me needs to be enabled
> during polling.
> 
> This is suboptimal since expensive event_notifier_set(&ctx->notifier)
> and event_notifier_test_and_clear(&ctx->notifier) calls are required
> when ctx->aio_notify_me is enabled.
> 
> Change aio_notify() so that aio->notified is always set, regardless of
> ctx->aio_notify_me. This will make polling cheaper since
> ctx->aio_notify_me can remain disabled. Move the
> event_notifier_test_and_clear() to the fd handler function (which is now
> no longer an empty function so "dummy" has been dropped from its name).
> 
> The next patch takes advantage of this by optimizing polling in
> util/aio-posix.c.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  util/async.c | 32 +---
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/util/async.c b/util/async.c
> index d9f987e133..3ec3e8d135 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx)
>  
>  void aio_notify(AioContext *ctx)
>  {
> -/* Write e.g. bh->scheduled before reading ctx->notify_me.  Pairs
> +/*
> + * Write e.g. bh->flags before writing ctx->notified.  Pairs with smp_mb 
> in
> + * aio_notify_accept.
> + */
> +smp_wmb();
> +atomic_set(&ctx->notified, true);
> +
> +/*
> + * Write ctx->notified before reading ctx->notify_me.  Pairs
>   * with smp_mb in aio_ctx_prepare or aio_poll.
>   */
>  smp_mb();
>  if (atomic_read(&ctx->notify_me)) {
>  event_notifier_set(&ctx->notifier);
> -atomic_mb_set(&ctx->notified, true);
>  }
>  }
>  
>  void aio_notify_accept(AioContext *ctx)
>  {
> -if (atomic_xchg(&ctx->notified, false)
> -#ifdef WIN32
> -|| true
> -#endif
> -) {
> -event_notifier_test_and_clear(&ctx->notifier);
> -}
> +atomic_set(&ctx->notified, false);
> +
> +/*
> + * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb 
> in
> + * aio_notify.

Just a nit: it's an smp_wmb().  (It's okay for a wmb to pair with
anything stronger than a smp_rmb()).

> + */
> +smp_mb();
>  }
>  
>  static void aio_timerlist_notify(void *opaque, QEMUClockType type)
> @@ -445,8 +452,11 @@ static void aio_timerlist_notify(void *opaque, 
> QEMUClockType type)
>  aio_notify(opaque);
>  }
>  
> -static void aio_context_notifier_dummy_cb(EventNotifier *e)
> +static void aio_context_notifier_cb(EventNotifier *e)
>  {
> +AioContext *ctx = container_of(e, AioContext, notifier);
> +
> +event_notifier_test_and_clear(&ctx->notifier);
>  }
>  
>  /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
> @@ -508,7 +518,7 @@ AioContext *aio_context_new(Error **errp)
>  
>  aio_set_event_notifier(ctx, &ctx->notifier,
> false,
> -   aio_context_notifier_dummy_cb,
> +   aio_context_notifier_cb,
> aio_context_notifier_poll);
>  #ifdef CONFIG_LINUX_AIO
>  ctx->linux_aio = NULL;
> 

Reviewed-by: Paolo Bonzini 

Much better, you can almost trust the code to do the right thing. :)

Paolo

Re: cleanups with long-term benefits

[Kevin Wolf]

Am 05.08.2020 um 12:08 hat Daniel P. Berrangé geschrieben:
> On Wed, Aug 05, 2020 at 11:11:55AM +0200, Cornelia Huck wrote:
> > On Wed, 5 Aug 2020 10:05:40 +0100
> > Daniel P. Berrangé  wrote:
> > 
> > > On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> > > > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:  
> > > > >> Do you really use "git blame" that much?  "git log -S" does more or 
> > > > >> less
> > > > >> the same function (in a different way) and is not affected as much by
> > > > >> large code movement and transformation patches.  
> > > > >
> > > > > I use it a lot!   Following stuff back to find where a change came
> > > > > from and then asking people.  
> > > > 
> > > > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > > > just do "git log -p" and search for a relevant line of the code I'm
> > > > "blaming".  
> > > 
> > > I used git blame alot too, but I don't think its a reason to not do the
> > > cleanups. It is easy enough to just tell blame to use an earlier commit
> > > if you see it displaying a refactor. I don't think such mild inconvenience
> > > should stop us making otherwise desirable code changes
> > 
> > I don't think people argue that it should block changes; it it simply
> > another thing to consider when weighing benefits vs. drawbacks.
> 
> Actually, I'm saying that including "git blame" in such a weighing is
> the mechanism for blocking otherwise beneficial change to the code. Or
> to put it another way, I believe the value assigned to "git blame" in
> such a comparison is miniscule / effectively zero. The only time
> "git blame" should win is if the change in question is purely the
> bike shed colour and has no technical benefits at all.  If there's any
> technical benefit to making the change it should always win.  We
> shouldn't preserve technical debt in the code merely to avoid an impact
> on "git blame".

We're basically weighing "git blame" against syntax highlighting
defaults. I don't think the latter has an obviously higher weight.

Kevin

Re: [PATCH v12 08/11] qcow2.py: Introduce '-j' key to dump in JSON format

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Add the command key to the qcow2.py arguments list to dump QCOW2
metadata in JSON format. Here is the suggested way to do that. The
implementation of the dump in JSON format is in the patch that follows.

Signed-off-by: Andrey Shinkevich


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[John Snow]

On 8/5/20 3:36 AM, Markus Armbruster wrote:

John Snow  writes:


On 8/4/20 4:03 AM, Markus Armbruster wrote:

The pain of tweaking the parser is likely dwarved several times over by
the pain of the flag day.


You mention this often; I wonder if I misunderstand the critique,
because the pain of a "flag day" for a new file format seems
negligible to me.

I don't think we edit these .json files very often. Generally, we add
a new command when we need one. The edits are usually one or two lines
plus docstrings.

If anyone has patches in-flight, I genuinely doubt it will take more
than a few minutes to rewrite for the new file format.

No?


You describe the the flag day's one-time pain.

There's also the longer term pain of having to work around git-blame
unable to see beyond the flag day.



So it's not really the "flag day" we're worried about anymore, it's the 
ongoing ease-of-use for vcs history.



I'm not claiming the pain is prohibitive (if I thought it was, I
would've tried to strange this thread in its crib), I am claiming it'll
be much more painful (read: expensive) than a parser tweak.



I do use `git blame` quite a lot, but with a project as old as QEMU, 
most of my trips through history do involve jumping across a few 
refactor gaps as a normal part of that process.


As Dan points out, I often have to back out and add refactorSHA^ to my 
invocation, and just keep hopping backwards until I find what I am truly 
after. It just feels like a fact of programmer life for me at this point.


I've not used Paolo's invocation before, but it looks like it might be 
useful. I'll try to remember to try it out.

Re: [PATCH v12 07/11] qcow2_format.py: Dump bitmap table serialized entries

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

Add bitmap table information to the QCOW2 metadata dump.

Bitmap name   bitmap-1
...
Bitmap table   typesize offset
0  serialized  6553610092544
1  all-zeroes  655360


For serialized, size and offset are size and offset of bitmap table.
But, all-zeroes bitmaps don't have any bitmap table, so size and offset
both are undefined (OK to print zero for them, but 65536 is unrelated).


2  all-zeroes  655360

Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/303.out |  4 
  tests/qemu-iotests/qcow2_format.py | 47 ++
  2 files changed, 51 insertions(+)

diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index dc3739b..d581fb4 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -70,6 +70,8 @@ type  1
  granularity_bits  15
  name_size 8
  extra_data_size   0
+Bitmap table   typesize offset
+0  serialized  6553610092544
  
  Bitmap name   bitmap-2

  bitmap_table_offset   0x9c
@@ -79,4 +81,6 @@ type  1
  granularity_bits  16
  name_size 8
  extra_data_size   0
+Bitmap table   typesize offset
+0  all-zeroes  655360
  
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py

index ca0d350..1f033d4 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -175,6 +175,10 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  entry_raw_size = self.bitmap_dir_entry_raw_size()
  padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
  fd.seek(padding, 1)
+self.bitmap_table = Qcow2BitmapTable(fd=fd,
+ offset=self.bitmap_table_offset,
+ nb_entries=self.bitmap_table_size,
+ cluster_size=self.cluster_size)
  
  def bitmap_dir_entry_raw_size(self):

  return struct.calcsize(self.fmt) + self.name_size + \
@@ -183,6 +187,49 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  def dump(self):
  print(f'{"Bitmap name":<25} {self.name}')
  super(Qcow2BitmapDirEntry, self).dump()
+self.bitmap_table.dump()
+
+
+class Qcow2BitmapTableEntry(Qcow2Struct):
+
+fields = (
+('u64',  '{}', 'entry'),
+)
+
+BME_TABLE_ENTRY_RESERVED_MASK = 0xff0001fe
+BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffe00
+BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
+
+def __init__(self, fd):
+super().__init__(fd=fd)
+self.reserved = self.entry & self.BME_TABLE_ENTRY_RESERVED_MASK
+self.offset = self.entry & self.BME_TABLE_ENTRY_OFFSET_MASK
+if self.offset:
+if self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'invalid'
+else:
+self.type = 'serialized'
+elif self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'all-ones'
+else:
+self.type = 'all-zeroes'
+
+
+class Qcow2BitmapTable:
+
+def __init__(self, fd, offset, nb_entries, cluster_size):
+self.cluster_size = cluster_size
+position = fd.tell()
+fd.seek(offset)
+self.entries = [Qcow2BitmapTableEntry(fd) for _ in range(nb_entries)]
+fd.seek(position)
+
+def dump(self):
+size = self.cluster_size
+bitmap_table = enumerate(self.entries)
+print(f'{"Bitmap table":<14} {"type":<15} {"size":<12} {"offset"}')
+for i, entry in bitmap_table:
+print(f'{i:<14} {entry.type:<15} {size:<12} {entry.offset}')
  


All this looks like 'cluster_size' is not really needed for Qcow2BitmapTable 
class (we can print only offsets).
Still, if you want to save it, can we print it only for entries with 
'serialized' type?

It's minor, I'm OK with it as is:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


  
  QCOW2_EXT_MAGIC_BITMAPS = 0x23852875





--
Best regards,
Vladimir

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Peter Maydell]

On Wed, 5 Aug 2020 at 10:24, Tuguoyi  wrote:
>
> When calculating the offset, the result of left shift operation will be 
> promoted
> to type int64 automatically because the left operand of + operator is 
> uint64_t.
> but the result after integer promotion may be produce an error value for us 
> and
> trigger the following asserting error.
>
> For example, consider i=0x2000, cluster_bits=18, the result of left shift
> operation will be 0x8000. Cause argument i is of signed integer type,
> the result is automatically promoted to 0x8000 which is not
> we expected
>
> The way to trigger the assertion error:
>   qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G
>
> This patch fix it by casting @i to uint64_t before doing left shift operation
>
> Signed-off-by: Guoyi Tu 
> ---

Applied to master, thanks.

-- PMM

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Alberto Garcia]

On Wed 05 Aug 2020 04:16:57 PM CEST, Kevin Wolf wrote:
>> nb_clusters is an int and there are more cases of
>> 
>> nb_clusters << s->cluster_bits
>> 
>> I can see at least these: handle_alloc(), qcow2_free_any_clusters(),
>> qcow2_alloc_cluster_abort().
>
> Actuallyx, handle_alloc() and everything that comes from it should be
> fine. It has a uint64_t nb_clusters locally and limits it:
>
> nb_clusters = MIN(nb_clusters, INT_MAX >> s->cluster_bits);

INT_MAX replaced with BDRV_REQUEST_MAX_BYTES in my subcluster allocation
series, so it should still be fine.

> The problematic request that causes the crash comes actually from
> qcow2_co_truncate(). It limits it only to s->l2_slice_size, which can
> be larger than that, but will be at most 256k (= 2 MB /
> sizeof(uint64_t)).
>
> cow_end.offset will get a wraparound then, too. This is harmless
> because cow_end.nb_bytes = 0, so the offset will be ignored anyway.

In that one nb_clusters is actually int64_t so there's no wraparound.

> I think the proper fix to be made in the 5.2 release cycle would revert
> this one and instead fix the limit in qcow2_co_truncate().
>
> But this one is good enough as a band-aid for 5.1.

The other one is just as simple, no? This line in the while() loop in
qcow2_co_truncate():

   nb_clusters = MIN(nb_clusters, BDRV_REQUEST_MAX_BYTES >> s->cluster_bits);

Berto

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Kevin Wolf]

Am 05.08.2020 um 15:44 hat Alberto Garcia geschrieben:
> On Wed 05 Aug 2020 11:22:58 AM CEST, Tuguoyi wrote:
> > This patch fix it by casting @i to uint64_t before doing left shift
> > operation
> 
> The patch seems fine and I also think that it's perhaps worth a test
> case (although it only seems to happen with preallocation=falloc or full
> so the test would need to generate very large files).
> 
> But I also wonder if there are other cases where this can happen.
> 
> nb_clusters is an int and there are more cases of
> 
> nb_clusters << s->cluster_bits
> 
> I can see at least these: handle_alloc(), qcow2_free_any_clusters(),
> qcow2_alloc_cluster_abort().

Actuallyx, handle_alloc() and everything that comes from it should be
fine. It has a uint64_t nb_clusters locally and limits it:

nb_clusters = MIN(nb_clusters, INT_MAX >> s->cluster_bits);

The problematic request that causes the crash comes actually from
qcow2_co_truncate(). It limits it only to s->l2_slice_size, which can be
larger than that, but will be at most 256k (= 2 MB / sizeof(uint64_t)).

cow_end.offset will get a wraparound then, too. This is harmless because
cow_end.nb_bytes = 0, so the offset will be ignored anyway.

I think the proper fix to be made in the 5.2 release cycle would revert
this one and instead fix the limit in qcow2_co_truncate().

But this one is good enough as a band-aid for 5.1.

Kevin

Re: [PATCH 2/3] block: add logging facility for long standing IO requests

[Denis V. Lunev]

On 8/5/20 4:51 PM, Philippe Mathieu-Daudé wrote:
> On 8/5/20 12:08 PM, Denis V. Lunev wrote:
>> There are severe delays with IO requests processing if QEMU is running in
>> virtual machine or over software defined storage. Such delays potentially
>> results in unpredictable guest behavior. For example, guests over IDE or
>> SATA drive could remount filesystem read-only if write is performed
>> longer than 10 seconds.
>>
>> Such reports are very complex to process. Some good starting point for this
>> seems quite reasonable. This patch provides one. It adds logging of such
>> potentially dangerous long IO operations.
>>
>> Signed-off-by: Denis V. Lunev 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>> CC: Stefan Hajnoczi 
>> CC: Kevin Wolf 
>> CC: Max Reitz 
>> ---
>>  block/accounting.c | 59 +-
>>  blockdev.c |  7 -
>>  include/block/accounting.h |  5 +++-
>>  slirp  |  2 +-
>>  4 files changed, 69 insertions(+), 4 deletions(-)
>>
> ...
>
>>  typedef struct BlockAcctCookie {
>> @@ -101,7 +104,7 @@ typedef struct BlockAcctCookie {
>>  
>>  void block_acct_init(BlockAcctStats *stats);
>>  void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
>> - bool account_failed);
>> +  bool account_failed, unsigned 
>> latency_log_threshold_ms);
>>  void block_acct_cleanup(BlockAcctStats *stats);
>>  void block_acct_add_interval(BlockAcctStats *stats, unsigned 
>> interval_length);
>>  BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,
>> diff --git a/slirp b/slirp
>> index ce94eba204..2faae0f778 16
>> --- a/slirp
>> +++ b/slirp
>> @@ -1 +1 @@
>> -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
>> +Subproject commit 2faae0f778f818fadc873308f983289df697eb93
> SLiRP change unrelated I presume...
>
yes :(

subprojects comes into play.. I have not event changed this.
Just a result from pull :

Re: [PATCH 2/3] block: add logging facility for long standing IO requests

[Philippe Mathieu-Daudé]

On 8/5/20 12:08 PM, Denis V. Lunev wrote:
> There are severe delays with IO requests processing if QEMU is running in
> virtual machine or over software defined storage. Such delays potentially
> results in unpredictable guest behavior. For example, guests over IDE or
> SATA drive could remount filesystem read-only if write is performed
> longer than 10 seconds.
> 
> Such reports are very complex to process. Some good starting point for this
> seems quite reasonable. This patch provides one. It adds logging of such
> potentially dangerous long IO operations.
> 
> Signed-off-by: Denis V. Lunev 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> CC: Stefan Hajnoczi 
> CC: Kevin Wolf 
> CC: Max Reitz 
> ---
>  block/accounting.c | 59 +-
>  blockdev.c |  7 -
>  include/block/accounting.h |  5 +++-
>  slirp  |  2 +-
>  4 files changed, 69 insertions(+), 4 deletions(-)
> 
...

>  typedef struct BlockAcctCookie {
> @@ -101,7 +104,7 @@ typedef struct BlockAcctCookie {
>  
>  void block_acct_init(BlockAcctStats *stats);
>  void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
> - bool account_failed);
> +  bool account_failed, unsigned 
> latency_log_threshold_ms);
>  void block_acct_cleanup(BlockAcctStats *stats);
>  void block_acct_add_interval(BlockAcctStats *stats, unsigned 
> interval_length);
>  BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,
> diff --git a/slirp b/slirp
> index ce94eba204..2faae0f778 16
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> +Subproject commit 2faae0f778f818fadc873308f983289df697eb93

SLiRP change unrelated I presume...

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Alberto Garcia]

On Wed 05 Aug 2020 03:44:08 PM CEST, Alberto Garcia wrote:
> On Wed 05 Aug 2020 11:22:58 AM CEST, Tuguoyi wrote:
>> This patch fix it by casting @i to uint64_t before doing left shift
>> operation
>
> The patch seems fine and I also think that it's perhaps worth a test
> case (although it only seems to happen with preallocation=falloc or full
> so the test would need to generate very large files).
>
> But I also wonder if there are other cases where this can happen.
>
> nb_clusters is an int and there are more cases of
>
> nb_clusters << s->cluster_bits
>
> I can see at least these: handle_alloc(), qcow2_free_any_clusters(),
> qcow2_alloc_cluster_abort().

I forgot to add

Reviewed-by: Alberto Garcia 

Berto

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Alberto Garcia]

On Wed 05 Aug 2020 11:22:58 AM CEST, Tuguoyi wrote:
> This patch fix it by casting @i to uint64_t before doing left shift
> operation

The patch seems fine and I also think that it's perhaps worth a test
case (although it only seems to happen with preallocation=falloc or full
so the test would need to generate very large files).

But I also wonder if there are other cases where this can happen.

nb_clusters is an int and there are more cases of

nb_clusters << s->cluster_bits

I can see at least these: handle_alloc(), qcow2_free_any_clusters(),
qcow2_alloc_cluster_abort().

Berto

Re: [PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Kevin Wolf]

Am 05.08.2020 um 11:22 hat Tuguoyi geschrieben:
> When calculating the offset, the result of left shift operation will be 
> promoted
> to type int64 automatically because the left operand of + operator is 
> uint64_t.
> but the result after integer promotion may be produce an error value for us 
> and
> trigger the following asserting error.
> 
> For example, consider i=0x2000, cluster_bits=18, the result of left shift
> operation will be 0x8000. Cause argument i is of signed integer type,
> the result is automatically promoted to 0x8000 which is not
> we expected
> 
> The way to trigger the assertion error:
>   qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G
> 
> This patch fix it by casting @i to uint64_t before doing left shift operation
> 
> Signed-off-by: Guoyi Tu 

Reviewed-by: Kevin Wolf 

Re: [PATCH for-5.1?] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Eric Blake]

On 8/5/20 4:22 AM, Tuguoyi wrote:

When calculating the offset, the result of left shift operation will be promoted
to type int64 automatically because the left operand of + operator is uint64_t.
but the result after integer promotion may be produce an error value for us and
trigger the following asserting error.

For example, consider i=0x2000, cluster_bits=18, the result of left shift
operation will be 0x8000. Cause argument i is of signed integer type,
the result is automatically promoted to 0x8000 which is not
we expected

The way to trigger the assertion error:
   qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G


I wonder if it is worth an iotest addition to cover this.



This patch fix it by casting @i to uint64_t before doing left shift operation

Signed-off-by: Guoyi Tu 
---
  block/qcow2-cluster.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


While this appears to be long-standing, rather than a regression in 5.1, 
this would be worth getting into -rc3 today if we still have time (if 
not, slipping to 5.2 is okay).




diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a677ba9..550850b 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -980,7 +980,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
  
  assert(l2_index + m->nb_clusters <= s->l2_slice_size);

  for (i = 0; i < m->nb_clusters; i++) {
-uint64_t offset = cluster_offset + (i << s->cluster_bits);
+uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);


We have:
offset = uint64_t + (int << int)

which is indeed a cause of unwanted sign extension.

If I'm not mistaken, it would also be feasible to fix this by changing 
qcow2.h to fix typedef struct BDRVQcow2State to use an unsigned type for 
cluster_bits (the way we already do for struct QCowHeader), avoiding the 
need for a cast here.


But if we're trying to get this in today, rather than auditing all other 
uses of BDRVQcow2State for incorrect typing,


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v12 01/11] iotests: add test for QCOW2 header dump

[Andrey Shinkevich]

On 05.08.2020 14:23, Vladimir Sementsov-Ogievskiy wrote:

30.07.2020 17:15, Andrey Shinkevich wrote:

The simple script creates a QCOW2 image and fills it with some data.
Two bitmaps are created as well. Then the script reads the image header
with extensions from the disk by running the script qcow2.py and dumps
the information to the output. Other entities, such as snapshots, may
be added to the test later.

Suggested-by: Eric Blake 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/303 | 59 
++
  tests/qemu-iotests/303.out | 64 
++

  tests/qemu-iotests/group   |  1 +
  3 files changed, 124 insertions(+)
  create mode 100755 tests/qemu-iotests/303
  create mode 100644 tests/qemu-iotests/303.out

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
new file mode 100755
index 000..3c7a611
--- /dev/null
+++ b/tests/qemu-iotests/303
@@ -0,0 +1,59 @@
+#!/usr/bin/env python3
+#
+# Test for dumping of qcow2 image metadata
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+import subprocess
+from iotests import qemu_img_create, qemu_io, file_path, log, 
filter_qemu_io

+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+disk = file_path('disk')
+chunk = 1024 * 1024
+
+
+def create_bitmap(bitmap_number, disabled):
+    granularity = 1 << (14 + bitmap_number)
+    bitmap_name = 'bitmap-' + str(bitmap_number)
+    vm = iotests.VM().add_drive(disk)
+    vm.launch()
+    vm.qmp_log('block-dirty-bitmap-add', node='drive0', 
name=bitmap_name,
+   granularity=granularity, persistent=True, 
disabled=disabled)

+    vm.shutdown()
+
+
+def write_to_disk(offset, size):
+    write = f'write {offset} {size}'
+    log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+
+
+def add_bitmap(num, begin, end, disabled):
+    log(f'Add bitmap {num}')
+    create_bitmap(num, disabled)
+    for i in range(begin, end):
+    write_to_disk((i-1) * chunk, chunk)


a bit unusual to count chunks starting from "1"..

also, any difference with just

write_to_disk((i-1) * chunk, (end-begin) * chunk)

?



QEMU-IMG limits the maximum number of bytes written with one command by 
... 2M, if I am not wrong.


Andrey



+    log('')
+
+
+qemu_img_create('-f', iotests.imgfmt, disk, '10M')
+
+add_bitmap(1, 1, 7, False)
+add_bitmap(2, 7, 9, True)
+dump = ['qcow2.py', f'{disk}', 'dump-header']


No reason to put disk into f-string, it's a string anyway: f'{disk}' 
is equal to just disk.



Thanks





+subprocess.run(dump)



And you may use just

   subprocess.run(['qcow2.py', disk, 'dump-header'])

without additional variable.



Yes, but further adding the '-j' key to the list looks more elegant to me ))

Andrey





Still, I'm OK with it as is:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

Re: [PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Markus Armbruster]

Markus Armbruster  writes:

> I let this series slide to get my Error API rework done, along with much
> else.  My sincere apologies!
>
> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
> is first.

I'm done with v6.  Summary:

* A few trivial things to correct here and there.

* A few ideas to improve things in relatively minor ways.

* PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
  did the former to enable the latter.  If you had captured that in your
  commit message back then, like you did for the similar PATCH 05, I
  wouldn't be scratching my head now :)

* I dislike PATCH 06, and would like to explore an alternative idea.

* PATCH 08 makes hairy monitor code even hairier, but I don't have
  better ideas.

* I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
  10-12.  Let's ask Stefan for an eye-over.

I'd like to proceed as follows.  You rebase, and address "easy" review
comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
AIO magic and David Gilbert for HMP.  While they review (hopefully), I
explore a replacement for PATCH 06.  And then we touch bases and decide
how to get this thing wrapped.

Okay?

Re: [PATCH v12 01/11] iotests: add test for QCOW2 header dump

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:

The simple script creates a QCOW2 image and fills it with some data.
Two bitmaps are created as well. Then the script reads the image header
with extensions from the disk by running the script qcow2.py and dumps
the information to the output. Other entities, such as snapshots, may
be added to the test later.

Suggested-by: Eric Blake 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/303 | 59 ++
  tests/qemu-iotests/303.out | 64 ++
  tests/qemu-iotests/group   |  1 +
  3 files changed, 124 insertions(+)
  create mode 100755 tests/qemu-iotests/303
  create mode 100644 tests/qemu-iotests/303.out

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
new file mode 100755
index 000..3c7a611
--- /dev/null
+++ b/tests/qemu-iotests/303
@@ -0,0 +1,59 @@
+#!/usr/bin/env python3
+#
+# Test for dumping of qcow2 image metadata
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+import subprocess
+from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+disk = file_path('disk')
+chunk = 1024 * 1024
+
+
+def create_bitmap(bitmap_number, disabled):
+granularity = 1 << (14 + bitmap_number)
+bitmap_name = 'bitmap-' + str(bitmap_number)
+vm = iotests.VM().add_drive(disk)
+vm.launch()
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
+   granularity=granularity, persistent=True, disabled=disabled)
+vm.shutdown()
+
+
+def write_to_disk(offset, size):
+write = f'write {offset} {size}'
+log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+
+
+def add_bitmap(num, begin, end, disabled):
+log(f'Add bitmap {num}')
+create_bitmap(num, disabled)
+for i in range(begin, end):
+write_to_disk((i-1) * chunk, chunk)


a bit unusual to count chunks starting from "1"..

also, any difference with just

write_to_disk((i-1) * chunk, (end-begin) * chunk)

?


+log('')
+
+
+qemu_img_create('-f', iotests.imgfmt, disk, '10M')
+
+add_bitmap(1, 1, 7, False)
+add_bitmap(2, 7, 9, True)
+dump = ['qcow2.py', f'{disk}', 'dump-header']


No reason to put disk into f-string, it's a string anyway: f'{disk}' is equal 
to just disk.


+subprocess.run(dump)



And you may use just

   subprocess.run(['qcow2.py', disk, 'dump-header'])

without additional variable.


Still, I'm OK with it as is:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

[Vladimir Sementsov-Ogievskiy]

30.07.2020 15:02, Max Reitz wrote:

During migration, we release all bitmaps after storing them on disk, as
long as they are (1) stored on disk, (2) not read-only, and (3)
consistent.

(2) seems arbitrary, though.  The reason we do not release them is
because we do not write them, as there is no need to; and then we just
forget about all bitmaps that we have not written to the file.  However,
read-only persistent bitmaps are still in the file and in sync with
their in-memory representation, so we may as well release them just like
any R/W bitmap that we have updated.


Agree, better to release all image-owned bitmaps on inactivation of the image.



It leads to actual problems, too: After migration, letting the source
continue may result in an error if there were any bitmaps on read-only
nodes (such as backing images), because those have not been released by
bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
them (which fails, because they are still present in memory).

Signed-off-by: Max Reitz 


The patch seems OK to me, thanks!

--
Best regards,
Vladimir

Re: [PATCH v6 09/12] hmp: Add support for coroutine command handlers

[Markus Armbruster]

Kevin Wolf  writes:

> Often, QMP command handlers are not only called to handle QMP commands,
> but also from a corresponding HMP command handler. In order to give them
> a consistent environment, optionally run HMP command handlers in a
> coroutine, too.
>
> The implementation is a lot simpler than in QMP because for HMP, we
> still block the VM while the coroutine is running.
>
> Signed-off-by: Kevin Wolf 
> ---
>  monitor/monitor-internal.h |  1 +
>  monitor/hmp.c  | 38 --
>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index b55d6df07f..ad2e64be13 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -74,6 +74,7 @@ typedef struct HMPCommand {
>  const char *help;
>  const char *flags; /* p=preconfig */
>  void (*cmd)(Monitor *mon, const QDict *qdict);
> +bool coroutine;
>  /*
>   * @sub_table is a list of 2nd level of commands. If it does not exist,
>   * cmd should be used. If it exists, sub_table[?].cmd should be
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 3e73a4c3ce..ab0e3e279f 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1056,12 +1056,25 @@ fail:
>  return NULL;
>  }
>  
> +typedef struct HandleHmpCommandCo {
> +Monitor *mon;
> +const HMPCommand *cmd;
> +QDict *qdict;
> +bool done;
> +} HandleHmpCommandCo;
> +
> +static void handle_hmp_command_co(void *opaque)
> +{
> +HandleHmpCommandCo *data = opaque;
> +data->cmd->cmd(data->mon, data->qdict);
> +data->done = true;
> +}
> +
>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>  {
>  QDict *qdict;
>  const HMPCommand *cmd;
>  const char *cmd_start = cmdline;
> -Monitor *old_mon;
>  
>  trace_handle_hmp_command(mon, cmdline);
>  
> @@ -1080,11 +1093,24 @@ void handle_hmp_command(MonitorHMP *mon, const char 
> *cmdline)
>  return;
>  }
>  
> -/* old_mon is non-NULL when called from qmp_human_monitor_command() */
> -old_mon = monitor_cur();
> -monitor_set_cur(qemu_coroutine_self(), &mon->common);
> -cmd->cmd(&mon->common, qdict);
> -monitor_set_cur(qemu_coroutine_self(), old_mon);
> +if (!cmd->coroutine) {
> +/* old_mon is non-NULL when called from qmp_human_monitor_command() 
> */
> +Monitor *old_mon = monitor_cur();
> +monitor_set_cur(qemu_coroutine_self(), &mon->common);
> +cmd->cmd(&mon->common, qdict);
> +monitor_set_cur(qemu_coroutine_self(), old_mon);
> +} else {
> +HandleHmpCommandCo data = {
> +.mon = &mon->common,
> +.cmd = cmd,
> +.qdict = qdict,
> +.done = false,
> +};
> +Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
> +monitor_set_cur(co, &mon->common);

Where is the matching monitor_set_cur(co, NULL)?

> +aio_co_enter(qemu_get_aio_context(), co);
> +AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> +}
>  
>  qobject_unref(qdict);
>  }

Re: cleanups with long-term benefits

[Cornelia Huck]

On Wed, 5 Aug 2020 11:08:02 +0100
Daniel P. Berrangé  wrote:

> On Wed, Aug 05, 2020 at 11:11:55AM +0200, Cornelia Huck wrote:
> > On Wed, 5 Aug 2020 10:05:40 +0100
> > Daniel P. Berrangé  wrote:
> >   
> > > On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:  
> > > > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:
> > > > >> Do you really use "git blame" that much?  "git log -S" does more or 
> > > > >> less
> > > > >> the same function (in a different way) and is not affected as much by
> > > > >> large code movement and transformation patches.
> > > > >
> > > > > I use it a lot!   Following stuff back to find where a change came
> > > > > from and then asking people.
> > > > 
> > > > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > > > just do "git log -p" and search for a relevant line of the code I'm
> > > > "blaming".
> > > 
> > > I used git blame alot too, but I don't think its a reason to not do the
> > > cleanups. It is easy enough to just tell blame to use an earlier commit
> > > if you see it displaying a refactor. I don't think such mild inconvenience
> > > should stop us making otherwise desirable code changes  
> > 
> > I don't think people argue that it should block changes; it it simply
> > another thing to consider when weighing benefits vs. drawbacks.  
> 
> Actually, I'm saying that including "git blame" in such a weighing is
> the mechanism for blocking otherwise beneficial change to the code. Or
> to put it another way, I believe the value assigned to "git blame" in
> such a comparison is miniscule / effectively zero. The only time
> "git blame" should win is if the change in question is purely the
> bike shed colour and has no technical benefits at all.  If there's any
> technical benefit to making the change it should always win.  We
> shouldn't preserve technical debt in the code merely to avoid an impact
> on "git blame".

I think we have to agree to disagree on this. Making "git blame" harder
to use is annoying, and at least for me there's a threshold of
usefulness for the code change that should be considered. (No judgment
on the proposed change here; I don't have really have a horse in that
race.)

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Alex Bennée]

Markus Armbruster  writes:

> Paolo Bonzini  writes:
>
>> On 05/08/20 09:36, Markus Armbruster wrote:
>>> There's also the longer term pain of having to work around git-blame
>>> unable to see beyond the flag day.
>>
>> Do you really use "git blame" that much?  "git log -S" does more or less
>> the same function (in a different way) and is not affected as much by
>> large code movement and transformation patches.
>
> C-x v g
>
> When that doesn't work, I fall back to git-log -S in a shell.

Yep, I'm another that uses blame in the first instance (or magit-blame
inside emacs). My usual fallback after that is git log -p and liberal
use of / which is very inefficient.

-- 
Alex Bennée

Re: [PATCH v6 08/12] qmp: Move dispatcher to a coroutine

[Markus Armbruster]

Kevin Wolf  writes:

> This moves the QMP dispatcher to a coroutine and runs all QMP command
> handlers that declare 'coroutine': true in coroutine context so they
> can avoid blocking the main loop while doing I/O or waiting for other
> events.
>
> For commands that are not declared safe to run in a coroutine, the
> dispatcher drops out of coroutine context by calling the QMP command
> handler from a bottom half.
>
> Signed-off-by: Kevin Wolf 
> ---
>  include/qapi/qmp/dispatch.h |   1 +
>  monitor/monitor-internal.h  |   6 +-
>  monitor/monitor.c   |  55 +---
>  monitor/qmp.c   | 122 +++-
>  qapi/qmp-dispatch.c |  52 +--
>  qapi/qmp-registry.c |   3 +
>  util/aio-posix.c|   8 ++-
>  7 files changed, 201 insertions(+), 46 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 9fd2b720a7..af8d96c570 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -31,6 +31,7 @@ typedef enum QmpCommandOptions
>  typedef struct QmpCommand
>  {
>  const char *name;
> +/* Runs in coroutine context if QCO_COROUTINE is set */
>  QmpCommandFunc *fn;
>  QmpCommandOptions options;
>  QTAILQ_ENTRY(QmpCommand) node;
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index b39e03b744..b55d6df07f 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
>  
>  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
>  extern IOThread *mon_iothread;
> -extern QEMUBH *qmp_dispatcher_bh;
> +extern Coroutine *qmp_dispatcher_co;
> +extern bool qmp_dispatcher_co_shutdown;
> +extern bool qmp_dispatcher_co_busy;
>  extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  extern QemuMutex monitor_lock;
>  extern MonitorList mon_list;
> @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
>  
>  void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
>  void monitor_data_destroy_qmp(MonitorQMP *mon);
> -void monitor_qmp_bh_dispatcher(void *data);
> +void coroutine_fn monitor_qmp_dispatcher_co(void *data);
>  
>  int get_monitor_def(int64_t *pval, const char *name);
>  void help_cmd(Monitor *mon, const char *name);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 35003bb486..50fb5b20d3 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -55,8 +55,32 @@ typedef struct {
>  /* Shared monitor I/O thread */
>  IOThread *mon_iothread;
>  
> -/* Bottom half to dispatch the requests received from I/O thread */
> -QEMUBH *qmp_dispatcher_bh;
> +/* Coroutine to dispatch the requests received from I/O thread */
> +Coroutine *qmp_dispatcher_co;
> +
> +/* Set to true when the dispatcher coroutine should terminate */
> +bool qmp_dispatcher_co_shutdown;
> +
> +/*
> + * qmp_dispatcher_co_busy is used for synchronisation between the
> + * monitor thread and the main thread to ensure that the dispatcher
> + * coroutine never gets scheduled a second time when it's already
> + * scheduled (scheduling the same coroutine twice is forbidden).
> + *
> + * It is true if the coroutine is active and processing requests.
> + * Additional requests may then be pushed onto a mon->qmp_requests,

s/onto a/onto/

> + * and @qmp_dispatcher_co_shutdown may be set without further ado.
> + * @qmp_dispatcher_co_busy must not be woken up in this case.
> + *
> + * If false, you also have to set @qmp_dispatcher_co_busy to true and
> + * wake up @qmp_dispatcher_co after pushing the new requests.
> + *
> + * The coroutine will automatically change this variable back to false
> + * before it yields.  Nobody else may set the variable to false.
> + *
> + * Access must be atomic for thread safety.
> + */
> +bool qmp_dispatcher_co_busy;
>  
>  /*
>   * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> @@ -608,9 +632,24 @@ void monitor_cleanup(void)
>  }
>  qemu_mutex_unlock(&monitor_lock);
>  
> -/* QEMUBHs needs to be deleted before destroying the I/O thread */
> -qemu_bh_delete(qmp_dispatcher_bh);
> -qmp_dispatcher_bh = NULL;
> +/*
> + * The dispatcher needs to stop before destroying the I/O thread.
> + *
> + * We need to poll both qemu_aio_context and iohandler_ctx to make
> + * sure that the dispatcher coroutine keeps making progress and
> + * eventually terminates.  qemu_aio_context is automatically
> + * polled by calling AIO_WAIT_WHILE on it, but we must poll
> + * iohandler_ctx manually.
> + */
> +qmp_dispatcher_co_shutdown = true;
> +if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) {
> +aio_co_wake(qmp_dispatcher_co);
> +}
> +
> +AIO_WAIT_WHILE(qemu_get_aio_context(),
> +   (aio_poll(iohandler_get_aio_context(), false),
> +atomic_mb_read(&qmp_dispatcher_co_busy)));
> +
>  if (mon_iothread) {
>  io

[PATCH 1/3] block/block-backend: add converter from BlockAcctStats to BlockBackend

[Denis V. Lunev]

Right now BlockAcctStats is always reside on BlockBackend. This structure
is not used in any other place. Thus we are able to create a converter
from one pointer to another.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Stefan Hajnoczi 
CC: Kevin Wolf 
CC: Max Reitz 
---
 block/block-backend.c  | 5 +
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 3a13cb5f0b..88e531df98 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2153,6 +2153,11 @@ BlockAcctStats *blk_get_stats(BlockBackend *blk)
 return &blk->stats;
 }
 
+BlockBackend *blk_stats2blk(BlockAcctStats *s)
+{
+return container_of(s, BlockBackend, stats);
+}
+
 void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque)
 {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..bd4694e7bc 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -227,6 +227,7 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier 
*notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
+BlockBackend *blk_stats2blk(BlockAcctStats *stats);
 BlockBackendRootState *blk_get_root_state(BlockBackend *blk);
 void blk_update_root_state(BlockBackend *blk);
 bool blk_get_detect_zeroes_from_root_state(BlockBackend *blk);
-- 
2.17.1

Re: cleanups with long-term benefits

[Daniel P . Berrangé]

On Wed, Aug 05, 2020 at 11:11:55AM +0200, Cornelia Huck wrote:
> On Wed, 5 Aug 2020 10:05:40 +0100
> Daniel P. Berrangé  wrote:
> 
> > On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> > > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:  
> > > >> Do you really use "git blame" that much?  "git log -S" does more or 
> > > >> less
> > > >> the same function (in a different way) and is not affected as much by
> > > >> large code movement and transformation patches.  
> > > >
> > > > I use it a lot!   Following stuff back to find where a change came
> > > > from and then asking people.  
> > > 
> > > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > > just do "git log -p" and search for a relevant line of the code I'm
> > > "blaming".  
> > 
> > I used git blame alot too, but I don't think its a reason to not do the
> > cleanups. It is easy enough to just tell blame to use an earlier commit
> > if you see it displaying a refactor. I don't think such mild inconvenience
> > should stop us making otherwise desirable code changes
> 
> I don't think people argue that it should block changes; it it simply
> another thing to consider when weighing benefits vs. drawbacks.

Actually, I'm saying that including "git blame" in such a weighing is
the mechanism for blocking otherwise beneficial change to the code. Or
to put it another way, I believe the value assigned to "git blame" in
such a comparison is miniscule / effectively zero. The only time
"git blame" should win is if the change in question is purely the
bike shed colour and has no technical benefits at all.  If there's any
technical benefit to making the change it should always win.  We
shouldn't preserve technical debt in the code merely to avoid an impact
on "git blame".

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PATCH 3/3] block: enable long IO requests report by default

[Denis V. Lunev]

Latency threshold is set to 10 seconds following guest request timeout
on legacy storage controller.

Signed-off-by: Denis V. Lunev 
CC: Vladimir Sementsov-Ogievskiy 
CC: Stefan Hajnoczi 
CC: Kevin Wolf 
CC: Max Reitz 
---
 blockdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 66158d1292..9eb58efc2b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -623,7 +623,8 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 bs->detect_zeroes = detect_zeroes;
 
 block_acct_setup(blk_get_stats(blk), account_invalid, account_failed,
-qemu_opt_get_number(opts, "latency-log-threshold", 0));
+qemu_opt_get_number(opts, "latency-log-threshold",
+1 /* ms */));
 
 if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) {
 blk_unref(blk);
-- 
2.17.1

[PATCH v2 0/3] block: add logging facility for long standing IO requests

[Denis V. Lunev]

There are severe delays with IO requests processing if QEMU is running in
virtual machine or over software defined storage. Such delays potentially
results in unpredictable guest behavior. For example, guests over IDE or
SATA drive could remount filesystem read-only if write is performed
longer than 10 seconds.

Such reports are very complex to process. Some good starting point for this
seems quite reasonable. This patch provides one. It adds logging of such
potentially dangerous long IO operations.

Changes from v1:
- fixed conversions using macros suggested by Stefan
- fixed option declaration
- enabled by default with patch 3

Signed-off-by: Denis V. Lunev 
CC: Vladimir Sementsov-Ogievskiy 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 

[PATCH 2/3] block: add logging facility for long standing IO requests

[Denis V. Lunev]

There are severe delays with IO requests processing if QEMU is running in
virtual machine or over software defined storage. Such delays potentially
results in unpredictable guest behavior. For example, guests over IDE or
SATA drive could remount filesystem read-only if write is performed
longer than 10 seconds.

Such reports are very complex to process. Some good starting point for this
seems quite reasonable. This patch provides one. It adds logging of such
potentially dangerous long IO operations.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
CC: Stefan Hajnoczi 
CC: Kevin Wolf 
CC: Max Reitz 
---
 block/accounting.c | 59 +-
 blockdev.c |  7 -
 include/block/accounting.h |  5 +++-
 slirp  |  2 +-
 4 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index 8d41c8a83a..24f48c84b8 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -27,7 +27,9 @@
 #include "block/accounting.h"
 #include "block/block_int.h"
 #include "qemu/timer.h"
+#include "qemu/log.h"
 #include "sysemu/qtest.h"
+#include "sysemu/block-backend.h"
 
 static QEMUClockType clock_type = QEMU_CLOCK_REALTIME;
 static const int qtest_latency_ns = NANOSECONDS_PER_SECOND / 1000;
@@ -41,10 +43,11 @@ void block_acct_init(BlockAcctStats *stats)
 }
 
 void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
-  bool account_failed)
+  bool account_failed, unsigned latency_log_threshold_ms)
 {
 stats->account_invalid = account_invalid;
 stats->account_failed = account_failed;
+stats->latency_log_threshold_ms = latency_log_threshold_ms;
 }
 
 void block_acct_cleanup(BlockAcctStats *stats)
@@ -182,6 +185,58 @@ void block_latency_histograms_clear(BlockAcctStats *stats)
 }
 }
 
+static const char *block_account_type(enum BlockAcctType type)
+{
+switch (type) {
+case BLOCK_ACCT_READ:
+return "READ";
+case BLOCK_ACCT_WRITE:
+return "WRITE";
+case BLOCK_ACCT_FLUSH:
+return "DISCARD";
+case BLOCK_ACCT_UNMAP:
+return "TRUNCATE";
+case BLOCK_ACCT_NONE:
+return "NONE";
+case BLOCK_MAX_IOTYPE:
+break;
+}
+return "UNKNOWN";
+}
+
+static void block_acct_report_long(BlockAcctStats *stats,
+   BlockAcctCookie *cookie, int64_t latency_ns)
+{
+unsigned latency_ms = latency_ns / SCALE_MS;
+int64_t start_time_host_s;
+char buf[64];
+struct tm t;
+BlockDriverState *bs;
+
+if (cookie->type == BLOCK_ACCT_NONE) {
+return;
+}
+if (stats->latency_log_threshold_ms == 0) {
+return;
+}
+if (latency_ms < stats->latency_log_threshold_ms) {
+return;
+}
+
+start_time_host_s = cookie->start_time_ns / NANOSECONDS_PER_SECOND;
+strftime(buf, sizeof(buf), "%m-%d %H:%M:%S",
+ localtime_r(&start_time_host_s, &t));
+
+bs = blk_bs(blk_stats2blk(stats));
+qemu_log("long %s[%ld] IO request: %d.03%d since %s.%03d bs: %s(%s, %s)\n",
+ block_account_type(cookie->type), cookie->bytes,
+ (int)(latency_ms / 1000), (int)(latency_ms % 1000), buf,
+ (int)((cookie->start_time_ns / 100) % 1000),
+ bs == NULL ? "unknown" : bdrv_get_node_name(bs),
+ bs == NULL ? "unknown" : bdrv_get_format_name(bs),
+ bs == NULL ? "unknown" : bs->filename);
+}
+
 static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie 
*cookie,
  bool failed)
 {
@@ -222,6 +277,8 @@ static void block_account_one_io(BlockAcctStats *stats, 
BlockAcctCookie *cookie,
 
 qemu_mutex_unlock(&stats->lock);
 
+block_acct_report_long(stats, cookie, latency_ns);
+
 cookie->type = BLOCK_ACCT_NONE;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..66158d1292 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -622,7 +622,8 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 
 bs->detect_zeroes = detect_zeroes;
 
-block_acct_setup(blk_get_stats(blk), account_invalid, account_failed);
+block_acct_setup(blk_get_stats(blk), account_invalid, account_failed,
+qemu_opt_get_number(opts, "latency-log-threshold", 0));
 
 if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) {
 blk_unref(blk);
@@ -3727,6 +3728,10 @@ QemuOptsList qemu_common_drive_opts = {
 .type = QEMU_OPT_BOOL,
 .help = "whether to account for failed I/O operations "
 "in the statistics",
+},{
+.name = "latency-log-threshold",
+.type = QEMU_OPT_NUMBER,
+.help = "threshold for long I/O report (disabled if <=0), in ms",
 },
 { /* end of list */ }
 },
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 

[PATCH v2 2/3] async: always set ctx->notified in aio_notify()

[Stefan Hajnoczi]

aio_notify() does not set ctx->notified when called with
ctx->aio_notify_me disabled. Therefore aio_notify_me needs to be enabled
during polling.

This is suboptimal since expensive event_notifier_set(&ctx->notifier)
and event_notifier_test_and_clear(&ctx->notifier) calls are required
when ctx->aio_notify_me is enabled.

Change aio_notify() so that aio->notified is always set, regardless of
ctx->aio_notify_me. This will make polling cheaper since
ctx->aio_notify_me can remain disabled. Move the
event_notifier_test_and_clear() to the fd handler function (which is now
no longer an empty function so "dummy" has been dropped from its name).

The next patch takes advantage of this by optimizing polling in
util/aio-posix.c.

Signed-off-by: Stefan Hajnoczi 
---
 util/async.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/util/async.c b/util/async.c
index d9f987e133..3ec3e8d135 100644
--- a/util/async.c
+++ b/util/async.c
@@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx)
 
 void aio_notify(AioContext *ctx)
 {
-/* Write e.g. bh->scheduled before reading ctx->notify_me.  Pairs
+/*
+ * Write e.g. bh->flags before writing ctx->notified.  Pairs with smp_mb in
+ * aio_notify_accept.
+ */
+smp_wmb();
+atomic_set(&ctx->notified, true);
+
+/*
+ * Write ctx->notified before reading ctx->notify_me.  Pairs
  * with smp_mb in aio_ctx_prepare or aio_poll.
  */
 smp_mb();
 if (atomic_read(&ctx->notify_me)) {
 event_notifier_set(&ctx->notifier);
-atomic_mb_set(&ctx->notified, true);
 }
 }
 
 void aio_notify_accept(AioContext *ctx)
 {
-if (atomic_xchg(&ctx->notified, false)
-#ifdef WIN32
-|| true
-#endif
-) {
-event_notifier_test_and_clear(&ctx->notifier);
-}
+atomic_set(&ctx->notified, false);
+
+/*
+ * Write ctx->notified before reading e.g. bh->flags.  Pairs with smp_mb in
+ * aio_notify.
+ */
+smp_mb();
 }
 
 static void aio_timerlist_notify(void *opaque, QEMUClockType type)
@@ -445,8 +452,11 @@ static void aio_timerlist_notify(void *opaque, 
QEMUClockType type)
 aio_notify(opaque);
 }
 
-static void aio_context_notifier_dummy_cb(EventNotifier *e)
+static void aio_context_notifier_cb(EventNotifier *e)
 {
+AioContext *ctx = container_of(e, AioContext, notifier);
+
+event_notifier_test_and_clear(&ctx->notifier);
 }
 
 /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
@@ -508,7 +518,7 @@ AioContext *aio_context_new(Error **errp)
 
 aio_set_event_notifier(ctx, &ctx->notifier,
false,
-   aio_context_notifier_dummy_cb,
+   aio_context_notifier_cb,
aio_context_notifier_poll);
 #ifdef CONFIG_LINUX_AIO
 ctx->linux_aio = NULL;
-- 
2.26.2

[PATCH v2 1/3] async: rename event_notifier_dummy_cb/poll()

[Stefan Hajnoczi]

The event_notifier_*() prefix can be confused with the EventNotifier
APIs that are also called event_notifier_*().

Rename the functions to aio_context_notifier_*() to make it clear that
they relate to the AioContext::notifier field.

Signed-off-by: Stefan Hajnoczi 
---
 util/async.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/async.c b/util/async.c
index 1319eee3bc..d9f987e133 100644
--- a/util/async.c
+++ b/util/async.c
@@ -445,12 +445,12 @@ static void aio_timerlist_notify(void *opaque, 
QEMUClockType type)
 aio_notify(opaque);
 }
 
-static void event_notifier_dummy_cb(EventNotifier *e)
+static void aio_context_notifier_dummy_cb(EventNotifier *e)
 {
 }
 
 /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */
-static bool event_notifier_poll(void *opaque)
+static bool aio_context_notifier_poll(void *opaque)
 {
 EventNotifier *e = opaque;
 AioContext *ctx = container_of(e, AioContext, notifier);
@@ -508,8 +508,8 @@ AioContext *aio_context_new(Error **errp)
 
 aio_set_event_notifier(ctx, &ctx->notifier,
false,
-   event_notifier_dummy_cb,
-   event_notifier_poll);
+   aio_context_notifier_dummy_cb,
+   aio_context_notifier_poll);
 #ifdef CONFIG_LINUX_AIO
 ctx->linux_aio = NULL;
 #endif
-- 
2.26.2

[PATCH v2 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

Polling only monitors the ctx->notified field and does not need the
ctx->notifier EventNotifier to be signalled. Keep ctx->aio_notify_me
disabled while polling to avoid unnecessary EventNotifier syscalls.

This optimization improves virtio-blk 4KB random read performance by
18%. The following results are with an IOThread and the null-co block
driver:

Test IOPS   Error
Before  244518.62 ± 1.20%
After   290706.11 ± 0.44%

Signed-off-by: Stefan Hajnoczi 
---
 util/aio-posix.c | 59 +---
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 1b2a3af65b..8d10910bcf 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -464,9 +464,6 @@ static bool remove_idle_poll_handlers(AioContext *ctx, 
int64_t now)
  *
  * Polls for a given time.
  *
- * Note that ctx->notify_me must be non-zero so this function can detect
- * aio_notify().
- *
  * Note that the caller must have incremented ctx->list_lock.
  *
  * Returns: true if progress was made, false otherwise
@@ -476,7 +473,6 @@ static bool run_poll_handlers(AioContext *ctx, int64_t 
max_ns, int64_t *timeout)
 bool progress;
 int64_t start_time, elapsed_time;
 
-assert(ctx->notify_me);
 assert(qemu_lockcnt_count(&ctx->list_lock) > 0);
 
 trace_run_poll_handlers_begin(ctx, max_ns, *timeout);
@@ -520,8 +516,6 @@ static bool run_poll_handlers(AioContext *ctx, int64_t 
max_ns, int64_t *timeout)
  * @timeout: timeout for blocking wait, computed by the caller and updated if
  *polling succeeds.
  *
- * ctx->notify_me must be non-zero so this function can detect aio_notify().
- *
  * Note that the caller must have incremented ctx->list_lock.
  *
  * Returns: true if progress was made, false otherwise
@@ -566,23 +560,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
  */
 assert(in_aio_context_home_thread(ctx));
 
-/* aio_notify can avoid the expensive event_notifier_set if
- * everything (file descriptors, bottom halves, timers) will
- * be re-evaluated before the next blocking poll().  This is
- * already true when aio_poll is called with blocking == false;
- * if blocking == true, it is only true after poll() returns,
- * so disable the optimization now.
- */
-if (blocking) {
-atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
-/*
- * Write ctx->notify_me before computing the timeout
- * (reading bottom half flags, etc.).  Pairs with
- * smp_mb in aio_notify().
- */
-smp_mb();
-}
-
 qemu_lockcnt_inc(&ctx->list_lock);
 
 if (ctx->poll_max_ns) {
@@ -597,15 +574,41 @@ bool aio_poll(AioContext *ctx, bool blocking)
  * system call---a single round of run_poll_handlers_once suffices.
  */
 if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
+bool use_notify_me = timeout != 0;
+
+/*
+ * aio_notify can avoid the expensive event_notifier_set if
+ * everything (file descriptors, bottom halves, timers) will
+ * be re-evaluated before the next blocking poll().  This is
+ * already true when aio_poll is called with blocking == false;
+ * if blocking == true, it is only true after poll() returns,
+ * so disable the optimization now.
+ */
+if (use_notify_me) {
+atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
+/*
+ * Write ctx->notify_me before reading ctx->notified.  Pairs with
+ * smp_mb in aio_notify().
+ */
+smp_mb();
+
+/* Don't block if aio_notify() was called */
+if (atomic_read(&ctx->notified)) {
+timeout = 0;
+}
+}
+
 ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
-}
 
-if (blocking) {
-/* Finish the poll before clearing the flag.  */
-atomic_store_release(&ctx->notify_me, atomic_read(&ctx->notify_me) - 
2);
-aio_notify_accept(ctx);
+if (use_notify_me) {
+/* Finish the poll before clearing the flag.  */
+atomic_store_release(&ctx->notify_me,
+ atomic_read(&ctx->notify_me) - 2);
+}
 }
 
+aio_notify_accept(ctx);
+
 /* Adjust polling time */
 if (ctx->poll_max_ns) {
 int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start;
-- 
2.26.2

[PATCH v2 0/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

v2:
 * Added smp_mb() in aio_notify_accept() [Paolo]
 * Added comments about memory barrier pairing [Paolo]
 * Eliminated extra aio_compute_timeout() before calling ppoll()

This patch series eliminates ctx->notifier EventNotifier activity when
aio_poll() is in polling mode. There is no need to use the EventNotifier since
a polling handler can detect that aio_notify() has been called by monitoring a
field in memory instead.

Optimizing out the EventNotifier calls improves null-co random read 4KB
iodepth=1 IOPS by 18%.

I have not modified docs/spin/aio_notify*.promela because I'm not familiar with
the SPIN model checker.

Stefan Hajnoczi (3):
  async: rename event_notifier_dummy_cb/poll()
  async: always set ctx->notified in aio_notify()
  aio-posix: keep aio_notify_me disabled during polling

 util/aio-posix.c | 59 +---
 util/async.c | 36 ++---
 2 files changed, 54 insertions(+), 41 deletions(-)

-- 
2.26.2

[PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()

[Tuguoyi]

When calculating the offset, the result of left shift operation will be promoted
to type int64 automatically because the left operand of + operator is uint64_t.
but the result after integer promotion may be produce an error value for us and
trigger the following asserting error.

For example, consider i=0x2000, cluster_bits=18, the result of left shift
operation will be 0x8000. Cause argument i is of signed integer type,
the result is automatically promoted to 0x8000 which is not
we expected

The way to trigger the assertion error:
  qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G

This patch fix it by casting @i to uint64_t before doing left shift operation

Signed-off-by: Guoyi Tu 
---
 block/qcow2-cluster.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a677ba9..550850b 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -980,7 +980,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
 
 assert(l2_index + m->nb_clusters <= s->l2_slice_size);
 for (i = 0; i < m->nb_clusters; i++) {
-uint64_t offset = cluster_offset + (i << s->cluster_bits);
+uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);
 /* if two concurrent writes happen to the same unallocated cluster
  * each write allocates separate cluster and writes data concurrently.
  * The first one to complete updates l2 table with pointer to its
-- 
2.7.4

--
Best regards,
Guoyi

Re: cleanups with long-term benefits

[Cornelia Huck]

On Wed, 5 Aug 2020 10:05:40 +0100
Daniel P. Berrangé  wrote:

> On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> > On 05/08/20 10:39, Dr. David Alan Gilbert wrote:  
> > >> Do you really use "git blame" that much?  "git log -S" does more or less
> > >> the same function (in a different way) and is not affected as much by
> > >> large code movement and transformation patches.  
> > >
> > > I use it a lot!   Following stuff back to find where a change came
> > > from and then asking people.  
> > 
> > Indeed, but I use "git log -S" instead. :)  Another possibility is to
> > just do "git log -p" and search for a relevant line of the code I'm
> > "blaming".  
> 
> I used git blame alot too, but I don't think its a reason to not do the
> cleanups. It is easy enough to just tell blame to use an earlier commit
> if you see it displaying a refactor. I don't think such mild inconvenience
> should stop us making otherwise desirable code changes

I don't think people argue that it should block changes; it it simply
another thing to consider when weighing benefits vs. drawbacks.

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Daniel P . Berrangé]

On Wed, Aug 05, 2020 at 10:49:35AM +0200, Paolo Bonzini wrote:
> On 05/08/20 10:39, Dr. David Alan Gilbert wrote:
> >> Do you really use "git blame" that much?  "git log -S" does more or less
> >> the same function (in a different way) and is not affected as much by
> >> large code movement and transformation patches.
> >
> > I use it a lot!   Following stuff back to find where a change came
> > from and then asking people.
> 
> Indeed, but I use "git log -S" instead. :)  Another possibility is to
> just do "git log -p" and search for a relevant line of the code I'm
> "blaming".

I used git blame alot too, but I don't think its a reason to not do the
cleanups. It is easy enough to just tell blame to use an earlier commit
if you see it displaying a refactor. I don't think such mild inconvenience
should stop us making otherwise desirable code changes


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] aio-posix: keep aio_notify_me disabled during polling

[Stefan Hajnoczi]

On Tue, Aug 04, 2020 at 06:53:09PM +0200, Paolo Bonzini wrote:
> On 04/08/20 12:29, Stefan Hajnoczi wrote:
> > On Tue, Aug 04, 2020 at 06:28:04AM +0100, Stefan Hajnoczi wrote:
> >> @@ -597,15 +574,38 @@ bool aio_poll(AioContext *ctx, bool blocking)
> >>   * system call---a single round of run_poll_handlers_once suffices.
> >>   */
> >>  if (timeout || ctx->fdmon_ops->need_wait(ctx)) {
> >> +/*
> >> + * aio_notify can avoid the expensive event_notifier_set if
> >> + * everything (file descriptors, bottom halves, timers) will
> >> + * be re-evaluated before the next blocking poll().  This is
> >> + * already true when aio_poll is called with blocking == false;
> >> + * if blocking == true, it is only true after poll() returns,
> >> + * so disable the optimization now.
> >> + */
> >> +if (timeout) {
> >> +atomic_set(&ctx->notify_me, atomic_read(&ctx->notify_me) + 2);
> >> +/*
> >> + * Write ctx->notify_me before computing the timeout
> >> + * (reading bottom half flags, etc.).  Pairs with
> >> + * smp_mb in aio_notify().
> >> + */
> >> +smp_mb();
> >> +
> >> +/* Check again in case a shorter timer was added */
> >> +timeout = qemu_soonest_timeout(timeout, 
> >> aio_compute_timeout(ctx));
> >> +}
> >> +
> >>  ret = ctx->fdmon_ops->wait(ctx, &ready_list, timeout);
> >> -}
> >>  
> >> -if (blocking) {
> >> -/* Finish the poll before clearing the flag.  */
> >> -atomic_store_release(&ctx->notify_me, 
> >> atomic_read(&ctx->notify_me) - 2);
> >> -aio_notify_accept(ctx);
> >> +if (timeout) {
> >> +/* Finish the poll before clearing the flag.  */
> >> +atomic_store_release(&ctx->notify_me,
> >> + atomic_read(&ctx->notify_me) - 2);
> >> +}
> >>  }
> > 
> > Hi Paolo,
> > We can avoid calling aio_compute_timeout() like this, what do you think?
> 
> I don't understand :) except I guess you mean we can avoid the second
> call.  Can you post either a complete patch with this squashed, or a 4th
> patch (whatever you think is best)?

Sure, I'll post a new revision of this series.

Stefan


signature.asc
Description: PGP signature

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Markus Armbruster]

Paolo Bonzini  writes:

> On 05/08/20 09:36, Markus Armbruster wrote:
>> There's also the longer term pain of having to work around git-blame
>> unable to see beyond the flag day.
>
> Do you really use "git blame" that much?  "git log -S" does more or less
> the same function (in a different way) and is not affected as much by
> large code movement and transformation patches.

C-x v g

When that doesn't work, I fall back to git-log -S in a shell.

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Paolo Bonzini]

On 05/08/20 10:39, Dr. David Alan Gilbert wrote:
>> Do you really use "git blame" that much?  "git log -S" does more or less
>> the same function (in a different way) and is not affected as much by
>> large code movement and transformation patches.
>
> I use it a lot!   Following stuff back to find where a change came
> from and then asking people.

Indeed, but I use "git log -S" instead. :)  Another possibility is to
just do "git log -p" and search for a relevant line of the code I'm
"blaming".

Paolo

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Nir Soffer]

On Wed, Aug 5, 2020 at 10:38 AM Vladimir Sementsov-Ogievskiy
 wrote:
>
> 28.07.2020 19:05, Nir Soffer wrote:
> > On Tue, Jul 28, 2020 at 4:43 PM Vladimir Sementsov-Ogievskiy
> >  wrote:
> >>
> >> 28.07.2020 00:58, Nir Soffer wrote:
> >>> Instead of duplicating the code to wait until the server is ready and
> >>> remember to terminate the server and wait for it, make it possible to
> >>> use like this:
> >>>
> >>>   with qemu_nbd_popen('-k', sock, image):
> >>>   # Access image via qemu-nbd socket...
> >>>
> >>> Only test 264 used this helper, but I had to modify the output since it
> >>> did not consistently when starting and stopping qemu-nbd.
> >>>
> >>> Signed-off-by: Nir Soffer 
> >>> ---
> >>>tests/qemu-iotests/264| 76 +--
> >>>tests/qemu-iotests/264.out|  2 +
> >>>tests/qemu-iotests/iotests.py | 28 -
> >>>3 files changed, 56 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
> >>> index 304a7443d7..666f164ed8 100755
> >>> --- a/tests/qemu-iotests/264
> >>> +++ b/tests/qemu-iotests/264
> >>> @@ -36,48 +36,32 @@ wait_step = 0.2
> >>>
> >>>qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
> >>>qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
> >>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >>>
> >>> -# Wait for NBD server availability
> >>> -t = 0
> >>> -ok = False
> >>> -while t < wait_limit:
> >>> -ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
> >>> -if ok:
> >>> -break
> >>> -time.sleep(wait_step)
> >>> -t += wait_step
> >>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> >>> +vm = iotests.VM().add_drive(disk_a)
> >>> +vm.launch()
> >>> +vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> >>> +
> >>> +vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> >>> +   **{'node_name': 'backup0',
> >>> +  'driver': 'raw',
> >>> +  'file': {'driver': 'nbd',
> >>> +   'server': {'type': 'unix', 'path': nbd_sock},
> >>> +   'reconnect-delay': 10}})
> >>> +vm.qmp_log('blockdev-backup', device='drive0', sync='full', 
> >>> target='backup0',
> >>> +   speed=(1 * 1024 * 1024))
> >>> +
> >>> +# Wait for some progress
> >>> +t = 0
> >>> +while t < wait_limit:
> >>> +jobs = vm.qmp('query-block-jobs')['return']
> >>> +if jobs and jobs[0]['offset'] > 0:
> >>> +break
> >>> +time.sleep(wait_step)
> >>> +t += wait_step
> >>>
> >>> -assert ok
> >>> -
> >>> -vm = iotests.VM().add_drive(disk_a)
> >>> -vm.launch()
> >>> -vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> >>> -
> >>> -vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> >>> -   **{'node_name': 'backup0',
> >>> -  'driver': 'raw',
> >>> -  'file': {'driver': 'nbd',
> >>> -   'server': {'type': 'unix', 'path': nbd_sock},
> >>> -   'reconnect-delay': 10}})
> >>> -vm.qmp_log('blockdev-backup', device='drive0', sync='full', 
> >>> target='backup0',
> >>> -   speed=(1 * 1024 * 1024))
> >>> -
> >>> -# Wait for some progress
> >>> -t = 0
> >>> -while t < wait_limit:
> >>> -jobs = vm.qmp('query-block-jobs')['return']
> >>>if jobs and jobs[0]['offset'] > 0:
> >>> -break
> >>> -time.sleep(wait_step)
> >>> -t += wait_step
> >>> -
> >>> -if jobs and jobs[0]['offset'] > 0:
> >>> -log('Backup job is started')
> >>> -
> >>> -log('Kill NBD server')
> >>> -srv.kill()
> >>> -srv.wait()
> >>> +log('Backup job is started')
> >>>
> >>>jobs = vm.qmp('query-block-jobs')['return']
> >>>if jobs and jobs[0]['offset'] < jobs[0]['len']:
> >>> @@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', 
> >>> speed=0)
> >>># Emulate server down time for 1 second
> >>>time.sleep(1)
> >>>
> >>> -log('Start NBD server')
> >>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >>> -
> >>> -e = vm.event_wait('BLOCK_JOB_COMPLETED')
> >>> -log('Backup completed: {}'.format(e['data']['offset']))
> >>> -
> >>> -vm.qmp_log('blockdev-del', node_name='backup0')
> >>> -srv.kill()
> >>> -vm.shutdown()
> >>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> >>> +e = vm.event_wait('BLOCK_JOB_COMPLETED')
> >>> +log('Backup completed: {}'.format(e['data']['offset']))
> >>> +vm.qmp_log('blockdev-del', node_name='backup0')
> >>> +vm.shutdown()
> >>> diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
> >>> index 3000944b09..c45b1e81ef 100644
> >>> --- a/tests/qemu-iotests/264.out
> >>> +++ b/tests/qemu-iotests/264.out
> >>> @@ -1,3 +1,4 @@
> >>> +Start NBD server
> >>>{"execute": "blockdev-add", "arguments": {"driver": "raw", "fil

Re: cleanups with long-term benefits

[Cornelia Huck]

On Wed, 5 Aug 2020 10:25:30 +0200
Paolo Bonzini  wrote:

> On 05/08/20 09:36, Markus Armbruster wrote:
> > There's also the longer term pain of having to work around git-blame
> > unable to see beyond the flag day.  
> 
> Do you really use "git blame" that much?  "git log -S" does more or less
> the same function (in a different way) and is not affected as much by
> large code movement and transformation patches.

I'm not sure the two of them really perform the same function.

FWIW, I like using git {blame|annotate} to find out when/why some code
areas were changed, and it's often not "when was this line introduced",
but "I see some commits changing this function, let's find out more
about them." And yes, I use that quite regularly.

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Markus Armbruster]

Markus Armbruster  writes:

> Paolo Bonzini  writes:
[...]
>> That said, after a bit more research I'm skeptical about the possibility
>> of using an off-the-shelf parser because most of them either don't
>> support comments, or are based on YAJL which simply discards comments.
>>
>> Since '//' comments are harder to parse than "#" comments, this would
>> actually _add_ code instead of removing it.  Also since our doc comment
>> syntax uses "##" as a delimiter, we'd have to bikeshed what the doc
>> comments would look like ("//!", "///", etc.).
>
> Doc comments don't have to be comments in the schema language.  They
> could be doc strings.  Requires decent support for long strings, which
> JSON does not provide.

There's another complication besides multi-line strings: funny
characters.

Since QAPI schema strings are all names, and names are restricted to
ASCII letters, digits, hyphen, and underscore, we limit strings to
printable ASCII, so we don't have to deal with control characters,
escape sequences, surrogate pairs, and all that crap.  Comments are
UTF-8.

[...]

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> On 05/08/20 09:36, Markus Armbruster wrote:
> > There's also the longer term pain of having to work around git-blame
> > unable to see beyond the flag day.
> 
> Do you really use "git blame" that much?  "git log -S" does more or less
> the same function (in a different way) and is not affected as much by
> large code movement and transformation patches.

I use it a lot!   Following stuff back to find where a change came
from and then asking people.

Dave

> Paolo
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Kevin Wolf]

Am 05.08.2020 um 09:28 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
> >> Kevin Wolf  writes:
> >> 
> >> > This way, a monitor command handler will still be able to access the
> >> > current monitor, but when it yields, all other code code will correctly
> >> > get NULL from monitor_cur().
> >> >
> >> > Outside of coroutine context, qemu_coroutine_self() returns the leader
> >> > coroutine of the current thread.
> >> 
> >> Unsaid: you use it as a hash table key to map from coroutine to monitor,
> >> and for that you need it to return a value unique to the coroutine in
> >> coroutine context, and a value unique to the thread outside coroutine
> >> context.  Which qemu_coroutine_self() does.  Correct?
> >
> > Correct.
> >
> >> The hash table works, but I hate it just as much as I hate
> >> pthread_getspecific() / pthread_setspecific().
> >> 
> >> What we have here is a need for coroutine-local data.  Feels like a
> >> perfectly natural concept to me.
> >
> > If you have a good concept how to implement this in a generic way that
> > doesn't impact the I/O fast path, feel free to implement it and I'll
> > happily use it.
> 
> Fair enough; I'll give it a shot.
> 
> > But the hash table is simple and works for this use case, so I see
> > little reason to invest a lot of time in something that we haven't ever
> > had another user for.
> >
> >> Are we going to create another hash table whenever we need another piece
> >> of coroutine-local data?  Or shall we reuse the hash table, suitably
> >> renamed and moved to another file?
> >
> > I think I would vote for separate hash tables rather than having a hash
> > table containing a struct that mixes values from all subsystems, but
> > this can be discussed when (if) the need arises.
> >
> >> Why not simply associate an opaque pointer with each coroutine?  All it
> >> takes is one more member of struct Coroutine.  Whatever creates the
> >> coroutine decides what to use it for.  The monitor coroutine would use
> >> it to point to the monitor.
> >
> > This doesn't work. error_report() is called from all kinds of
> > coroutines, not just from coroutines created from the monitor, and it
> > wants to know the current monitor.
> 
> Yup, monitor_cur() and monitor_set_cur() need to work both in coroutine
> context and outside coroutine context.

And in coroutine contexts, but in coroutine created by someone else than
the monitor.

> >> At least, discuss the design alternatives in the commit message.
> >
> > *sigh* Fine. Tell me which set of alternatives to discuss.
> 
> Let me first play with the alternative I suggested.
> 
> >> > Signed-off-by: Kevin Wolf 
> >> > ---
> >> >  include/monitor/monitor.h |  2 +-
> >> >  monitor/hmp.c |  4 ++--
> >> >  monitor/monitor.c | 27 +--
> >> >  qapi/qmp-dispatch.c   |  4 ++--
> >> >  stubs/monitor-core.c  |  2 +-
> >> >  5 files changed, 27 insertions(+), 12 deletions(-)
> >> >
> >> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >> > index 43cc746078..16072e325c 100644
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
> >> >  extern QemuOptsList qemu_mon_opts;
> >> >  
> >> >  Monitor *monitor_cur(void);
> >> > -void monitor_set_cur(Monitor *mon);
> >> > +void monitor_set_cur(Coroutine *co, Monitor *mon);
> >> >  bool monitor_cur_is_qmp(void);
> >> >  
> >> >  void monitor_init_globals(void);
> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> >> > index 79be6f26de..3e73a4c3ce 100644
> >> > --- a/monitor/hmp.c
> >> > +++ b/monitor/hmp.c
> >> > @@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const 
> >> > char *cmdline)
> >> >  
> >> >  /* old_mon is non-NULL when called from qmp_human_monitor_command() 
> >> > */
> >> >  old_mon = monitor_cur();
> >> > -monitor_set_cur(&mon->common);
> >> > +monitor_set_cur(qemu_coroutine_self(), &mon->common);
> >> >  cmd->cmd(&mon->common, qdict);
> >> > -monitor_set_cur(old_mon);
> >> > +monitor_set_cur(qemu_coroutine_self(), old_mon);
> >> >  
> >> >  qobject_unref(qdict);
> >> >  }
> >> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> >> > index 182ba136b4..35003bb486 100644
> >> > --- a/monitor/monitor.c
> >> > +++ b/monitor/monitor.c
> >> > @@ -58,24 +58,38 @@ IOThread *mon_iothread;
> >> >  /* Bottom half to dispatch the requests received from I/O thread */
> >> >  QEMUBH *qmp_dispatcher_bh;
> >> >  
> >> > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
> >> > +/*
> >> > + * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> >> > + * monitor_destroyed.
> >> > + */
> >> >  QemuMutex monitor_lock;
> >> >  static GHashTable *monitor_qapi_event_state;
> >> > +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
> >> >  
> >> >  MonitorList mon_list;
> 

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Kevin Wolf]

Am 05.08.2020 um 09:19 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> >> > index d598dd02bb..f609fcf75b 100644
> >> > --- a/monitor/hmp.c
> >> > +++ b/monitor/hmp.c
> >> > @@ -1301,11 +1301,11 @@ cleanup:
> >> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
> >> >  {
> >> >  MonitorHMP *mon;
> >> > -Monitor *old_mon = cur_mon;
> >> > +Monitor *old_mon = monitor_cur();
> >> >  int i;
> >> >  
> >> > -cur_mon = opaque;
> >> > -mon = container_of(cur_mon, MonitorHMP, common);
> >> > +monitor_set_cur(opaque);
> >> > +mon = container_of(monitor_cur(), MonitorHMP, common);
> >> 
> >> Simpler:
> >> 
> >>MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
> >
> > opaque is void*, so it doesn't have a field 'common'.
> 
> I actually compile-tested before I sent this.  For once ;)
> 
> Here's container_of():
> 
> #define container_of(ptr, type, member) ({  \
> const typeof(((type *) 0)->member) *__mptr = (ptr); \
> (type *) ((char *) __mptr - offsetof(type, member));})
> 
> Its first argument's only use is as an initializer for a pointer
> variable.  Both type * and void * work fine there.

Ah, we just lose type checking.

That's what I get for replying from what I remember from over two months
ago. I was pretty sure I didn't like this way, but went with it because
the other way didn't work. Maybe I just assumed it didn't work, or tried
something different that actually fails. Who knows.

Kevin

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Paolo Bonzini]

On 05/08/20 09:36, Markus Armbruster wrote:
> There's also the longer term pain of having to work around git-blame
> unable to see beyond the flag day.

Do you really use "git blame" that much?  "git log -S" does more or less
the same function (in a different way) and is not affected as much by
large code movement and transformation patches.

Paolo

Re: [PATCH 0/2] qcow2: Release read-only bitmaps when inactivated

[Vladimir Sementsov-Ogievskiy]

30.07.2020 15:02, Max Reitz wrote:

Hi,

When beginning migration, the qcow2 driver syncs all persistent bitmaps
to disk and then releases them.  If the user decides to continue on the
source after migration, those bitmaps are re-loaded from the qcow2
image.

However, we only do this for bitmaps that were actively synced, i.e. R/W
bitmaps.  RO bitmaps (those on backing images) are not written and thus
not released.  However, we still try to re-load them when continuing,
and that will then fail.

To fix this problem, I think we should just consider RO bitmaps to be in
sync from the beginning, so we can release them just like bitmaps that
we have actively written back to the image.  This is done by patch 1.

However, there’s a catch: Peter Krempa noted that it isn’t in libvirt’s
interest for the bitmaps to be released before migration at all, because
this makes them disappear from query-named-block-node’s dirty-bitmaps
list, but libvirt needs the bitmaps to be there:

https://bugzilla.redhat.com/show_bug.cgi?id=1858739#c3

If it’s really not feasible to keep the bitmaps around, then I suppose
what might work for libvirt is to query
image/format-specific/data/bitmaps in addition to dirty-bitmaps (every
bitmap that we released before migration must be a persistent bitmap).

What are your thoughts on this?


Sorry, I was on vocation for a week, so missed this. I see, it was merged, 
still, I'll look through it now.

Hmm.

1. Why we sync bitmaps on inactivation: it's obvious, it's the last point when 
it is possible to store them

2. Why we release bitmaps on inactivation: after inactivation, the bitmaps are 
not owned by Qemu. The image is not locked, someone other can change persistent 
bitmaps (target Qemu for example)..




Max Reitz (2):
   qcow2: Release read-only bitmaps when inactivated
   iotests/169: Test source cont with backing bmap

  block/qcow2-bitmap.c   | 23 +++---
  tests/qemu-iotests/169 | 64 +-
  tests/qemu-iotests/169.out |  4 +--
  3 files changed, 84 insertions(+), 7 deletions(-)




--
Best regards,
Vladimir

Re: [PATCH v11 09/11] qcow2_format.py: collect fields to dump in JSON format

[Vladimir Sementsov-Ogievskiy]

29.07.2020 08:56, Andrey Shinkevich wrote:

On 28.07.2020 14:09, Vladimir Sementsov-Ogievskiy wrote:

17.07.2020 11:14, Andrey Shinkevich wrote:

As __dict__ is being extended with class members we do not want to
print, add the to_dict() method to classes that returns a dictionary
with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
---
  tests/qemu-iotests/qcow2_format.py | 38 ++
  1 file changed, 38 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2921a27..19d29b8 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py

...

    class Qcow2BitmapDirEntry(Qcow2Struct):
  @@ -190,6 +198,13 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
  super(Qcow2BitmapDirEntry, self).dump()
  self.bitmap_table.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    fields_dict.update(bitmap_table=self.bitmap_table)
+    bmp_name = dict(name=self.name)
+    bme_dict = {**bmp_name, **fields_dict}


hmmm... I don't follow, why not simply

   fields_dict = super().to_dict()
   fields_dict['name'] = self.name
   fields_dict['bitmap_table'] = self.bitmap_table
   ?



The idea is to put the name ahead of the dict. It is the same to 
QcowHeaderExtension::to_dict(). The relevant comment will be supplied in the 
code.


Not worth doing. Json is not human output, it's mostly for parsing, so using so 
hard magic in the code to sort fields as you want is not worth doing. And I'm 
not sure how much is it guaranteed to keep some ordering of dict fields, why 
can't it change from version to version?



The .update() will be replaced with the assignment operator.

Andrey





+    return bme_dict
+

...

@@ -303,6 +327,17 @@ class QcowHeaderExtension(Qcow2Struct):
  else:
  self.obj.dump()
  +    def to_dict(self):
+    fields_dict = super().to_dict()
+    ext_name = dict(name=self.Magic(self.magic))
+    he_dict = {**ext_name, **fields_dict}


again, why not just add a field to fields_dict ?


+    if self.obj is not None:
+    he_dict.update(data=self.obj)
+    else:
+    he_dict.update(data_str=self.data_str)
+
+    return he_dict
+

...



--
Best regards,
Vladimir

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Vladimir Sementsov-Ogievskiy]

28.07.2020 19:05, Nir Soffer wrote:

On Tue, Jul 28, 2020 at 4:43 PM Vladimir Sementsov-Ogievskiy
 wrote:


28.07.2020 00:58, Nir Soffer wrote:

Instead of duplicating the code to wait until the server is ready and
remember to terminate the server and wait for it, make it possible to
use like this:

  with qemu_nbd_popen('-k', sock, image):
  # Access image via qemu-nbd socket...

Only test 264 used this helper, but I had to modify the output since it
did not consistently when starting and stopping qemu-nbd.

Signed-off-by: Nir Soffer 
---
   tests/qemu-iotests/264| 76 +--
   tests/qemu-iotests/264.out|  2 +
   tests/qemu-iotests/iotests.py | 28 -
   3 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
index 304a7443d7..666f164ed8 100755
--- a/tests/qemu-iotests/264
+++ b/tests/qemu-iotests/264
@@ -36,48 +36,32 @@ wait_step = 0.2

   qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
   qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
-srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)

-# Wait for NBD server availability
-t = 0
-ok = False
-while t < wait_limit:
-ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
-if ok:
-break
-time.sleep(wait_step)
-t += wait_step
+with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
+vm = iotests.VM().add_drive(disk_a)
+vm.launch()
+vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
+
+vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
+   **{'node_name': 'backup0',
+  'driver': 'raw',
+  'file': {'driver': 'nbd',
+   'server': {'type': 'unix', 'path': nbd_sock},
+   'reconnect-delay': 10}})
+vm.qmp_log('blockdev-backup', device='drive0', sync='full', 
target='backup0',
+   speed=(1 * 1024 * 1024))
+
+# Wait for some progress
+t = 0
+while t < wait_limit:
+jobs = vm.qmp('query-block-jobs')['return']
+if jobs and jobs[0]['offset'] > 0:
+break
+time.sleep(wait_step)
+t += wait_step

-assert ok
-
-vm = iotests.VM().add_drive(disk_a)
-vm.launch()
-vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
-
-vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
-   **{'node_name': 'backup0',
-  'driver': 'raw',
-  'file': {'driver': 'nbd',
-   'server': {'type': 'unix', 'path': nbd_sock},
-   'reconnect-delay': 10}})
-vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
-   speed=(1 * 1024 * 1024))
-
-# Wait for some progress
-t = 0
-while t < wait_limit:
-jobs = vm.qmp('query-block-jobs')['return']
   if jobs and jobs[0]['offset'] > 0:
-break
-time.sleep(wait_step)
-t += wait_step
-
-if jobs and jobs[0]['offset'] > 0:
-log('Backup job is started')
-
-log('Kill NBD server')
-srv.kill()
-srv.wait()
+log('Backup job is started')

   jobs = vm.qmp('query-block-jobs')['return']
   if jobs and jobs[0]['offset'] < jobs[0]['len']:
@@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
   # Emulate server down time for 1 second
   time.sleep(1)

-log('Start NBD server')
-srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
-
-e = vm.event_wait('BLOCK_JOB_COMPLETED')
-log('Backup completed: {}'.format(e['data']['offset']))
-
-vm.qmp_log('blockdev-del', node_name='backup0')
-srv.kill()
-vm.shutdown()
+with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
+e = vm.event_wait('BLOCK_JOB_COMPLETED')
+log('Backup completed: {}'.format(e['data']['offset']))
+vm.qmp_log('blockdev-del', node_name='backup0')
+vm.shutdown()
diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
index 3000944b09..c45b1e81ef 100644
--- a/tests/qemu-iotests/264.out
+++ b/tests/qemu-iotests/264.out
@@ -1,3 +1,4 @@
+Start NBD server
   {"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": 
"TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
   {"return": {}}
   {"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": 
"full", "target": "backup0"}}
@@ -11,3 +12,4 @@ Start NBD server
   Backup completed: 5242880
   {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
   {"return": {}}
+Kill NBD server
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3590ed78a0..8f79668435 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,10 +28,13 @@ import signal
   import struct
   import subprocess
   import sys
+import time
   from typing import (Any, Callable, Dict, Iterable,
   L

Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Markus Armbruster]

John Snow  writes:

> On 8/4/20 4:03 AM, Markus Armbruster wrote:
>> The pain of tweaking the parser is likely dwarved several times over by
>> the pain of the flag day.
>
> You mention this often; I wonder if I misunderstand the critique,
> because the pain of a "flag day" for a new file format seems
> negligible to me.
>
> I don't think we edit these .json files very often. Generally, we add
> a new command when we need one. The edits are usually one or two lines
> plus docstrings.
>
> If anyone has patches in-flight, I genuinely doubt it will take more
> than a few minutes to rewrite for the new file format.
>
> No?

You describe the the flag day's one-time pain.

There's also the longer term pain of having to work around git-blame
unable to see beyond the flag day.

I'm not claiming the pain is prohibitive (if I thought it was, I
would've tried to strange this thread in its crib), I am claiming it'll
be much more painful (read: expensive) than a parser tweak.

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Markus Armbruster]

Kevin Wolf  writes:

> Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > This way, a monitor command handler will still be able to access the
>> > current monitor, but when it yields, all other code code will correctly
>> > get NULL from monitor_cur().
>> >
>> > Outside of coroutine context, qemu_coroutine_self() returns the leader
>> > coroutine of the current thread.
>> 
>> Unsaid: you use it as a hash table key to map from coroutine to monitor,
>> and for that you need it to return a value unique to the coroutine in
>> coroutine context, and a value unique to the thread outside coroutine
>> context.  Which qemu_coroutine_self() does.  Correct?
>
> Correct.
>
>> The hash table works, but I hate it just as much as I hate
>> pthread_getspecific() / pthread_setspecific().
>> 
>> What we have here is a need for coroutine-local data.  Feels like a
>> perfectly natural concept to me.
>
> If you have a good concept how to implement this in a generic way that
> doesn't impact the I/O fast path, feel free to implement it and I'll
> happily use it.

Fair enough; I'll give it a shot.

> But the hash table is simple and works for this use case, so I see
> little reason to invest a lot of time in something that we haven't ever
> had another user for.
>
>> Are we going to create another hash table whenever we need another piece
>> of coroutine-local data?  Or shall we reuse the hash table, suitably
>> renamed and moved to another file?
>
> I think I would vote for separate hash tables rather than having a hash
> table containing a struct that mixes values from all subsystems, but
> this can be discussed when (if) the need arises.
>
>> Why not simply associate an opaque pointer with each coroutine?  All it
>> takes is one more member of struct Coroutine.  Whatever creates the
>> coroutine decides what to use it for.  The monitor coroutine would use
>> it to point to the monitor.
>
> This doesn't work. error_report() is called from all kinds of
> coroutines, not just from coroutines created from the monitor, and it
> wants to know the current monitor.

Yup, monitor_cur() and monitor_set_cur() need to work both in coroutine
context and outside coroutine context.

>> At least, discuss the design alternatives in the commit message.
>
> *sigh* Fine. Tell me which set of alternatives to discuss.

Let me first play with the alternative I suggested.

>> > Signed-off-by: Kevin Wolf 
>> > ---
>> >  include/monitor/monitor.h |  2 +-
>> >  monitor/hmp.c |  4 ++--
>> >  monitor/monitor.c | 27 +--
>> >  qapi/qmp-dispatch.c   |  4 ++--
>> >  stubs/monitor-core.c  |  2 +-
>> >  5 files changed, 27 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index 43cc746078..16072e325c 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
>> >  extern QemuOptsList qemu_mon_opts;
>> >  
>> >  Monitor *monitor_cur(void);
>> > -void monitor_set_cur(Monitor *mon);
>> > +void monitor_set_cur(Coroutine *co, Monitor *mon);
>> >  bool monitor_cur_is_qmp(void);
>> >  
>> >  void monitor_init_globals(void);
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index 79be6f26de..3e73a4c3ce 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const char 
>> > *cmdline)
>> >  
>> >  /* old_mon is non-NULL when called from qmp_human_monitor_command() */
>> >  old_mon = monitor_cur();
>> > -monitor_set_cur(&mon->common);
>> > +monitor_set_cur(qemu_coroutine_self(), &mon->common);
>> >  cmd->cmd(&mon->common, qdict);
>> > -monitor_set_cur(old_mon);
>> > +monitor_set_cur(qemu_coroutine_self(), old_mon);
>> >  
>> >  qobject_unref(qdict);
>> >  }
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 182ba136b4..35003bb486 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -58,24 +58,38 @@ IOThread *mon_iothread;
>> >  /* Bottom half to dispatch the requests received from I/O thread */
>> >  QEMUBH *qmp_dispatcher_bh;
>> >  
>> > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
>> > +/*
>> > + * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
>> > + * monitor_destroyed.
>> > + */
>> >  QemuMutex monitor_lock;
>> >  static GHashTable *monitor_qapi_event_state;
>> > +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>> >  
>> >  MonitorList mon_list;
>> >  int mon_refcount;
>> >  static bool monitor_destroyed;
>> >  
>> > -static __thread Monitor *cur_monitor;
>> > -
>> >  Monitor *monitor_cur(void)
>> >  {
>> > -return cur_monitor;
>> > +Monitor *mon;
>> > +
>> > +qemu_mutex_lock(&monitor_lock);
>> > +mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
>> > +qemu_mutex_unlock(&monitor_lock);
>> > +

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Markus Armbruster]

Kevin Wolf  writes:

> Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index d598dd02bb..f609fcf75b 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -1301,11 +1301,11 @@ cleanup:
>> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
>> >  {
>> >  MonitorHMP *mon;
>> > -Monitor *old_mon = cur_mon;
>> > +Monitor *old_mon = monitor_cur();
>> >  int i;
>> >  
>> > -cur_mon = opaque;
>> > -mon = container_of(cur_mon, MonitorHMP, common);
>> > +monitor_set_cur(opaque);
>> > +mon = container_of(monitor_cur(), MonitorHMP, common);
>> 
>> Simpler:
>> 
>>MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
>
> opaque is void*, so it doesn't have a field 'common'.

I actually compile-tested before I sent this.  For once ;)

Here's container_of():

#define container_of(ptr, type, member) ({  \
const typeof(((type *) 0)->member) *__mptr = (ptr); \
(type *) ((char *) __mptr - offsetof(type, member));})

Its first argument's only use is as an initializer for a pointer
variable.  Both type * and void * work fine there.

>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 125494410a..182ba136b4 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -66,13 +66,24 @@ MonitorList mon_list;
>> >  int mon_refcount;
>> >  static bool monitor_destroyed;
>> >  
>> > -__thread Monitor *cur_mon;
>> > +static __thread Monitor *cur_monitor;
>> > +
>> > +Monitor *monitor_cur(void)
>> > +{
>> > +return cur_monitor;
>> > +}
>> > +
>> > +void monitor_set_cur(Monitor *mon)
>> > +{
>> > +cur_monitor = mon;
>> > +}
>> 
>> All uses of monitor_set_cur() look like this:
>> 
>> old_mon = monitor_cur();
>> monitor_set_cur(new_mon);
>> ...
>> monitor_set_cur(old_mon);
>> 
>> If we let monitor_set_cur() return the old value, this becomes
>> 
>> old_mon = monitor_set_cur(new_mon);
>> ...
>> monitor_set_cur(old_mon);
>> 
>> I like this better.
>
> Fine with me.
>
>> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
>> > index 6cff1c4e1d..0cd2d864b2 100644
>> > --- a/stubs/monitor-core.c
>> > +++ b/stubs/monitor-core.c
>> > @@ -3,7 +3,10 @@
>> >  #include "qemu-common.h"
>> >  #include "qapi/qapi-emit-events.h"
>> >  
>> > -__thread Monitor *cur_mon;
>> > +Monitor *monitor_cur(void)
>> > +{
>> > +return NULL;
>> > +}
>> 
>> Is this meant to be called?  If not, abort().
>
> error_report() and friends are supposed to be called pretty much
> everywhere, so I'd say yes.

Okay.