wido commented on code in PR #6909:
URL: https://github.com/apache/cloudstack/pull/6909#discussion_r1056194243
##########
agent/src/main/java/com/cloud/agent/properties/AgentProperties.java:
##########
@@ -69,6 +69,13 @@
*/
public static final Property<Integer> VM_MEMBALLOON_STATS_PERIOD = new
Property<>("vm.memballoon.stats.period", 0);
+ /**
+ * The number of iothreads
+ * Data type: Integer.<br>
+ * Default value: <code>0</code>
Review Comment:
This says default is 0, but the line below (Java code) shows that we set it
to 1 by default.
##########
agent/conf/agent.properties:
##########
@@ -295,3 +295,6 @@ iscsi.session.cleanup.enabled=false
# Enable/disable IO driver for Qemu (in case it is not set CloudStack can also
detect if its supported by qemu)
# enable.io.uring=true
+
+# The number of iothreads. There should be only 1 or 2 IOThreads per host CPU
(default is 1)
Review Comment:
Should we add a comment here that this is applied per VM and not per host?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java:
##########
@@ -1883,12 +1909,14 @@ public String toString() {
}
public static class SCSIDef {
+
Review Comment:
Empty line
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java:
##########
@@ -761,6 +776,14 @@ public void setDeviceType(DeviceType deviceType) {
_deviceType = deviceType;
}
+ public boolean getIothreads() {
Review Comment:
This returns a boolean, but to me it implies a number which it will return.
Is this method even ever called?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java:
##########
@@ -105,6 +106,7 @@ public String toString() {
private String _machine;
private String _nvram;
private String _nvramTemplate;
+ private String iothreads;
Review Comment:
Shouldn't this be a int and casted to a string only at the latest moment?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3185,12 +3199,32 @@ protected String getIoUringCheckCommand() {
* (i) Qemu >= 5.0;
* (ii) Libvirt >= 6.3.0
*/
- protected void setDiskIoDriver(DiskDef disk) {
- if (enableIoUring) {
+ public void setDiskIoDriver(DiskDef disk, boolean iothreadsEnabled,
IoDriver ioDriver) {
+ if (iothreadsEnabled) {
+ disk.setIothreads(iothreadsEnabled);
+ if (!enableIoUring && IoDriver.IOURING == ioDriver) {
Review Comment:
Should we maybe remove this check for iouring? That this PR simply allows
you to enable io_uring per VM/Storage pool? Remove the agent.properties setting.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3185,12 +3199,32 @@ protected String getIoUringCheckCommand() {
* (i) Qemu >= 5.0;
* (ii) Libvirt >= 6.3.0
*/
- protected void setDiskIoDriver(DiskDef disk) {
- if (enableIoUring) {
+ public void setDiskIoDriver(DiskDef disk, boolean iothreadsEnabled,
IoDriver ioDriver) {
+ if (iothreadsEnabled) {
+ disk.setIothreads(iothreadsEnabled);
+ if (!enableIoUring && IoDriver.IOURING == ioDriver) {
+ //setting io=threads as default
+ disk.setIoDriver(IoDriver.THREADS);
+ } else {
+ disk.setIoDriver(ioDriver);
+ }
+ } else if (enableIoUring) {
disk.setIoDriver(DiskDef.IoDriver.IOURING);
Review Comment:
Applies here as well. Try to make this uniform and have it work the same is
iothreads now works?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
