On Sun, Aug 4, 2019 at 6:32 PM Erik Skultety <eskultet@xxxxxxxxxx> wrote:
>
> On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
> > ---
> >
> > I deliberately omitted the logic from qemuDomainSetBlockIoTuneDefaults
> > in order to leave the function simpler. I think that in the case of the
> > test driver it is ok.
> >
> > After all, how we handle the parameters it is supposed to be
> > hypervisor-specific.
> >
> > src/test/test_driver.c | 196 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 196 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 9f4e255e35..3272ecf4b7 100755
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3502,6 +3502,201 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> > }
> >
> >
> > +static int
> > +testDomainSetBlockIoTune(virDomainPtr dom,
> > + const char *path,
> > + virTypedParameterPtr params,
> > + int nparams,
> > + unsigned int flags)
> > +{
> > + virDomainObjPtr vm = NULL;
> > + virDomainDefPtr def = NULL;
> > + virDomainBlockIoTuneInfo info = {0};
> > + virDomainDiskDefPtr conf_disk = NULL;
> > + virTypedParameterPtr eventParams = NULL;
> > + int eventNparams = 0;
> > + int eventMaxparams = 0;
> > + size_t i;
> > + int ret = -1;
> > +
> > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > + VIR_DOMAIN_AFFECT_CONFIG, -1);
> > +
> > + if (virTypedParamsValidate(params, nparams,
> > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> > + VIR_TYPED_PARAM_STRING,
> > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH,
> > + VIR_TYPED_PARAM_ULLONG,
> > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,
> > + VIR_TYPED_PARAM_ULLONG,
> > + NULL) < 0)
> > + return -1;
> > +
> > + if (!(vm = testDomObjFromDomain(dom)))
> > + return -1;
> > +
> > + if (!(def = virDomainObjGetOneDef(vm, flags)))
> > + goto cleanup;
> > +
> > + if (!(conf_disk = virDomainDiskByName(def, path, true))) {
> > + virReportError(VIR_ERR_INVALID_ARG,
> > + _("missing persistent configuration for disk '%s'"),
> > + path);
> > + goto cleanup;
> > + }
> > +
> > + info = conf_disk->blkdeviotune;
> > + if (VIR_STRDUP(info.group_name, conf_disk->blkdeviotune.group_name) < 0)
> > + goto cleanup;
> > +
> > + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams,
> > + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
> > + goto cleanup;
> > +
> > +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \
>
> redundant BOOL argument...
>
> > + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \
> > + info.FIELD = param->value.ul; \
> > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \
> > + &eventMaxparams, \
> > + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \
>
> We've already discussed this and I believe we decided we're not going to
> oncatenate the enum names, so let's do it differently in the test driver.
Haha. Alright, so I believe that in 1/2 I thought that there is a
macro that we can re-use (TEST_SET_PARAM) and re-using stuff is good.
On the other hand, for 2/2 a new macro "had to" be introduced. I
copied the one from the QEMU implementation, and indeed the macro
string concatenation done here is particularly useful because it's
used twice and with different prefixes.
So if you also think that it's fine we can probably leave it as is
here? (by removing the redundant argument of course)
>
> > + param->value.ul) < 0) \
> > + goto cleanup; \
> > + continue; \
> > + }
> > +
> > + for (i = 0; i < nparams; i++) {
> > + virTypedParameterPtr param = ¶ms[i];
> > +
> > + if (param->value.ul > 1000000000000000LL) {
>
> I think we may want to define ^this as a macro constant for test driver as
> well.
Sure.
>
> > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> > + _("block I/O throttle limit value must"
> > + " be no more than %llu"), 1000000000000000LL);
> > + goto cleanup;
> > + }
> > +
> > + SET_IOTUNE_FIELD(total_bytes_sec, BYTES, TOTAL_BYTES_SEC);
> > + SET_IOTUNE_FIELD(read_bytes_sec, BYTES, READ_BYTES_SEC);
> > + SET_IOTUNE_FIELD(write_bytes_sec, BYTES, WRITE_BYTES_SEC);
> > + SET_IOTUNE_FIELD(total_iops_sec, IOPS, TOTAL_IOPS_SEC);
> > + SET_IOTUNE_FIELD(read_iops_sec, IOPS, READ_IOPS_SEC);
> > + SET_IOTUNE_FIELD(write_iops_sec, IOPS, WRITE_IOPS_SEC);
> > +
> > + SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, TOTAL_BYTES_SEC_MAX);
> > + SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, READ_BYTES_SEC_MAX);
> > + SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, WRITE_BYTES_SEC_MAX);
> > + SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, TOTAL_IOPS_SEC_MAX);
> > + SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, READ_IOPS_SEC_MAX);
> > + SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX);
> > + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC);
> > +
> > + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) {
> > + VIR_FREE(info.group_name);
> > + if (VIR_STRDUP(info.group_name, param->value.s) < 0)
> > + goto cleanup;
> > + if (virTypedParamsAddString(&eventParams,
> > + &eventNparams,
> > + &eventMaxparams,
> > + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME,
> > + param->value.s) < 0)
> > + goto cleanup;
> > + continue;
> > + }
> > +
> > + SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH,
> > + TOTAL_BYTES_SEC_MAX_LENGTH);
> > + SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH,
> > + READ_BYTES_SEC_MAX_LENGTH);
> > + SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH,
> > + WRITE_BYTES_SEC_MAX_LENGTH);
> > + SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH,
> > + TOTAL_IOPS_SEC_MAX_LENGTH);
> > + SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH,
> > + READ_IOPS_SEC_MAX_LENGTH);
> > + SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH,
> > + WRITE_IOPS_SEC_MAX_LENGTH);
> > + }
> > +#undef SET_IOTUNE_FIELD
> > +
> > + if ((info.total_bytes_sec && info.read_bytes_sec) ||
> > + (info.total_bytes_sec && info.write_bytes_sec)) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("total and read/write of bytes_sec "
> > + "cannot be set at the same time"));
> > + goto cleanup;
> > + }
> > +
> > + if ((info.total_iops_sec && info.read_iops_sec) ||
> > + (info.total_iops_sec && info.write_iops_sec)) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("total and read/write of iops_sec "
> > + "cannot be set at the same time"));
> > + goto cleanup;
> > + }
> > +
> > + if ((info.total_bytes_sec_max && info.read_bytes_sec_max) ||
> > + (info.total_bytes_sec_max && info.write_bytes_sec_max)) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("total and read/write of bytes_sec_max "
> > + "cannot be set at the same time"));
> > + goto cleanup;
> > + }
> > +
> > + if ((info.total_iops_sec_max && info.read_iops_sec_max) ||
> > + (info.total_iops_sec_max && info.write_iops_sec_max)) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("total and read/write of iops_sec_max "
> > + "cannot be set at the same time"));
> > + goto cleanup;
> > + }
> > +
> > + if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0)
> > + goto cleanup;
> > + info.group_name = NULL;
>
> You should also be checking violations against the max settings, like you
> mentioned in the commit note, some of the checks are hypervisor dependent, but
> some are purely logical, e.g. setting one of the limits higher than the
> corresponding MAX feels odd. I'm not insisting on the need to specify the limit
> along the max (that really feels like per-hypervisor detail).
>
Aah, right. I'll fix that.
Thanks!
Ilias
> > +
> > + ret = 0;
> > + cleanup:
> > + VIR_FREE(info.group_name);
> > + virDomainObjEndAPI(&vm);
> > + if (eventNparams)
> > + virTypedParamsFree(eventParams, eventNparams);
> > + return ret;
> > +}
> > +
> > +
> > static int
> > testDomainGetBlockIoTune(virDomainPtr dom,
> > const char *path,
> > @@ -8600,6 +8795,7 @@ static virHypervisorDriver testHypervisorDriver = {
> > .domainSetNumaParameters = testDomainSetNumaParameters, /* 5.6.0 */
> > .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */
> > .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */
> > + .domainSetBlockIoTune = testDomainSetBlockIoTune, /* 5.6.0 */
>
> Since you're going to re-spin, don't forget to update ^this.
>
> Erik
>
> > .domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */
> > .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */
> > .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */
> > --
> > 2.22.0
> >
> > --
> > libvir-list mailing list
> > libvir-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
