| Message ID | 1459783562-28859-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/04/2016 09:26 AM, Kevin Wolf wrote: > As the patches to move I/O throttling to BlockBackend didn't make it in > time for the 2.6 release, but the release adds new ways of configuring > VMs whose behaviour would change once the move is done, we need to > outlaw such configurations temporarily. > > The problem exists whenever a BDS has more users than just its BB, for > example it is used as a backing file for another node. (This wasn't > possible in 2.5 yet as we introduced node references to specify a > backing file only recently.) In these cases, the throttling would > apply to these other users now, but after moving throttling to the > BlockBackend the other users wouldn't be throttled any more. > > This patch both prevents making new references to a throttled node as > well as using monitor commands to throttle a node with multiple parents. > > Compared to 2.5 this changes behaviour in some corner cases where > references were allowed before, like bs->file or Quorum children. It > seems reasonable to assume that users didn't use I/O throttling on such > low level nodes. With the upcoming move of throttling into BlockBackend, > such configurations won't be possible anyway. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 7 +++++++ > blockdev.c | 7 +++++++ > 2 files changed, 14 insertions(+) Reviewed-by: Eric Blake <eblake@redhat.com>
On Mon 04 Apr 2016 05:26:02 PM CEST, Kevin Wolf wrote: > The problem exists whenever a BDS has more users than just its BB, for > example it is used as a backing file for another node. The code seems correct, but I don't understand this use case. Berto
Am 06.04.2016 um 11:45 hat Alberto Garcia geschrieben: > On Mon 04 Apr 2016 05:26:02 PM CEST, Kevin Wolf wrote: > > The problem exists whenever a BDS has more users than just its BB, for > > example it is used as a backing file for another node. > > The code seems correct, but I don't understand this use case. Let me draw some ASCII art. I'm drawing I/O throttling like a filter to make the difference clear; in reality it's part of the BDS now or of the BB in 2.7, so don't let this confuse you. Let's assume these command line options: -drive file=backimg.img,if=none,id=backing,iops=1024 -drive file=overlay.qcow2,id=overlay,backing=backing If we didn't catch this configuration and error out, this is what we would end up with in 2.6: [BB] overlay | | [BDS] qcow2 [BB] backing | | +--------------------+ . | . I/O throttling . | [BDS] raw . . . After moving I/O throttling to the BDS level in 2.7, the resulting configuration would look different: [BB] overlay | | [BDS] qcow2 [BB] backing | | | | | I/O throttling | | +--------------------+ . [BDS] raw . . . . . As you can see, requests coming from the overlay image wouldn't be throttled any more. If we allowed this set of options now, we would have an API breakage in 2.7, which we don't want. That's why I want to disable it for the 2.6 release. Kevin
On Wed 06 Apr 2016 11:59:47 AM CEST, Kevin Wolf wrote: > Let me draw some ASCII art. I'm drawing I/O throttling like a filter > to make the difference clear; in reality it's part of the BDS now or > of the BB in 2.7, so don't let this confuse you. > > Let's assume these command line options: > > -drive file=backimg.img,if=none,id=backing,iops=1024 > -drive file=overlay.qcow2,id=overlay,backing=backing > > If we didn't catch this configuration and error out, this is what we > would end up with in 2.6: > > [BB] overlay > | > | > [BDS] qcow2 [BB] backing > | | > +--------------------+ > . | > . I/O throttling > . | > [BDS] raw > . > . > . I see, thanks for the explanation! But what's the use of having a BlockBackend on a node that is used as a backing image? Is it because "we allow this so let's prevent the user from doing weird things", or is there anything useful we can do with it? Berto
On 06/04/2016 17:55, Alberto Garcia wrote: >> > [BB] overlay >> > | >> > | >> > [BDS] qcow2 [BB] backing >> > | | >> > +--------------------+ >> > . | >> > . I/O throttling >> > . | >> > [BDS] raw >> > . >> > . >> > . > I see, thanks for the explanation! > > But what's the use of having a BlockBackend on a node that is used as a > backing image? Is it because "we allow this so let's prevent the user > from doing weird things", or is there anything useful we can do with it? The backing image can be the one that is visible to the guest, while the overlay can be a block-backup target (with 'sync':'none) whose BlockBackend is created by the NBD server. Paolo
On Mon 04 Apr 2016 05:26:02 PM CEST, Kevin Wolf wrote: > As the patches to move I/O throttling to BlockBackend didn't make it in > time for the 2.6 release, but the release adds new ways of configuring > VMs whose behaviour would change once the move is done, we need to > outlaw such configurations temporarily. > > The problem exists whenever a BDS has more users than just its BB, for > example it is used as a backing file for another node. (This wasn't > possible in 2.5 yet as we introduced node references to specify a > backing file only recently.) In these cases, the throttling would > apply to these other users now, but after moving throttling to the > BlockBackend the other users wouldn't be throttled any more. > > This patch both prevents making new references to a throttled node as > well as using monitor commands to throttle a node with multiple parents. > > Compared to 2.5 this changes behaviour in some corner cases where > references were allowed before, like bs->file or Quorum children. It > seems reasonable to assume that users didn't use I/O throttling on such > low level nodes. With the upcoming move of throttling into BlockBackend, > such configurations won't be possible anyway. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
diff --git a/block.c b/block.c index d36eb75..d4939b4 100644 --- a/block.c +++ b/block.c @@ -1526,6 +1526,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, if (!bs) { return -ENODEV; } + + if (bs->throttle_state) { + error_setg(errp, "Cannot reference an existing block device for " + "which I/O throttling is enabled"); + return -EINVAL; + } + bdrv_ref(bs); *pbs = bs; return 0; diff --git a/blockdev.c b/blockdev.c index 332068a..f1f520a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2661,6 +2661,13 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, goto out; } + /* The BlockBackend must be the only parent */ + assert(QLIST_FIRST(&bs->parents)); + if (QLIST_NEXT(QLIST_FIRST(&bs->parents), next_parent)) { + error_setg(errp, "Cannot throttle device with multiple parents"); + goto out; + } + throttle_config_init(&cfg); cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps; cfg.buckets[THROTTLE_BPS_READ].avg = bps_rd;
As the patches to move I/O throttling to BlockBackend didn't make it in time for the 2.6 release, but the release adds new ways of configuring VMs whose behaviour would change once the move is done, we need to outlaw such configurations temporarily. The problem exists whenever a BDS has more users than just its BB, for example it is used as a backing file for another node. (This wasn't possible in 2.5 yet as we introduced node references to specify a backing file only recently.) In these cases, the throttling would apply to these other users now, but after moving throttling to the BlockBackend the other users wouldn't be throttled any more. This patch both prevents making new references to a throttled node as well as using monitor commands to throttle a node with multiple parents. Compared to 2.5 this changes behaviour in some corner cases where references were allowed before, like bs->file or Quorum children. It seems reasonable to assume that users didn't use I/O throttling on such low level nodes. With the upcoming move of throttling into BlockBackend, such configurations won't be possible anyway. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 7 +++++++ blockdev.c | 7 +++++++ 2 files changed, 14 insertions(+)