Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices

[Greg Kurz]

On Tue, 20 Sep 2016 11:02:07 +0200
Pradeep Jagadeesh <address@hidden> 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