[Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Vladimir Sementsov-Ogievskiy]

Do not continue any operation if s->quit is set in parallel.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 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;
 }
 
@@ -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;
 }
-- 
2.11.1

Re: [Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Vladimir Sementsov-Ogievskiy]

19.09.2017 13:03, Paolo Bonzini wrote:

On 19/09/2017 11:43, Vladimir Sementsov-Ogievskiy wrote:

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.

But why is that a goal?  See it as a state machine that goes between the
"waiting for header" and "waiting for payload" states.  Reading header
corresponds to read_reply_co waiting on the socket (and reading when
it's ready); reading payload corresponds to the request coroutine
waiting on the socket and reading when it's ready.

Paolo



I just dislike public access to ioc for commands and extra coroutine 
switching and synchronization.




--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Paolo Bonzini]

On 19/09/2017 11:43, Vladimir Sementsov-Ogievskiy wrote:
>>
>> 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.

But why is that a goal?  See it as a state machine that goes between the
"waiting for header" and "waiting for payload" states.  Reading header
corresponds to read_reply_co waiting on the socket (and reading when
it's ready); reading payload corresponds to the request coroutine
waiting on the socket and reading when it's ready.

Paolo

Re: [Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Vladimir Sementsov-Ogievskiy]

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 
---
  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

Re: [Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Eric Blake]

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 
> ---
>  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,
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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 6/7] block/nbd-client: early fail nbd_read_reply_entry if s->quit is set

[Vladimir Sementsov-Ogievskiy]

Do not continue any operation if s->quit is set in parallel.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 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;
 }
 
@@ -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;
 }
-- 
2.11.1