[GSoC][PATCH v2 3/6] qemu_domainjob: `maxQueuedJobs` added to `qemuDomainJobPrivate`

[Erik Skultety]

On Wed, Aug 19, 2020 at 01:45:37PM +0200, Michal Privoznik wrote:
> On 8/18/20 10:48 AM, Erik Skultety wrote:
> > On Mon, Aug 17, 2020 at 10:37:18AM +0530, Prathamesh Chavan wrote:
> > > Reference to `maxQueuedJobs` required us to access
> > > config of the qemu-driver. And creating its copy in
> > > the `qemuDomainJob` helped us access the variable
> > > without referencing the driver's config.
> > >
> > > Signed-off-by: Prathamesh Chavan <pc44800 at gmail.com>
> > > ---
> > >   src/qemu/qemu_domain.c    |  5 ++++-
> > >   src/qemu/qemu_domainjob.c | 13 +++++++------
> > >   src/qemu/qemu_domainjob.h |  4 +++-
> > >   3 files changed, 14 insertions(+), 8 deletions(-)
> > >
>
>
> > Looking at this again, I don't think we want to move any of the jobs_queued or
> > maxQueuedJobs attributes to the job structure, I'm sorry I didn't spot this
> > right away in the previous revision of the series, I take responsibility for
> > it.
> > The job structure represents a single job, so from the design POV,
> > referencing both jobs_queued (which is really tied to a domain) and
> > maxQueuedJobs (which is a global driver setting) from within the job structure
> > simply doesn't feel right. Instead, I think qemuDomainObjBeginJobInternal will
> > simply have to be split in parts specific to QEMU that have access to the
> > driver and hypervisor-agnostic bits that can be moved to src/hypervisor.
>
> Actually, I think maxQueuedJobs is job specific and not a global driver
> setting. I mean, I can imagine other drivers benefiting from it too.
> It's true that we currently don't allow specifying different values for
> different domains and only apply the setting from qemu.conf, but in the
> future we might make this available in domain XML somehow [1] and thus be
> per domain. Moreover, I can imagine users wanting to set the limit for
> volumes too [2].

I can relate to the reasoning, but I still don't think the Job structure is the
right place, like I said, it holds everything that a single job needs, from
that POV a single job doesn't really need to do anything with that value, it's
just it happens to be a very convenient place right now, I say let's try
finding a different location for the attribute where it makes more sense.

Erik

>
> Michal
>
> 1: this refers to discussion we had on the list (too lazy to find the link,
> sorry) about the qemu:///embed. So far, if an user wants to set some values,
> they have to create qemu.conf in the root before connecting to the embed
> driver (so that the driver loads them). The idea was to allow them to
> specify everything in domain XML so no file needs to be created.
>
> 2: although, it's not as bad as it used to be, but with rotational disks
> having two processes accessing different portions of a file resulted in very
> poor performance as the head had to jump up & down.