Re: [GSoC][PATCH v2 3/6] qemu_domainjob: `maxQueuedJobs` added to `qemuDomainJobPrivate`
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(a)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.