[libvirt] [PATCH v5] qemu: Introduce state_lock_timeout toqemu.conf

[wang.yi59 at zte.com.cn]

Thank you both for your patience and discussion, John and Michal.
I will send a new version to fix the issues referred in the discussion.

> On 09/10/2018 10:22 PM, John Ferlan wrote:
> >
> >
> > On 09/05/2018 11:09 PM, Yi Wang wrote:
> >> When doing some job holding state lock for a long time,
> >> we may come across error:
> >
> > blank line
> >
> >> "Timed out during operation: cannot acquire state change lock"
> >
> > blank line
> >
> >> Well, sometimes it's not a problem and users want to continue
> >> to wait, and this patch allow users decide how long time they
> >> can wait the state lock.
> >
> > As I noted in a previous review... With this patch it's not the
> > individual user or command that can decide how long to wait, rather it's
> > set by qemu.conf policy. The duration of the wait just becomes
> > customize-able. Not that it's any different the current implementation,
> > since the value is set in qemuDomainObjBeginJobInternal that means
> > there's no distinguishing between job, asyncjob, and agentjob. There's
> > also no attempt to allow changing the value via virt-admin.
>
> I'm not sure how we would implement per API timeout. We could perhaps do
> per connection but that's also not desired.
>
> And regarding "how does one know how long to wait" - well, they don't.
> That's the thing with timeouts. It's not an argument against them
> though. Even primitive functions (e.g. those from pthread) have
> XXX_timed() variants.
>
> >
> > A "strong recommendation" of longer than 30 seconds doesn't provide much
> > guidance. The setting of value generally depends on the configuration
> > and what causes an API to hold the lock for longer than 30 seconds.
> > That's probably limited to primarily virDomainListGetStats and/or
> > virConnectGetAllDomainStats. There's other factors in play including CPU
> > speed, CPU quota allowed, network throughput/latency, disk speed, etc.
> > Tough to create a value the works for every command/purpose.
>
> Agreed, "strong recommendation" is use case dependant (although it's
> perhaps what initiated this patch). If anything, we should discourage
> people from touching this knob.
>
> >
> > Ironically (perhaps) allowing a value of 0 would essentially make it so
> > no one waited - such as how qemuDomainObjBeginJobNowait operates
> > essentially. OTOH, in order to have everything wait "as long as
> > possible" passing -1 would be a lot easier than providing 2147483647 (or
> > if properly typed to uint, then 4294967295U).
>
> Passing 0 is explicitly forbidden (although in a bit clumsy way - we
> need to check the value right after it's parsed because multiplying it
> by 1000 can lead to overflow and a negative value becomes positive or
> vice versa).

[......]

> > If this were an unsigned int, then the < 0 check wouldn't necessary.
> >
> > So then the question becomes, should 0 (zero) be special cased as wait
> > forever.
>
> This check needs to be done before multiplication otherwise an overflow
> might occur. Also, the multiplication can be done in BeginJobInternal so
> that cfg->stateLockTimeout holds value entered by user.
>
> >
> >> +        virReportError(VIR_ERR_CONF_SYNTAX, "%s",
> >> +                       _("state_lock_timeout should larger than zero"));
> >
> > If 0 were left as invalid, then s/should/must be/
> >
> >> +        return -1;
> >> +    }
> >> +
> >>      return 0;
> >>  }
> >>
> Michal


---
Best wishes
Yi Wang