These patches provide the qmp interface, to query the io throttle 
status of the all fsdev devices that are present in a vm.
also, it provides an interface to set the io throttle parameters of a
fsdev to a required value. some of the patches also remove the duplicate
code that was present in block and fsdev files. 

Pradeep Jagadeesh (6):
  throttle: factor out duplicate code
  qmp: Create IOThrottle structure
  throttle: move out function to reuse the code
  hmp: create a throttle initialization function for code reusability
  fsdev: hmp interface for throttling
  fsdev: QMP interface for throttling

 Makefile                        |   4 ++
 blockdev.c                      |  97 ++------------------------------
 fsdev/qemu-fsdev-dummy.c        |  11 ++++
 fsdev/qemu-fsdev-throttle.c     | 120 ++++++++++++++++++++++++++--------------
 fsdev/qemu-fsdev-throttle.h     |   8 ++-
 fsdev/qemu-fsdev.c              |  38 +++++++++++++
 hmp-commands-info.hx            |  18 ++++++
 hmp-commands.hx                 |  19 +++++++
 hmp.c                           |  81 +++++++++++++++++++++++++--
 hmp.h                           |   4 ++
 include/qemu/throttle-options.h |   7 +++
 include/qemu/throttle.h         |   4 +-
 include/qemu/typedefs.h         |   1 +
 monitor.c                       |   5 ++
 qapi-schema.json                |   3 +
 qapi/block-core.json            |  34 +++++++++---
 qapi/fsdev.json                 |  81 +++++++++++++++++++++++++++
 qmp.c                           |  14 +++++
 util/throttle.c                 | 110 ++++++++++++++++++++++++++++++++++++
 19 files changed, 505 insertions(+), 154 deletions(-)
 create mode 100644 qapi/fsdev.json

v0 -> v1:
 Addressed comments from Eric Blake, Greg Kurz and Daniel P.Berrange
 Mainly renaming the functions and removing the redundant code.

v1 -> v2:
 Addressed comments from Eric Blake and Greg Kurz.
 As per the suggestion I split the patches into smaller patches.
 Removed some more duplicate code.

v2 -> v3:
 Addresssed comments from Alberto Garcia.
 Changed the comment from block to iothrottle in the iothrottle.json 
 Added the dummy functions in qemu-fsdev-dummy.c to address the compilation
 issues that were observed.

v3 -> v4:
 Addressed comments from Eric Blake and Greg Kurz
 Re-ordered the patches
 Added the dummy functions in qmp.c to address the cross compilation issues

v4 -> v5:
  Addressed comments from Eric Blake and Greg Kurz
  Split the fsdev qmp patch into hmp and qmp related patches
  Moved the common functionalities to throttle.c instead of creating
  a new file

v5 -> v6:
  Addressed comments from Greg Kurz and Markus Armbruster
  Split the commits to specific to hmp and throttle as suggested by Greg
  Moved ThrottleConfig typedef to qemu/typedefs.h
  Addressed compilation issue on FreeBSD by adding flags in qmp.c

v6 -> v7:
  Addressed comments from Albert Garcia and Dr. David Alan Gilbert
  Fixed the hmp-commands-info.hx and hmp-commands.hx as per Dr. David's
  comments.
  Fixed the bug with the hmp fsdev_set_io_throttle and info fsdev_iothrottle  

v7 -> v8:
  Addressed comments from Markus Armbruster and Eric Blake
  Removed the iothrottle.json and pushed the iothrottle struct to
  block-core.json
-- 
1.8.3.1

On Tue 29 Aug 2017 04:23:01 PM CEST, Pradeep Jagadeesh wrote:
> These patches provide the qmp interface, to query the io throttle 
> status of the all fsdev devices that are present in a vm.
> also, it provides an interface to set the io throttle parameters of a
> fsdev to a required value. some of the patches also remove the duplicate
> code that was present in block and fsdev files. 

Oops, I was just reviewing the previous version of this series, but it
looks like you just moved code around, so my comments still apply.

Anyway, this should be v9, shouldn't it? v8 was published weeks ago:

   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01043.html

Berto

On 8/29/2017 4:36 PM, Alberto Garcia wrote:
> On Tue 29 Aug 2017 04:23:01 PM CEST, Pradeep Jagadeesh wrote:
>> These patches provide the qmp interface, to query the io throttle
>> status of the all fsdev devices that are present in a vm.
>> also, it provides an interface to set the io throttle parameters of a
>> fsdev to a required value. some of the patches also remove the duplicate
>> code that was present in block and fsdev files.
>
> Oops, I was just reviewing the previous version of this series, but it
> looks like you just moved code around, so my comments still apply.
>
> Anyway, this should be v9, shouldn't it? v8 was published weeks ago:
>
>    https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01043.html

Hmm, sorry it should be v9, I missed. I will resend the patches with 
newer version.

Regards,
Pradeep
>
> Berto
>

On Tue 29 Aug 2017 04:23:01 PM CEST, Pradeep Jagadeesh wrote:
> These patches provide the qmp interface, to query the io throttle 
> status of the all fsdev devices that are present in a vm.

I'm trying to read from an 9p share that has limits set with hmp
fsdev_set_io_throttle and I'm having some problems.

For example if I'm reading a large file and I change the I/O limits
while the file is still being read then the guest process freezes.

Can you try to reproduce that scenario?

Berto

On 8/30/2017 2:07 PM, Alberto Garcia wrote:
> On Tue 29 Aug 2017 04:23:01 PM CEST, Pradeep Jagadeesh wrote:
>> These patches provide the qmp interface, to query the io throttle
>> status of the all fsdev devices that are present in a vm.
>
> I'm trying to read from an 9p share that has limits set with hmp
> fsdev_set_io_throttle and I'm having some problems.
>
> For example if I'm reading a large file and I change the I/O limits
> while the file is still being read then the guest process freezes.
>
> Can you try to reproduce that scenario?
Thanks for letting me know about the issue.
OK, I will try to reproduce.have a look.

-Pradeep

>
> Berto
>

On Wed 30 Aug 2017 02:10:53 PM CEST, Pradeep Jagadeesh wrote:

>> I'm trying to read from an 9p share that has limits set with hmp
>> fsdev_set_io_throttle and I'm having some problems.

Here's one simple way to reproduce it:

1) Launch qemu with

   -fsdev local,security_model=none,id=fs0,path=/some/files
   -device virtio-9p-pci,fsdev=fs0,mount_tag=fs0

2) In the guest, mount the fs0 share in /mnt

3) Run this hmp command

   fsdev_set_io_throttle fs0 0 4096 0 0 0 0

4) In the guest, start reading some large file from the 9p share:

   dd if=/mnt/large_file of=/dev/null bs=4k iflag=direct status=progress

5) Check the progress, reading speed should be around 4k per second.

6) While dd is still running, change the I/O limits:

   fsdev_set_io_throttle fs0 0 8192 0 0 0 0

7) If you check the status of the dd command, reading should be faster
   now. Instead, it is stalled.

Berto

On 8/30/2017 4:54 PM, Alberto Garcia wrote:
> On Wed 30 Aug 2017 02:10:53 PM CEST, Pradeep Jagadeesh wrote:
>
>>> I'm trying to read from an 9p share that has limits set with hmp
>>> fsdev_set_io_throttle and I'm having some problems.
>
> Here's one simple way to reproduce it:
>
> 1) Launch qemu with
>
>    -fsdev local,security_model=none,id=fs0,path=/some/files
>    -device virtio-9p-pci,fsdev=fs0,mount_tag=fs0
>
> 2) In the guest, mount the fs0 share in /mnt
>
> 3) Run this hmp command
>
>    fsdev_set_io_throttle fs0 0 4096 0 0 0 0
>
> 4) In the guest, start reading some large file from the 9p share:
>
>    dd if=/mnt/large_file of=/dev/null bs=4k iflag=direct status=progress
>
> 5) Check the progress, reading speed should be around 4k per second.
>
> 6) While dd is still running, change the I/O limits:
>
>    fsdev_set_io_throttle fs0 0 8192 0 0 0 0
>
> 7) If you check the status of the dd command, reading should be faster
>    now. Instead, it is stalled.

Thanks for the steps, I did reproduce the issue easily.
Looking into the code, may be we also need to try the same with the 
block devices.

-Pradeep
>
> Berto
>

On Wed 30 Aug 2017 05:07:29 PM CEST, Pradeep Jagadeesh wrote:

> Thanks for the steps, I did reproduce the issue easily. Looking into
> the code, may be we also need to try the same with the block devices.

I did some tests and it was working fine, so I'd suspect of the fsdev
code first.

Berto

On 8/30/2017 5:10 PM, Alberto Garcia wrote:
> On Wed 30 Aug 2017 05:07:29 PM CEST, Pradeep Jagadeesh wrote:
>
>> Thanks for the steps, I did reproduce the issue easily. Looking into
>> the code, may be we also need to try the same with the block devices.
>
> I did some tests and it was working fine, so I'd suspect of the fsdev
> code first.
>
OK, thanks for the clarification. I will look into fsdev code.

-Pradeep
> Berto
>

On Wed 30 Aug 2017 05:12:22 PM CEST, Pradeep Jagadeesh wrote:
> On 8/30/2017 5:10 PM, Alberto Garcia wrote:
>> On Wed 30 Aug 2017 05:07:29 PM CEST, Pradeep Jagadeesh wrote:
>>
>>> Thanks for the steps, I did reproduce the issue easily. Looking into
>>> the code, may be we also need to try the same with the block devices.
>>
>> I did some tests and it was working fine, so I'd suspect of the fsdev
>> code first.
>>
> OK, thanks for the clarification. I will look into fsdev code.

I just took a quick look at the code, the problem is almost certainly in
fsdev_set_io_throttle(): that doesn't simply update the config, it also
reinitializes the FsThrottle structure completely, creates new timers
and new throttled_reqs queues. If there were pending requests there
they're probably lost forever.

Take a look at blk_set_io_limits() and see how it is done for block
devices.

Berto

On 8/31/2017 3:34 PM, Alberto Garcia wrote:
> On Wed 30 Aug 2017 05:12:22 PM CEST, Pradeep Jagadeesh wrote:
>> On 8/30/2017 5:10 PM, Alberto Garcia wrote:
>>> On Wed 30 Aug 2017 05:07:29 PM CEST, Pradeep Jagadeesh wrote:
>>>
>>>> Thanks for the steps, I did reproduce the issue easily. Looking into
>>>> the code, may be we also need to try the same with the block devices.
>>>
>>> I did some tests and it was working fine, so I'd suspect of the fsdev
>>> code first.
>>>
>> OK, thanks for the clarification. I will look into fsdev code.
>
> I just took a quick look at the code, the problem is almost certainly in
> fsdev_set_io_throttle(): that doesn't simply update the config, it also
> reinitializes the FsThrottle structure completely, creates new timers
> and new throttled_reqs queues. If there were pending requests there
> they're probably lost forever.
>
> Take a look at blk_set_io_limits() and see how it is done for block
> devices.
Yes, that is right. I had a look. Now I am figuring out how to 
initialize the timers without loosing the pending requests.
If I update the config when there is no IO going, it works fine.
When IO is going and try to update it hangs.

-Pradeep
>
> Berto
>

On 8/31/2017 3:34 PM, Alberto Garcia wrote:
> On Wed 30 Aug 2017 05:12:22 PM CEST, Pradeep Jagadeesh wrote:
>> On 8/30/2017 5:10 PM, Alberto Garcia wrote:
>>> On Wed 30 Aug 2017 05:07:29 PM CEST, Pradeep Jagadeesh wrote:
>>>
>>>> Thanks for the steps, I did reproduce the issue easily. Looking into
>>>> the code, may be we also need to try the same with the block devices.
>>>
>>> I did some tests and it was working fine, so I'd suspect of the fsdev
>>> code first.
>>>
>> OK, thanks for the clarification. I will look into fsdev code.
>
> I just took a quick look at the code, the problem is almost certainly in
> fsdev_set_io_throttle(): that doesn't simply update the config, it also
> reinitializes the FsThrottle structure completely, creates new timers
> and new throttled_reqs queues. If there were pending requests there
> they're probably lost forever.
>
> Take a look at blk_set_io_limits() and see how it is done for block
> devices.
I fixed it. I am testing it. I was initializing the queues again.
But it just needs updation of throttle configuration.

-Pradeep
>
> Berto
>

On Wed, 30 Aug 2017 14:07:59 +0200
Alberto Garcia <berto@igalia.com> wrote:

> On Tue 29 Aug 2017 04:23:01 PM CEST, Pradeep Jagadeesh wrote:
> > These patches provide the qmp interface, to query the io throttle 
> > status of the all fsdev devices that are present in a vm.  
> 
> I'm trying to read from an 9p share that has limits set with hmp
> fsdev_set_io_throttle and I'm having some problems.
> 
> For example if I'm reading a large file and I change the I/O limits
> while the file is still being read then the guest process freezes.
> 
> Can you try to reproduce that scenario?
> 
> Berto

Thanks Berto for the review and the testing ! I'm on vacation but I'll
jump in next week when I'm back and Pradeep has posted the next version.

Cheers,

--
Greg