Re: [Qemu-devel] [RFC][PATCH v2] block: add write threshold reporting for block devices

[Francesco Romani]

----- Original Message -----
> From: "Stefan Hajnoczi" <address@hidden>
> To: "Francesco Romani" <address@hidden>
> Cc: address@hidden, "Stefan Hajnoczi" <address@hidden>, address@hidden, 
> address@hidden,
> address@hidden
> 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