[Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Alberto Garcia]

Disabling I/O limits from a BDS also drains all pending throttled
requests, so it should be done at the beginning of bdrv_close() with
the rest of the bdrv_drain() calls before the BlockDriver is closed.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 6268e37..1f90b47 100644
--- a/block.c
+++ b/block.c
@@ -1907,6 +1907,12 @@ void bdrv_close(BlockDriverState *bs)
     if (bs->job) {
         block_job_cancel_sync(bs->job);
     }
+
+    /* Disable I/O limits and drain all pending throttled requests */
+    if (bs->io_limits_enabled) {
+        bdrv_io_limits_disable(bs);
+    }
+
     bdrv_drain(bs); /* complete I/O */
     bdrv_flush(bs);
     bdrv_drain(bs); /* in case flush left pending I/O */
@@ -1958,11 +1964,6 @@ void bdrv_close(BlockDriverState *bs)
         blk_dev_change_media_cb(bs->blk, false);
     }
 
-    /*throttling disk I/O limits*/
-    if (bs->io_limits_enabled) {
-        bdrv_io_limits_disable(bs);
-    }
-
     QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_next) {
         g_free(ban);
     }
-- 
2.5.3

Re: [Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On 09/25/2015 07:41 AM, Alberto Garcia wrote:
> Disabling I/O limits from a BDS also drains all pending throttled
> requests, so it should be done at the beginning of bdrv_close() with
> the rest of the bdrv_drain() calls before the BlockDriver is closed.

Can this be abused? If I have a guest running in a cloud where the cloud
provider has put severe throttling limits on me, but lets me hotplug to
my heart's content, couldn't I just repeatedly plug/unplug the disk to
get around the throttling (every time I unplug, all writes flush at full
speed, then I immediately replug to start batching up a new set of
writes).  In other words, shouldn't the draining still be throttled, to
prevent my abuse?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Alberto Garcia]

On Fri 25 Sep 2015 04:22:26 PM CEST, Eric Blake wrote:

>> Disabling I/O limits from a BDS also drains all pending throttled
>> requests, so it should be done at the beginning of bdrv_close() with
>> the rest of the bdrv_drain() calls before the BlockDriver is closed.
>
> Can this be abused? If I have a guest running in a cloud where the
> cloud provider has put severe throttling limits on me, but lets me
> hotplug to my heart's content, couldn't I just repeatedly plug/unplug
> the disk to get around the throttling (every time I unplug, all writes
> flush at full speed, then I immediately replug to start batching up a
> new set of writes).  In other words, shouldn't the draining still be
> throttled, to prevent my abuse?

I didn't think about this case, and I don't know how practical this is,
but note that bdrv_drain() (which is already at the beginning of
bdrv_close()) flushes the I/O queue explicitly bypassing the limits, so
other cases where a user can trigger a bdrv_drain() would also be
vulnerable to this.

Berto

Re: [Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Fam Zheng]

On Fri, 09/25 16:31, Alberto Garcia wrote:
> On Fri 25 Sep 2015 04:22:26 PM CEST, Eric Blake wrote:
> 
> >> Disabling I/O limits from a BDS also drains all pending throttled
> >> requests, so it should be done at the beginning of bdrv_close() with
> >> the rest of the bdrv_drain() calls before the BlockDriver is closed.
> >
> > Can this be abused? If I have a guest running in a cloud where the
> > cloud provider has put severe throttling limits on me, but lets me
> > hotplug to my heart's content, couldn't I just repeatedly plug/unplug
> > the disk to get around the throttling (every time I unplug, all writes
> > flush at full speed, then I immediately replug to start batching up a
> > new set of writes).  In other words, shouldn't the draining still be
> > throttled, to prevent my abuse?
> 
> I didn't think about this case, and I don't know how practical this is,
> but note that bdrv_drain() (which is already at the beginning of
> bdrv_close()) flushes the I/O queue explicitly bypassing the limits, so
> other cases where a user can trigger a bdrv_drain() would also be
> vulnerable to this.
> 

Yes, the issue is pre-existing. This patch only reordered things inside
bdrv_close() so it's no worse.

But indeed there is this vulnerability, maybe we should throttle the queue in
all cases?

Fam

Re: [Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Alberto Garcia]

On Mon 28 Sep 2015 02:18:33 AM CEST, Fam Zheng <famz@redhat.com> wrote:

>> > Can this be abused? If I have a guest running in a cloud where the
>> > cloud provider has put severe throttling limits on me, but lets me
>> > hotplug to my heart's content, couldn't I just repeatedly
>> > plug/unplug the disk to get around the throttling (every time I
>> > unplug, all writes flush at full speed, then I immediately replug
>> > to start batching up a new set of writes).  In other words,
>> > shouldn't the draining still be throttled, to prevent my abuse?
>> 
>> I didn't think about this case, and I don't know how practical this
>> is, but note that bdrv_drain() (which is already at the beginning of
>> bdrv_close()) flushes the I/O queue explicitly bypassing the limits,
>> so other cases where a user can trigger a bdrv_drain() would also be
>> vulnerable to this.
>
> Yes, the issue is pre-existing. This patch only reordered things
> inside bdrv_close() so it's no worse.
>
> But indeed there is this vulnerability, maybe we should throttle the
> queue in all cases?

I would like to see a test case with numbers that show how much you can
actually bypass the I/O limits.

Berto

Re: [Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Andrey Korolyov]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

On Mon, Sep 28, 2015 at 12:04 PM, Alberto Garcia <berto@igalia.com> wrote:
> On Mon 28 Sep 2015 02:18:33 AM CEST, Fam Zheng <famz@redhat.com> wrote:
>
>>> > Can this be abused? If I have a guest running in a cloud where the
>>> > cloud provider has put severe throttling limits on me, but lets me
>>> > hotplug to my heart's content, couldn't I just repeatedly
>>> > plug/unplug the disk to get around the throttling (every time I
>>> > unplug, all writes flush at full speed, then I immediately replug
>>> > to start batching up a new set of writes).  In other words,
>>> > shouldn't the draining still be throttled, to prevent my abuse?
>>>
>>> I didn't think about this case, and I don't know how practical this
>>> is, but note that bdrv_drain() (which is already at the beginning of
>>> bdrv_close()) flushes the I/O queue explicitly bypassing the limits,
>>> so other cases where a user can trigger a bdrv_drain() would also be
>>> vulnerable to this.
>>
>> Yes, the issue is pre-existing. This patch only reordered things
>> inside bdrv_close() so it's no worse.
>>
>> But indeed there is this vulnerability, maybe we should throttle the
>> queue in all cases?
>
> I would like to see a test case with numbers that show how much you can
> actually bypass the I/O limits.
>
> Berto
>

For a wild real-world case, consider a written log/db xlog. As an
example, attached picture shows an actual IOPS measurement for the
test sample which has been automatically throttled to 70 wIOPS. The
application behind is an exim4 sending messages at a rate about 20/s.
Databases also could break the QEMU IOPS write limits but on more
specific conditions and I think it could be problematic to reproduce.
Breaking through limit could be possible on an advertised/set qd > 1.

[-- Attachment #2: exim-iops-break.png --]
[-- Type: image/png, Size: 107211 bytes --]

Re: [Qemu-devel] [PATCH] block: disable I/O limits at the beginning of bdrv_close()

[Kevin Wolf]

Am 25.09.2015 um 15:41 hat Alberto Garcia geschrieben:
> Disabling I/O limits from a BDS also drains all pending throttled
> requests, so it should be done at the beginning of bdrv_close() with
> the rest of the bdrv_drain() calls before the BlockDriver is closed.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Thanks, applied to the block branch.

Kevin