From: Kevin Wolf <kwolf@redhat.com> To: Zhi Yong Wu <zwu.kernel@gmail.com> Cc: stefanha@linux.vnet.ibm.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v8 1/4] block: add the block queue support Date: Tue, 18 Oct 2011 11:56:01 +0200 [thread overview] Message-ID: <4E9D4D31.1070406@redhat.com> (raw) In-Reply-To: <CAEH94LhP5Sq+Pn=shWLOR=Hx+Be8X+BFZsEb8Fd0USD1v=eeOg@mail.gmail.com> Am 18.10.2011 11:29, schrieb Zhi Yong Wu: >>>>>>> +void qemu_del_block_queue(BlockQueue *queue) >>>>>>> +{ >>>>>>> + BlockQueueAIOCB *request, *next; >>>>>>> + >>>>>>> + QTAILQ_FOREACH_SAFE(request, &queue->requests, entry, next) { >>>>>>> + QTAILQ_REMOVE(&queue->requests, request, entry); >>>>>>> + qemu_aio_release(request); >>>>>>> + } >>>>>>> + >>>>>>> + g_free(queue); >>>>>>> +} >>>>>> >>>>>> Can we be sure that no AIO requests are in flight that still use the now >>>>>> released AIOCB? >>>>> Yeah, since qemu core code is serially performed, i think that when >>>>> qemu_del_block_queue is performed, no requests are in flight. Right? >>>> >>>> Patch 2 has this code: >>>> >>>> +void bdrv_io_limits_disable(BlockDriverState *bs) >>>> +{ >>>> + bs->io_limits_enabled = false; >>>> + >>>> + if (bs->block_queue) { >>>> + qemu_block_queue_flush(bs->block_queue); >>>> + qemu_del_block_queue(bs->block_queue); >>>> + bs->block_queue = NULL; >>>> + } >>>> >>>> Does this mean that you can't disable I/O limits while the VM is running? >>> NO, you can even though VM is running. >> >> Okay, in general qemu_block_queue_flush() empties the queue so that >> there are no requests left that qemu_del_block_queue() could drop from >> the queue. So in the common case it doesn't even enter the FOREACH loop. > I think that we should adopt !QTAILQ_EMPTY(&queue->requests), not > QTAILQ_FOREACH_SAFE in qemu_del_block_queue(), > right? I think QTAILQ_FOREACH_SAFE is fine. >> >> I think problems start when requests have failed or exceeded the limit >> again, then you have requests queued even after >> qemu_block_queue_flush(). You must be aware of this, otherwise the code >> in qemu_del_block_queue() wouldn't exist. >> >> But you can't release the ACBs without having called their callback, >> otherwise the caller would still assume that its ACB pointer is valid. >> Maybe calling the callback before releasing the ACB would be enough. > Good, thanks. >>>> >>>>>>> + } >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> + queue->req_failed = true; >>>>>>> + queue->flushing = false; >>>>>>> +} >>>>>>> + >>>>>>> +bool qemu_block_queue_has_pending(BlockQueue *queue) >>>>>>> +{ >>>>>>> + return !queue->flushing && !QTAILQ_EMPTY(&queue->requests); >>>>>>> +} >>>>>> >>>>>> Why doesn't the queue have pending requests in the middle of a flush >>>>>> operation? (That is, the flush hasn't completed yet) >>>>> It is possible for the queue to have pending requests. if yes, how about? >>>> >>>> Sorry, can't parse this. >>>> >>>> I don't understand why the !queue->flushing part is correct. >> >> What about this? > When bdrv_aio_readv/writev handle one request, it will determine if > block queue is not being flushed and isn't NULL; if yes, It assume > that this request is one new request from upper layer, so it won't > determine if the I/O rate at runtime has exceeded the limits, but > immediately insert it into block queue. Hm, I think I understand what you're saying, but only after looking at patch 3. This is not really implementing a has_pending(), but has_pending_and_caller_wasnt_called_during_flush(). I think it would be better to handle the queue->flushing condition in the caller where its use is more obvious. Kevin
WARNING: multiple messages have this Message-ID (diff)
From: Kevin Wolf <kwolf@redhat.com> To: Zhi Yong Wu <zwu.kernel@gmail.com> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>, stefanha@linux.vnet.ibm.com, kvm@vger.kernel.org, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH v8 1/4] block: add the block queue support Date: Tue, 18 Oct 2011 11:56:01 +0200 [thread overview] Message-ID: <4E9D4D31.1070406@redhat.com> (raw) In-Reply-To: <CAEH94LhP5Sq+Pn=shWLOR=Hx+Be8X+BFZsEb8Fd0USD1v=eeOg@mail.gmail.com> Am 18.10.2011 11:29, schrieb Zhi Yong Wu: >>>>>>> +void qemu_del_block_queue(BlockQueue *queue) >>>>>>> +{ >>>>>>> + BlockQueueAIOCB *request, *next; >>>>>>> + >>>>>>> + QTAILQ_FOREACH_SAFE(request, &queue->requests, entry, next) { >>>>>>> + QTAILQ_REMOVE(&queue->requests, request, entry); >>>>>>> + qemu_aio_release(request); >>>>>>> + } >>>>>>> + >>>>>>> + g_free(queue); >>>>>>> +} >>>>>> >>>>>> Can we be sure that no AIO requests are in flight that still use the now >>>>>> released AIOCB? >>>>> Yeah, since qemu core code is serially performed, i think that when >>>>> qemu_del_block_queue is performed, no requests are in flight. Right? >>>> >>>> Patch 2 has this code: >>>> >>>> +void bdrv_io_limits_disable(BlockDriverState *bs) >>>> +{ >>>> + bs->io_limits_enabled = false; >>>> + >>>> + if (bs->block_queue) { >>>> + qemu_block_queue_flush(bs->block_queue); >>>> + qemu_del_block_queue(bs->block_queue); >>>> + bs->block_queue = NULL; >>>> + } >>>> >>>> Does this mean that you can't disable I/O limits while the VM is running? >>> NO, you can even though VM is running. >> >> Okay, in general qemu_block_queue_flush() empties the queue so that >> there are no requests left that qemu_del_block_queue() could drop from >> the queue. So in the common case it doesn't even enter the FOREACH loop. > I think that we should adopt !QTAILQ_EMPTY(&queue->requests), not > QTAILQ_FOREACH_SAFE in qemu_del_block_queue(), > right? I think QTAILQ_FOREACH_SAFE is fine. >> >> I think problems start when requests have failed or exceeded the limit >> again, then you have requests queued even after >> qemu_block_queue_flush(). You must be aware of this, otherwise the code >> in qemu_del_block_queue() wouldn't exist. >> >> But you can't release the ACBs without having called their callback, >> otherwise the caller would still assume that its ACB pointer is valid. >> Maybe calling the callback before releasing the ACB would be enough. > Good, thanks. >>>> >>>>>>> + } >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> + queue->req_failed = true; >>>>>>> + queue->flushing = false; >>>>>>> +} >>>>>>> + >>>>>>> +bool qemu_block_queue_has_pending(BlockQueue *queue) >>>>>>> +{ >>>>>>> + return !queue->flushing && !QTAILQ_EMPTY(&queue->requests); >>>>>>> +} >>>>>> >>>>>> Why doesn't the queue have pending requests in the middle of a flush >>>>>> operation? (That is, the flush hasn't completed yet) >>>>> It is possible for the queue to have pending requests. if yes, how about? >>>> >>>> Sorry, can't parse this. >>>> >>>> I don't understand why the !queue->flushing part is correct. >> >> What about this? > When bdrv_aio_readv/writev handle one request, it will determine if > block queue is not being flushed and isn't NULL; if yes, It assume > that this request is one new request from upper layer, so it won't > determine if the I/O rate at runtime has exceeded the limits, but > immediately insert it into block queue. Hm, I think I understand what you're saying, but only after looking at patch 3. This is not really implementing a has_pending(), but has_pending_and_caller_wasnt_called_during_flush(). I think it would be better to handle the queue->flushing condition in the caller where its use is more obvious. Kevin
next prev parent reply other threads:[~2011-10-18 9:53 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-09-08 10:11 [PATCH v8 0/4] The intro of QEMU block I/O throttling Zhi Yong Wu 2011-09-08 10:11 ` [Qemu-devel] " Zhi Yong Wu 2011-09-08 10:11 ` [PATCH v8 1/4] block: add the block queue support Zhi Yong Wu 2011-09-08 10:11 ` [Qemu-devel] " Zhi Yong Wu 2011-09-23 15:32 ` Kevin Wolf 2011-09-23 15:32 ` [Qemu-devel] " Kevin Wolf 2011-09-26 8:01 ` Zhi Yong Wu 2011-09-26 8:01 ` Zhi Yong Wu 2011-10-17 10:17 ` Kevin Wolf 2011-10-17 10:17 ` Kevin Wolf 2011-10-17 10:17 ` Paolo Bonzini 2011-10-18 7:00 ` Zhi Yong Wu 2011-10-18 7:00 ` Zhi Yong Wu 2011-10-18 8:07 ` Zhi Yong Wu 2011-10-18 8:07 ` [Qemu-devel] " Zhi Yong Wu 2011-10-18 8:36 ` Kevin Wolf 2011-10-18 8:36 ` Kevin Wolf 2011-10-18 9:29 ` Zhi Yong Wu 2011-10-18 9:29 ` Zhi Yong Wu 2011-10-18 9:56 ` Kevin Wolf [this message] 2011-10-18 9:56 ` Kevin Wolf 2011-10-18 13:29 ` Zhi Yong Wu 2011-10-18 13:29 ` Zhi Yong Wu 2011-09-08 10:11 ` [PATCH v8 2/4] block: add the command line support Zhi Yong Wu 2011-09-08 10:11 ` [Qemu-devel] " Zhi Yong Wu 2011-09-23 15:54 ` Kevin Wolf 2011-09-23 15:54 ` [Qemu-devel] " Kevin Wolf 2011-09-26 6:15 ` Zhi Yong Wu 2011-09-26 6:15 ` [Qemu-devel] " Zhi Yong Wu 2011-10-17 10:19 ` Kevin Wolf 2011-10-17 10:19 ` Kevin Wolf 2011-10-18 8:17 ` Zhi Yong Wu 2011-10-18 8:17 ` [Qemu-devel] " Zhi Yong Wu 2011-09-08 10:11 ` [PATCH v8 3/4] block: add block timer and throttling algorithm Zhi Yong Wu 2011-09-08 10:11 ` [Qemu-devel] " Zhi Yong Wu 2011-09-09 14:44 ` Marcelo Tosatti 2011-09-09 14:44 ` [Qemu-devel] " Marcelo Tosatti 2011-09-13 3:09 ` Zhi Yong Wu 2011-09-13 3:09 ` [Qemu-devel] " Zhi Yong Wu 2011-09-14 10:50 ` Marcelo Tosatti 2011-09-14 10:50 ` [Qemu-devel] " Marcelo Tosatti 2011-09-19 9:55 ` Zhi Yong Wu 2011-09-19 9:55 ` [Qemu-devel] " Zhi Yong Wu 2011-09-20 12:34 ` Marcelo Tosatti 2011-09-20 12:34 ` [Qemu-devel] " Marcelo Tosatti 2011-09-21 3:14 ` Zhi Yong Wu 2011-09-21 3:14 ` [Qemu-devel] " Zhi Yong Wu 2011-09-21 5:54 ` Zhi Yong Wu 2011-09-21 5:54 ` [Qemu-devel] " Zhi Yong Wu 2011-09-21 7:03 ` Zhi Yong Wu 2011-09-21 7:03 ` [Qemu-devel] " Zhi Yong Wu 2011-09-26 8:15 ` Zhi Yong Wu 2011-09-26 8:15 ` [Qemu-devel] " Zhi Yong Wu 2011-09-23 16:19 ` Kevin Wolf 2011-09-23 16:19 ` [Qemu-devel] " Kevin Wolf 2011-09-26 7:24 ` Zhi Yong Wu 2011-09-26 7:24 ` [Qemu-devel] " Zhi Yong Wu 2011-10-17 10:26 ` Kevin Wolf 2011-10-17 10:26 ` Kevin Wolf 2011-10-17 15:54 ` Stefan Hajnoczi 2011-10-17 15:54 ` Stefan Hajnoczi 2011-10-18 8:29 ` Zhi Yong Wu 2011-10-18 8:29 ` Zhi Yong Wu 2011-10-18 8:43 ` Zhi Yong Wu 2011-10-18 8:43 ` Zhi Yong Wu 2011-09-08 10:11 ` [PATCH v8 4/4] qmp/hmp: add block_set_io_throttle Zhi Yong Wu 2011-09-08 10:11 ` [Qemu-devel] " Zhi Yong Wu -- strict thread matches above, loose matches on Subject: below -- 2011-09-07 12:31 [PATCH v8 0/4] The intro of QEMU block I/O throttling Zhi Yong Wu 2011-09-07 12:31 ` [Qemu-devel] [PATCH v8 1/4] block: add the block queue support Zhi Yong Wu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4E9D4D31.1070406@redhat.com \ --to=kwolf@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@linux.vnet.ibm.com \ --cc=wuzhy@linux.vnet.ibm.com \ --cc=zwu.kernel@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.