On Fri 16 Sep 2016 10:33:36 AM CEST, Pradeep Jagadeesh wrote: Hi,
first of all, sorry for the late reply! Here are my comments: > --- a/fsdev/qemu-fsdev-opts.c > +++ b/fsdev/qemu-fsdev-opts.c > @@ -37,6 +37,82 @@ static QemuOptsList qemu_fsdev_opts = { > }, { > .name = "sock_fd", > .type = QEMU_OPT_NUMBER, > + }, { > + .name = "throttling.iops-total", > + .type = QEMU_OPT_NUMBER, > + .help = "limit total I/O operations per second", /*...*/ > + },{ > + .name = "throttling.iops-size", > + .type = QEMU_OPT_NUMBER, > + .help = "when limiting by iops max size of an I/O in bytes", It would be nice if we could factor these so we don't have to have them twice in the code. I think it should be doable with qemu_opts_append(). It can be done later in a separate patch if you prefer. > +typedef struct FsThrottle { > + ThrottleState ts; > + ThrottleTimers tt; > + AioContext *aioctx; > + ThrottleConfig cfg; > + bool enabled; > + CoQueue throttled_reqs[2]; > + unsigned pending_reqs[2]; > + bool any_timer_armed[2]; > + QemuMutex lock; > +} FsThrottle; You based your implementation on block/throttle-groups.c. I think yours can be made simpler because one of the problems with mine is that it needs to support multiple parallel I/O operations on the same throttling group, and that's why the locking rules are more complex. With a single user per ThrottleConfig this is not necessary. Have you checked if you really need them? My impression is that you might be able to get rid of the 'lock', 'any_timer_armed' and 'pending_reqs' fields. Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how was the transition from single-drive throttling to group throttling, specifically the bdrv_io_limits_intercept() function. You will see that it was simpler. > @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, > V9fsFidOpenState *fs, > const struct iovec *iov, > int iovcnt, off_t offset) > { > - ssize_t ret > -; > + ssize_t ret; > + This could go to a separate patch, but I'm fine if you include it in this one. > @@ -1213,6 +1213,8 @@ static int local_parse_opts(QemuOpts *opts, struct > FsDriverEntry *fse) > const char *sec_model = qemu_opt_get(opts, "security_model"); > const char *path = qemu_opt_get(opts, "path"); > > + FsThrottle *fst = &fse->fst; > + I'd remove the empty line between the declarations of 'path' and 'fst', however... > if (!sec_model) { > error_report("Security model not specified, local fs needs security > model"); > error_printf("valid options are:" > @@ -1240,6 +1242,11 @@ static int local_parse_opts(QemuOpts *opts, struct > FsDriverEntry *fse) > error_report("fsdev: No path specified"); > return -1; > } > + > + if (fsdev_throttle_configure_iolimits(opts, fst) < 0) { > + return -1; > + } ...you don't need to declare fst here, you could also pass &fse->fst directly. Berto