----- Original Message -----
> From: "Stefan Hajnoczi" <stefa...@redhat.com>
> To: "Francesco Romani" <from...@redhat.com>
> Cc: kw...@redhat.com, "Stefan Hajnoczi" <stefa...@gmail.com>,
> mdr...@linux.vnet.ibm.com, qemu-devel@nongnu.org,
> lcapitul...@redhat.com
> Sent: Wednesday, November 19, 2014 4:52:51 PM
> Subject: Re: [Qemu-devel] [RFC][PATCH v2] block: add write threshold
> reporting for block devices
>
> On Tue, Nov 18, 2014 at 03:12:12AM -0500, Francesco Romani wrote:
> > > > +static int coroutine_fn before_write_notify(NotifierWithReturn
> > > > *notifier,
> > > > + void *opaque)
> > > > +{
> > > > + BdrvTrackedRequest *req = opaque;
> > > > + BlockDriverState *bs = req->bs;
> > > > + int64_t amount = 0;
> > > > +
> > > > + assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > > > + assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > >
> > > Does the code still make these assumptions or are the asserts left over
> > > from previous versions of the patch?
> >
> > It's a leftover.
> > I understood they don't hurt and add a bit of safety, but if they are
> > confusing
> > I'll remove them.
>
> Yes, it made me wonder why. Probably best to remove them.
Will do
[...]
> > At risk of being redundant, in oVirt the usecase is QCOW2 over lvm block
> > device,
> > and we'd like to be notified about the allocation of the lvm block device,
> > which (IIUC)
> > is the last bs->file.
> >
> > This is a simple topology (unless I'm missing something), and that's
> > the reason why I go down just one level.
> >
> > Of course I want a general solution for this change, so...
>
> There is a block driver for error injection called "blkdebug" (see
> docs/blkdebug.txt). Here is an example of the following topology:
>
> raw_bsd (drive0) -> blkdebug -> raw-posix (test.img)
>
> qemu-system-x86_64 -drive
> if=virtio,format=raw,file.driver=blkdebug,file.image.filename=test.img
>
> The blkdebug driver is interposing between the raw_bsd (drive0) root and
> the raw-posix leaf node.
Thanks, I'll have a look
[...]
> The management tool should not need to inspect the graph because the
> graph can change at runtime (e.g. imagine I/O throttling is implemented
> as a BlockDriverState node then it could appear/disapper when the
> feature is activated/deactivated). Instead the management tool should
> name the nodes it knows about and then use those node names.
Agreed - and indeed simpler for us (oVirt), which it doesn't hurt :)
> > If we descend the bs->file chain, AFAIU the easiest mapping, being the
> > device name,
> > is easily lost because only the outermost BlockDriverState has a device
> > name attached, so when the
> > notification trigger
> > bdrv_get_device_name() will return NULL
>
> In the worst case a name string can be passed in along with the
> threshold values.
OK, I guess to keep a copy of the string with g_strdup() could be good enough
start,
at least for further discussion.
Thanks for your review and for the informations,
I'll submit a new revision of the patch in a couple of days,
to give to other reviewers some time to jump in.
Bests,
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
