qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_re


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set
Date: Tue, 19 Sep 2017 12:43:45 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

19.09.2017 01:27, Eric Blake wrote:
On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:
Do not continue any operation if s->quit is set in parallel.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  block/nbd-client.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 280147e6a7..f80a4c5564 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -81,7 +81,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
          if (ret < 0) {
              error_report_err(local_err);
          }
-        if (ret <= 0) {
+        if (ret <= 0 || s->quit) {
              break;
          }
I'm still not convinced this helps: either nbd_receive_reply() already
returned an error, or we got a successful reply header, at which point
either the command is done (no need to abort the command early - it
succeeded) or it is a read command (and we should detect at the point
where we try to populate the qiov that we don't want to read any more).
It depends on your 3/7 patch for the fact that we even try to read the
qiov directly in this while loop rather than in the coroutine handler,
where Paolo questioned whether we need that change.

@@ -105,9 +105,8 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
              assert(qiov != NULL);
              assert(s->requests[i].request->len ==
                     iov_size(qiov->iov, qiov->niov));
-            if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
-                                      NULL) < 0)
-            {
+            ret = qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov, NULL);
+            if (ret < 0 || s->quit) {
                  s->requests[i].ret = -EIO;
                  break;
              }
The placement here looks odd. Either we should not attempt the read
because s->quit was already set (okay, your first addition makes sense
in that light), or we succeeded at the read (at which point, why do we
need to claim it failed?).

I'm trying to look forward to structured reads, where we will have to
deal with more than one server message in reply to a single client
request.  For read, we just piece together portions of the qiov until
the server has sent us all the pieces.  But for block query, I really do
think we'll want to defer to specialized handlers for doing further
reads (the amount of data to be read from the server is not known by the
client a priori, so it is a two-part read, one to get the length, and
another to read that remaining length); if we need some sort of callback
function rather than cramming all the logic here in the main read loop,

by patch 3 I do not mean in any way that I want to have all reading staff in one function, ofcourse it should be refactored for block-status addition. I just want to have all reading staff in one coroutine. Reading from ioc is sequential, so it's
native to do it sequentially in one coroutine, without switches.

then the qio_channel_readv_all for read commands would belong in the
callback, and it is more a question of the main read loop checking
whether there is an early quit condition before calling into the callback.


If I understand correctly, the discussion is:

me: if something fails - fail all other parallel operations - it's simple and clear, but we need to check s->quit after every possible yield

you: .... _not_ all other parallel operations. - may be it is better, but in this case we need carefully define, which parallel operation we fail

 on s->quit and which not and understand all effects of this division.

--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]