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@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(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1ae44ae39f..677fa7ea91 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2085,11 +2085,14 @@ static void *
 qemuDomainObjPrivateAlloc(void *opaque)
 {
     qemuDomainObjPrivatePtr priv;
+    virQEMUDriverPtr driver = opaque;
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 
     if (VIR_ALLOC(priv) < 0)
         return NULL;
 
-    if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) {
+    if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks,
+                             cfg->maxQueuedJobs) < 0) {
         virReportSystemError(errno, "%s",
                              _("Unable to init qemu driver mutexes"));
         goto error;
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index 7cd1aabd9e..eebc144747 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -117,10 +117,12 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
 
 int
 qemuDomainObjInitJob(qemuDomainJobObjPtr job,
-                     qemuDomainObjPrivateJobCallbacksPtr cb)
+                     qemuDomainObjPrivateJobCallbacksPtr cb,
+                     unsigned int maxQueuedJobs)
 {
     memset(job, 0, sizeof(*job));
     job->cb = cb;
+    job->maxQueuedJobs = maxQueuedJobs;
 
     if (!(job->privateData = job->cb->allocJobPrivate()))
         return -1;
@@ -344,7 +346,6 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
     unsigned long long then;
     bool nested = job == QEMU_JOB_ASYNC_NESTED;
     bool async = job == QEMU_JOB_ASYNC;
-    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
     const char *blocker = NULL;
     const char *agentBlocker = NULL;
     int ret = -1;
@@ -370,8 +371,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
 
  retry:
     if ((!async && job != QEMU_JOB_DESTROY) &&
-        cfg->maxQueuedJobs &&
-        priv->job.jobs_queued > cfg->maxQueuedJobs) {
+        priv->job.maxQueuedJobs &&
+        priv->job.jobs_queued > priv->job.maxQueuedJobs) {
         goto error;
     }
 
@@ -501,8 +502,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
                            _("cannot acquire state change lock"));
         }
         ret = -2;
-    } else if (cfg->maxQueuedJobs &&
-               priv->job.jobs_queued > cfg->maxQueuedJobs) {
+    } else if (priv->job.maxQueuedJobs &&
+               priv->job.jobs_queued > priv->job.maxQueuedJobs) {
         if (blocker && agentBlocker) {
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("cannot acquire state change "
diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h
index 0696b79fe3..11e7f2f432 100644
--- a/src/qemu/qemu_domainjob.h
+++ b/src/qemu/qemu_domainjob.h
@@ -153,6 +153,7 @@ struct _qemuDomainJobObj {
     unsigned long apiFlags; /* flags passed to the API which started the async job */
 
     int jobs_queued;
+    unsigned int maxQueuedJobs;
 
     void *privateData;                  /* job specific collection of data */
     qemuDomainObjPrivateJobCallbacksPtr cb;
@@ -215,7 +216,8 @@ void qemuDomainObjFreeJob(qemuDomainJobObjPtr job);
 
 int
 qemuDomainObjInitJob(qemuDomainJobObjPtr job,
-                     qemuDomainObjPrivateJobCallbacksPtr cb);
+                     qemuDomainObjPrivateJobCallbacksPtr cb,
+                     unsigned int maxQueuedJobs);
 
 bool qemuDomainJobAllowed(qemuDomainJobObjPtr jobs, qemuDomainJob newJob);
 
-- 
2.25.1

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@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(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 1ae44ae39f..677fa7ea91 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2085,11 +2085,14 @@ static void *
>  qemuDomainObjPrivateAlloc(void *opaque)
>  {
>      qemuDomainObjPrivatePtr priv;
> +    virQEMUDriverPtr driver = opaque;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>
>      if (VIR_ALLOC(priv) < 0)
>          return NULL;
>
> -    if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks) < 0) {
> +    if (qemuDomainObjInitJob(&priv->job, &qemuPrivateJobCallbacks,
> +                             cfg->maxQueuedJobs) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Unable to init qemu driver mutexes"));
>          goto error;
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index 7cd1aabd9e..eebc144747 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
> @@ -117,10 +117,12 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
>
>  int
>  qemuDomainObjInitJob(qemuDomainJobObjPtr job,
> -                     qemuDomainObjPrivateJobCallbacksPtr cb)
> +                     qemuDomainObjPrivateJobCallbacksPtr cb,
> +                     unsigned int maxQueuedJobs)
>  {
>      memset(job, 0, sizeof(*job));
>      job->cb = cb;
> +    job->maxQueuedJobs = maxQueuedJobs;
>
>      if (!(job->privateData = job->cb->allocJobPrivate()))
>          return -1;
> @@ -344,7 +346,6 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>      unsigned long long then;
>      bool nested = job == QEMU_JOB_ASYNC_NESTED;
>      bool async = job == QEMU_JOB_ASYNC;
> -    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>      const char *blocker = NULL;
>      const char *agentBlocker = NULL;
>      int ret = -1;
> @@ -370,8 +371,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>
>   retry:
>      if ((!async && job != QEMU_JOB_DESTROY) &&
> -        cfg->maxQueuedJobs &&
> -        priv->job.jobs_queued > cfg->maxQueuedJobs) {
> +        priv->job.maxQueuedJobs &&
> +        priv->job.jobs_queued > priv->job.maxQueuedJobs) {
>          goto error;
>      }
>
> @@ -501,8 +502,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>                             _("cannot acquire state change lock"));
>          }
>          ret = -2;
> -    } else if (cfg->maxQueuedJobs &&
> -               priv->job.jobs_queued > cfg->maxQueuedJobs) {
> +    } else if (priv->job.maxQueuedJobs &&
> +               priv->job.jobs_queued > priv->job.maxQueuedJobs) {
>          if (blocker && agentBlocker) {
>              virReportError(VIR_ERR_OPERATION_FAILED,
>                             _("cannot acquire state change "
> diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h
> index 0696b79fe3..11e7f2f432 100644
> --- a/src/qemu/qemu_domainjob.h
> +++ b/src/qemu/qemu_domainjob.h
> @@ -153,6 +153,7 @@ struct _qemuDomainJobObj {
>      unsigned long apiFlags; /* flags passed to the API which started the async job */
>
>      int jobs_queued;
> +    unsigned int maxQueuedJobs;
>
>      void *privateData;                  /* job specific collection of data */
>      qemuDomainObjPrivateJobCallbacksPtr cb;

The comment below applies to both 2/6 and 3/6.

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.

Erik

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@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].

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.

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@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.

On 8/19/20 2:00 PM, Erik Skultety wrote:
> 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@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.

Sure, we can probably find different location, but question then is how 
usable the APIs would be? My idea of a good API is like this:

job = initJob(parameters);  /* alternatively, initJob(&job, parameters); */

beginJob(&job, type);

/* do something useful */

endJob(&job);

If we'd have maxQueuedJobs separate, then we would have to pass it in 
beginJob(), right?

Michal

On Wed, Aug 19, 2020 at 05:04:58PM +0200, Michal Privoznik wrote:
> On 8/19/20 2:00 PM, Erik Skultety wrote:
> > 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@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.
>
> Sure, we can probably find different location, but question then is how
> usable the APIs would be? My idea of a good API is like this:
>
> job = initJob(parameters);  /* alternatively, initJob(&job, parameters); */
>
> beginJob(&job, type);
>
> /* do something useful */
>
> endJob(&job);
>
> If we'd have maxQueuedJobs separate, then we would have to pass it in
> beginJob(), right?

Naturally. In a standard OOP design, such information would come from the
object calling the beginJob() method, in our case this would most likely
translate to the domain object over which we're executing a job. We'd therefore
need the domain object as well.

I see what you mean, but a usable API is as important as logically structured
data the API takes.

Erik

Keeping the above arguments in mind, I think having callback functions
for accessing them instead of added them should do the work for now.

Thanks,
Prathamesh Chavan

On Wed, Aug 19, 2020 at 9:05 PM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Wed, Aug 19, 2020 at 05:04:58PM +0200, Michal Privoznik wrote:
> > On 8/19/20 2:00 PM, Erik Skultety wrote:
> > > 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@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.
> >
> > Sure, we can probably find different location, but question then is how
> > usable the APIs would be? My idea of a good API is like this:
> >
> > job = initJob(parameters);  /* alternatively, initJob(&job, parameters); */
> >
> > beginJob(&job, type);
> >
> > /* do something useful */
> >
> > endJob(&job);
> >
> > If we'd have maxQueuedJobs separate, then we would have to pass it in
> > beginJob(), right?
>
> Naturally. In a standard OOP design, such information would come from the
> object calling the beginJob() method, in our case this would most likely
> translate to the domain object over which we're executing a job. We'd therefore
> need the domain object as well.
>
> I see what you mean, but a usable API is as important as logically structured
> data the API takes.
>
> Erik
>