Re: [Qemu-block] [Qemu-devel] [PATCH] block: allow write-threshold on device name

[Kevin Wolf]

Am 10.06.2015 um 00:35 hat Eric Blake geschrieben:
> On 06/06/2015 07:38 PM, Eric Blake wrote:
> > Commit e2462113 allowed the ability to fire an event if a BDS
> > node exceeds a threshold during a write, but limited the option
> > to only work on node names.  For convenience, expand this to
> > allow a device name as a way to set the threshold on the BDS
> > at the active layer of the device.
> > 
> > Signed-off-by: Eric Blake <address@hidden>
> > ---
> >  block/write-threshold.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> 
> > @@ -110,9 +110,8 @@ void qmp_block_set_write_threshold(const char 
> > *node_name,
> >      BlockDriverState *bs;
> >      AioContext *aio_context;
> > 
> > -    bs = bdrv_find_node(node_name);
> > +    bs = bdrv_lookup_bs(node_name, node_name, errp);
> 
> Hmm, I'm not quite sure this does what I want.  If I understand
> correctly, when you open a qcow2 image, you get the following
> query-block layout (abbreviated):
> 
>             "inserted": {
> ...
>                 "image": {
> ...
>                     "backing-filename-format": "qcow2",
>                     "virtual-size": 12884901888,
>                     "filename": "/dev/sda6",
>                     "cluster-size": 65536,
>                     "format": "qcow2",
>                     "actual-size": 0,
> ...
>                 "drv": "qcow2",
>                 "iops": 0,
>                 "bps_wr": 0,
>                 "write_threshold": 0,
> ...
>                 "file": "/dev/sda6",
> 
> 
> That is, the only write_threshold reported is that of the active layer
> BDS (write_threshold of other BDS is reported through
> query-named-block-nodes, but only if those BDS have a name), but
> query-block is not listing any secondary BDS details.
> 
> Meanwhile, here is the corresponding query-blockstats layout:
> 
>             "device": "drive-virtio-disk0",
>             "parent": {
>                 "stats": {
>                     "flush_total_time_ns": 0,
>                     "wr_highest_offset": 72482304,
> ...
>             "stats": {
>                 "flush_total_time_ns": 728455560,
>                 "wr_highest_offset": 9129332224,
> 
> which DOES show the BDS chain; in particular, each qcow2 file has two
> BDS (one for the protocol, and the other ('parent') for the actual file).
> 
> The statistic I'm interested in is the allocation of the block device
> (the host offset, aka wr_highest_offset 72482304 above), and NOT the
> usage pattern of the guest (the qcow2 protocol, wr_highest_offset
> 9129332224).  But bdrv_lookup_bs() finds the qcow2 protocol layer,
> rather than the intended backing file layer; likewise, query-block is
> only reporting write_threshold for the protocol layer.
> 
> I'm wondering if, when a device name is given rather than a node name,
> it is safe to blindly follow the active layer down to its lowest member
> (or error out if there are more than one lower members, as in quorum),
> as that is the statistic that libvirt and upper layers really want ("am
> I about to exceed the allocation of my underlying storage?").  Likewise,
> on reporting, it is more useful to know the threshold of the backing
> layer if the qcow2 protocol layer does not have a threshold.  I'm
> playing with that idea before submitting a v2.

That is indeed what you need in your specific use case. However, qemu
shouldn't try to guess what management tools really want. It should
provide a clean interface that allows management tools to express
themselves what they want.

The cleanest interface that I can think of is that you access exactly
the node whose name you specified. If we do any magic like going down
the chain (which chain? What do you do with things like quorum in the
path?), we make the interface inconsistent and if anyone really wants to
know the highest offset that the guest accessed on its virtual disk, it
wouldn't even be possible any more because we said that that's not what
a management tool is interested in.

Let's stay away from such magic, as much as we can. libvirt can just
specify a node-name for the protocol layer and use that.

Kevin

pgpqqMRdXTCRO.pgp
Description: PGP signature