[Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing

Vladimir Sementsov-Ogievskiy posted 17 patches 6 years, 7 months ago
Failed in applying to current master (apply log)
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
block/nbd-client.h         |   9 ++-
include/block/nbd.h        |   4 +-
nbd/nbd-internal.h         |  34 ++++++---
block/nbd-client.c         | 173 ++++++++++++++++++---------------------------
nbd/client.c               |  21 +++---
tests/qemu-iotests/083.out |   4 +-
6 files changed, 115 insertions(+), 130 deletions(-)
[Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing
Posted by Vladimir Sementsov-Ogievskiy 6 years, 7 months ago
A bit more refactoring and fixing before BLOCK_STATUS series.
I've tried to make individual patches simple enough, so there are
a lot of them.

Vladimir Sementsov-Ogievskiy (17):
  nbd/client: fix nbd_opt_go
  nbd/client: refactor nbd_read_eof
  nbd/client: refactor nbd_receive_reply
  nbd/client: fix nbd_send_request to return int
  block/nbd-client: get rid of ssize_t
  block/nbd-client: fix nbd_read_reply_entry
  block/nbd-client: refactor request send/receive
  block/nbd-client: rename nbd_recv_coroutines_enter_all
  block/nbd-client: move nbd_co_receive_reply content into
    nbd_co_request
  block/nbd-client: move nbd_coroutine_end content into nbd_co_request
  block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
    error
  block/nbd-client: refactor nbd_co_request
  block/nbd-client: refactor NBDClientSession.recv_coroutine
  block/nbd-client: exit reply-reading coroutine on incorrect handle
  block/nbd-client: refactor reading reply
  block/nbd-client: drop reply field from NBDClientSession
  block/nbd-client: always return EIO on and after the first io channel
    error

 block/nbd-client.h         |   9 ++-
 include/block/nbd.h        |   4 +-
 nbd/nbd-internal.h         |  34 ++++++---
 block/nbd-client.c         | 173 ++++++++++++++++++---------------------------
 nbd/client.c               |  21 +++---
 tests/qemu-iotests/083.out |   4 +-
 6 files changed, 115 insertions(+), 130 deletions(-)

-- 
2.11.1


Re: [Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing
Posted by Eric Blake 6 years, 7 months ago
On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> A bit more refactoring and fixing before BLOCK_STATUS series.
> I've tried to make individual patches simple enough, so there are
> a lot of them.

Is your BLOCK_STATUS series something that is in good enough shape to
post a preliminary version of it (the version you posted back in
February is now horribly out-of-date, with all the good cleanups you
have been doing in the meantime).  I want to get a running start at
reviewing what I can to make sure we get improved NBD functionality into
2.11.

Also, please feel free to offer your Reviewed-by on other patches
(whether NBD-related or not).  Speaking as the NBD maintainer, I welcome
any help I can get.  And from personal experience, reviews tend to be
one of the largest bottlenecks in open source software - if you are
writing patches but not offering reviews, then you are adding to the
bottleneck so reviewers tend to set your patches aside for when they
have more time; while if you are actively offering reviews, then it is
obvious that you care about the project and your patch contributions
tend to have an easier time getting in.  My personal rule of thumb is to
try and review at least 2 other patches for every one that I send,
although that is a rather ambitious goal and there's nothing wrong if
you can't commit to theh same level of effort.

> 
> Vladimir Sementsov-Ogievskiy (17):
>   nbd/client: fix nbd_opt_go
>   nbd/client: refactor nbd_read_eof
>   nbd/client: refactor nbd_receive_reply
>   nbd/client: fix nbd_send_request to return int
>   block/nbd-client: get rid of ssize_t
>   block/nbd-client: fix nbd_read_reply_entry
>   block/nbd-client: refactor request send/receive
>   block/nbd-client: rename nbd_recv_coroutines_enter_all
>   block/nbd-client: move nbd_co_receive_reply content into
>     nbd_co_request
>   block/nbd-client: move nbd_coroutine_end content into nbd_co_request
>   block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
>     error
>   block/nbd-client: refactor nbd_co_request
>   block/nbd-client: refactor NBDClientSession.recv_coroutine
>   block/nbd-client: exit reply-reading coroutine on incorrect handle
>   block/nbd-client: refactor reading reply
>   block/nbd-client: drop reply field from NBDClientSession
>   block/nbd-client: always return EIO on and after the first io channel
>     error

Of course, parts of this will need rebasing based on what finally landed
in 2.10, but I can start reviewing what I can for this round.

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

Re: [Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing
Posted by Vladimir Sementsov-Ogievskiy 6 years, 7 months ago
17.08.2017 00:21, Eric Blake wrote:
> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> A bit more refactoring and fixing before BLOCK_STATUS series.
>> I've tried to make individual patches simple enough, so there are
>> a lot of them.
> Is your BLOCK_STATUS series something that is in good enough shape to
> post a preliminary version of it (the version you posted back in
> February is now horribly out-of-date, with all the good cleanups you
> have been doing in the meantime).  I want to get a running start at
> reviewing what I can to make sure we get improved NBD functionality into
> 2.11.

Every time I want to produce a new version of BLOCK_STATUS, I stumble on
something and new 10-20 patches refactoring series appears)

>
> Also, please feel free to offer your Reviewed-by on other patches
> (whether NBD-related or not).  Speaking as the NBD maintainer, I welcome
> any help I can get.  And from personal experience, reviews tend to be
> one of the largest bottlenecks in open source software - if you are
> writing patches but not offering reviews, then you are adding to the
> bottleneck so reviewers tend to set your patches aside for when they
> have more time; while if you are actively offering reviews, then it is
> obvious that you care about the project and your patch contributions
> tend to have an easier time getting in.  My personal rule of thumb is to
> try and review at least 2 other patches for every one that I send,
> although that is a rather ambitious goal and there's nothing wrong if
> you can't commit to theh same level of effort.

Thanks, I get the point, I'll try to do better.


>
>> Vladimir Sementsov-Ogievskiy (17):
>>    nbd/client: fix nbd_opt_go
>>    nbd/client: refactor nbd_read_eof
>>    nbd/client: refactor nbd_receive_reply
>>    nbd/client: fix nbd_send_request to return int
>>    block/nbd-client: get rid of ssize_t
>>    block/nbd-client: fix nbd_read_reply_entry
>>    block/nbd-client: refactor request send/receive
>>    block/nbd-client: rename nbd_recv_coroutines_enter_all
>>    block/nbd-client: move nbd_co_receive_reply content into
>>      nbd_co_request
>>    block/nbd-client: move nbd_coroutine_end content into nbd_co_request
>>    block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
>>      error
>>    block/nbd-client: refactor nbd_co_request
>>    block/nbd-client: refactor NBDClientSession.recv_coroutine
>>    block/nbd-client: exit reply-reading coroutine on incorrect handle
>>    block/nbd-client: refactor reading reply
>>    block/nbd-client: drop reply field from NBDClientSession
>>    block/nbd-client: always return EIO on and after the first io channel
>>      error
> Of course, parts of this will need rebasing based on what finally landed
> in 2.10, but I can start reviewing what I can for this round.
>

-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing
Posted by Eric Blake 6 years, 7 months ago
On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> A bit more refactoring and fixing before BLOCK_STATUS series.
> I've tried to make individual patches simple enough, so there are
> a lot of them.
> 
> Vladimir Sementsov-Ogievskiy (17):
>   nbd/client: fix nbd_opt_go
>   nbd/client: refactor nbd_read_eof
>   nbd/client: refactor nbd_receive_reply
>   nbd/client: fix nbd_send_request to return int
>   block/nbd-client: get rid of ssize_t
>   block/nbd-client: fix nbd_read_reply_entry
>   block/nbd-client: refactor request send/receive
>   block/nbd-client: rename nbd_recv_coroutines_enter_all
>   block/nbd-client: move nbd_co_receive_reply content into
>     nbd_co_request
>   block/nbd-client: move nbd_coroutine_end content into nbd_co_request
>   block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
>     error
>   block/nbd-client: refactor nbd_co_request
>   block/nbd-client: refactor NBDClientSession.recv_coroutine
>   block/nbd-client: exit reply-reading coroutine on incorrect handle
>   block/nbd-client: refactor reading reply
>   block/nbd-client: drop reply field from NBDClientSession
>   block/nbd-client: always return EIO on and after the first io channel
>     error

I've pushed 1-5 and 7-10 onto my NBD staging branch for 2.11:

  git://repo.or.cz/qemu/ericb.git nbd

with a couple of changes squashed in as mentioned in individual patches;
please double-check that it looks okay.  If so, then I will use that
branch as the starting point for all NBD commits destined for 2.11,
sending a pull request once the tree opens.

Patches 6 and 11 are somewhat subsumed by the work that went into 2.10,
and the remaining patches are starting to cause enough conflicts that
I'd prefer you complete the rebase of patches 12-17 and post a v2 on top
of my staging branch.

I'm also hoping that Stefan will rebase a v2 of his "nbd-client: enter
read_reply_co during init to avoid crash" on top of your preliminary
work, since Paolo had a good suggestion on improving the semantics in
the review of v1.

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

Re: [Qemu-devel] [PATCH 00/17] nbd client refactoring and fixing
Posted by Eric Blake 6 years, 7 months ago
On 08/25/2017 05:10 PM, Eric Blake wrote:
> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> A bit more refactoring and fixing before BLOCK_STATUS series.
>> I've tried to make individual patches simple enough, so there are
>> a lot of them.
>>
>> Vladimir Sementsov-Ogievskiy (17):
>>   nbd/client: fix nbd_opt_go
>>   nbd/client: refactor nbd_read_eof
>>   nbd/client: refactor nbd_receive_reply
>>   nbd/client: fix nbd_send_request to return int
>>   block/nbd-client: get rid of ssize_t
>>   block/nbd-client: fix nbd_read_reply_entry
>>   block/nbd-client: refactor request send/receive
>>   block/nbd-client: rename nbd_recv_coroutines_enter_all
>>   block/nbd-client: move nbd_co_receive_reply content into
>>     nbd_co_request
>>   block/nbd-client: move nbd_coroutine_end content into nbd_co_request
>>   block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
>>     error
>>   block/nbd-client: refactor nbd_co_request
>>   block/nbd-client: refactor NBDClientSession.recv_coroutine
>>   block/nbd-client: exit reply-reading coroutine on incorrect handle
>>   block/nbd-client: refactor reading reply
>>   block/nbd-client: drop reply field from NBDClientSession
>>   block/nbd-client: always return EIO on and after the first io channel
>>     error
> 
> I've pushed 1-5 and 7-10 onto my NBD staging branch for 2.11:
> 
>   git://repo.or.cz/qemu/ericb.git nbd

Correction - I've decided to take Stefan's patches first (since his
patch 1/3 was a bit more elegant at doing the same thing as your patch
10); that caused rebase conflicts for your patch 7 (which I simplified
by creating nbd_co_request as a wrapper rather than trying to inline its
parts), and my change to your patch 7 obsoletes the need for 9 or 10.  I
also placed your patch 8 before your patch 7 (if you don't like what I
changed on patch 7, it is now last in my NBD staging area, so I can more
easily drop it from my tree if you'd prefer to respin it differently).

> with a couple of changes squashed in as mentioned in individual patches;
> please double-check that it looks okay.  If so, then I will use that
> branch as the starting point for all NBD commits destined for 2.11,
> sending a pull request once the tree opens.
> 
> Patches 6 and 11 are somewhat subsumed by the work that went into 2.10,
> and the remaining patches are starting to cause enough conflicts that
> I'd prefer you complete the rebase of patches 12-17 and post a v2 on top
> of my staging branch.

These statements still hold; you'll need to rebase the rest of your
series on top of my tree.

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