On Tue, 20 Sep 2016 11:02:07 +0200 Pradeep Jagadeesh <pradeep.jagade...@huawei.com> wrote:
> Hi Alberto, > Hi, > Thanks for having look at the patch. > My replies are inline. > > 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: > No problem! > > > >> --- 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(). > > Or maybe just push the list to a THROTTLE_COMMON_OPTS macro in a include/qemu/throttle-options.h header file ? > > It can be done later in a separate patch if you prefer. > OK,as of now I have just copied. We can rework and remove the redundant > code later. > > > >> +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. > OK,I am not sure,I will have to try. I will look into this and let you know. > > > >> @@ -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. > I have already included as Greg suggested. > > Isn't Berto referring to the semicolon change ? That's fine for me anyway :) > >> @@ -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... > OK > > > >> 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. > OK > > -Pradeep > > > > > Berto > > > Cheers. -- Greg