On Fri, Apr 16, 2021 at 8:23 AM Thomas Huth wrote:
>
> A customer reported that running
>
> qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2
>
> fails for them with the following error message when the images are
> stored on a GPFS file system:
>
> qemu-img: error while
Currently LBAF formats are being intialized based on metadata
size if and only if nvme-ns "ms" parameter is non-zero value.
Since FormatNVM command being supported device parameter "ms"
may not be the criteria to initialize the supported LBAFs.
Signed-off-by: Gollu Appalanaidu
---
-v3: Remove
QEMU currently crashes when the user tries to do something like:
qemu-system-x86_64 -M x-remote -device piix3-ide
This happens because the "isabus" variable is not initialized with
the x-remote machine yet. Add a proper check for this condition
and propagate the error to the caller, so we can
On Apr 16 12:52, Gollu Appalanaidu wrote:
Currently in compare command metadata aio read blk_aio_preadv return
value ignored, consider it and complete the block accounting.
Signed-off-by: Gollu Appalanaidu
---
hw/block/nvme.c | 11 +++
1 file changed, 11 insertions(+)
diff --git
Currently LBAF formats are being intialized based on metadata
size if and only if nvme-ns "ms" parameter is non-zero value.
Since FormatNVM command being supported device parameter "ms"
may not be the criteria to initialize the supported LBAFs.
Signed-off-by: Gollu Appalanaidu
---
-v2:
On Fri, Apr 16, 2021 at 10:48:16AM +0200, Klaus Jensen wrote:
On Apr 16 12:52, Gollu Appalanaidu wrote:
Currently LBAF formats are being intialized based on metadata
size if and only if nvme-ns "ms" parameter is non-zero value.
Since FormatNVM command being supported device parameter "ms"
may
On Apr 16 12:52, Gollu Appalanaidu wrote:
Currently LBAF formats are being intialized based on metadata
size if and only if nvme-ns "ms" parameter is non-zero value.
Since FormatNVM command being supported device parameter "ms"
may not be the criteria to initialize the supported LBAFs.
We are going to reuse the script to generate a qcow2_ function in
further commit. Prepare the script now.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/block-coroutine-wrapper.py | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git
We'll need a possibility of non-blocking nbd_co_establish_connection(),
so that it returns immediately, and it returns success only if
connections was previously established in background.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 2 +-
block/nbd.c |
block/nbd doesn't need underlying sioc channel anymore. So, we can
update nbd/client-connection interface to return only one top-most io
channel, which is more straight forward.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 4 ++--
block/nbd.c | 13
nbd_open() does it (through nbd_establish_connection()).
Actually we lost that call on reconnect path in 1dc4718d849e1a1fe
"block/nbd: use non-blocking connect: fix vm hang on connect()"
when we have introduced reconnect thread.
Fixes: 1dc4718d849e1a1fe665ce5241ed79048cfa2cfc
Signed-off-by:
OK, that's a big rewrite of the logic.
Pre-patch we have an always running coroutine - connection_co. It does
reply receiving and reconnecting. And it leads to a lot of difficult
and unobvious code around drained sections and context switch. We also
abuse bs->in_flight counter which is increased
16.04.2021 11:14, Vladimir Sementsov-Ogievskiy wrote:
16.04.2021 11:09, Vladimir Sementsov-Ogievskiy wrote:
OK, that's a big rewrite of the logic.
Pre-patch we have an always running coroutine - connection_co. It does
reply receiving and reconnecting. And it leads to a lot of difficult
and
Add an option for thread to retry connection until success. We'll use
nbd/client-connection both for reconnect and for initial connection in
nbd_open(), so we need a possibility to use same NBDClientConnection
instance to connect once in nbd_open() and then use retry semantics for
reconnect.
Add arguments and logic to support nbd negotiation in the same thread
after successful connection.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 9 +++-
block/nbd.c | 4 +-
nbd/client-connection.c | 105 ++--
3
req->receiving is a flag of request being in one concrete yield point
in nbd_co_do_receive_one_chunk().
Such kind of boolean flag is always better to unset before scheduling
the coroutine, to avoid double scheduling. So, let's be more careful.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
The only last step we need to reuse the function is coroutine-wrapper.
nbd_open() may be called from non-coroutine context. So, generate the
wrapper and use it.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/coroutines.h | 6 +++
block/nbd.c| 101
16.04.2021 11:09, Vladimir Sementsov-Ogievskiy wrote:
OK, that's a big rewrite of the logic.
Pre-patch we have an always running coroutine - connection_co. It does
reply receiving and reconnecting. And it leads to a lot of difficult
and unobvious code around drained sections and context switch.
monitor_cur() is used by socket_get_fd, but it doesn't work in
connection thread. Let's monitor directly to cover this thing. We are
going to unify connection establishing path in nbd_open and reconnect,
so we should support fd-passing.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
Use a new possibility of negotiation in connect thread. Now we are on
the way of simplifying connection_co. We want to move the whole
reconnect code to NBDClientConnection. NBDClientConnection already
updated to support negotiation and retry. Use now the first thing.
Signed-off-by: Vladimir
We are going to move connection code to own file and want clear names
and APIs.
The structure is shared between user and (possibly) several runs of
connect-thread. So it's wrong to call it "thread". Let's rename to
something more generic.
Appropriately rename connect_thread and thr variables to
To be reused in the following patch.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 99 ++---
1 file changed, 57 insertions(+), 42 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 5e63caaf4b..03ffe95231 100644
---
We already have two similar helpers for other state. Let's add another
one for convenience.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index
Split out part, that we want to reuse for nbd_open().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 79 +++--
1 file changed, 41 insertions(+), 38 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 15b5921725..59971bfba8
Currently sioc pointer is used just to pass from socket-connection to
nbd negotiation. Drop the field, and use local variables instead. With
next commit we'll update nbd/client-connection.c to behave
appropriately (return only top-most ioc, not two channels).
Signed-off-by: Vladimir
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 43 ++-
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 21a4039359..8531d019b2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -118,7 +118,7 @@
Detecting monitor by current coroutine works bad when we are not in
coroutine context. And that's exactly so in nbd reconnect code, where
qio_channel_socket_connect_sync() is called from thread.
Add a possibility to pass monitor by hand, to be used in the following
commit.
Signed-off-by:
Now, when thread can do negotiation and retry, it may run relatively
long. We need a mechanism to stop it, when user is not interested in
result anymore. So, on nbd_client_connection_release() let's shutdown
the socked, and do not retry connection if thread is detached.
Signed-off-by: Vladimir
We now have bs-independent connection API, which consists of four
functions:
nbd_client_connection_new()
nbd_client_connection_unref()
nbd_co_establish_connection()
nbd_co_establish_connection_cancel()
Move them to a separate file together with NBDClientConnection
structure which becomes
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
nbd/client-connection.c | 94 ++---
1 file changed, 42 insertions(+), 52 deletions(-)
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 4e39a5b1af..b45a0bd5f6 100644
---
Negotiation during reconnect is now done in thread, and s->sioc is not
available during negotiation. Negotiation in thread will be cancelled
by nbd_client_connection_release() called from nbd_clear_bdrvstate().
So, we don't need this code chunk anymore.
Signed-off-by: Vladimir Sementsov-Ogievskiy
This is the last step of creating bs-independent nbd connection
interface. With next commit we can finally move it to separate file.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Roman Kagan
---
block/nbd.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff
nbd_co_establish_connection_cancel() actually needs only pointer to
NBDConnectThread. So, make it clean.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Roman Kagan
---
block/nbd.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/block/nbd.c
We are going to split connection code to separate file. Now we are
ready to give nbd_co_establish_connection() clean and bs-independent
interface.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Roman Kagan
---
block/nbd.c | 49 +++--
1 file
We don't need all these states. The code refactored to use two boolean
variables looks simpler.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 125 ++--
1 file changed, 34 insertions(+), 91 deletions(-)
diff --git a/block/nbd.c
With the following patch we want to call wake coroutine from thread.
And it doesn't work with aio_co_wake:
Assume we have no iothreads.
Assume we have a coroutine A, which waits in the yield point for
external aio_co_wake(), and no progress can be done until it happen.
Main thread is in blocking
Instead of connect_bh, bh_ctx and wait_connect fields we can live with
only one link to waiting coroutine, protected by mutex.
So new logic is:
nbd_co_establish_connection() sets wait_co under mutex, release the
mutex and do yield(). Note, that wait_co may be scheduled by thread
immediately
We have two "return error" paths in nbd_open() after
nbd_process_options(). Actually we should call nbd_clear_bdrvstate()
on these paths. Interesting that nbd_process_options() calls
nbd_clear_bdrvstate() by itself.
Let's fix leaks and refactor things to be more obvious:
- intialize yank at top
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index 272af60b44..6efa11a185 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1891,6 +1891,8 @@ static int nbd_client_handshake(BlockDriverState *bs,
Error
From: Roman Kagan
Simplify lifetime management of BDRVNBDState->connect_thread by
delaying the possible cleanup of it until the BDRVNBDState itself goes
away.
This also reverts
0267101af6 "block/nbd: fix possible use after free of s->connect_thread"
as now s->connect_thread can't be cleared
These fields are write-only. Drop them.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Roman Kagan
---
block/nbd.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 6efa11a185..d67556c7ee 100644
--- a/block/nbd.c
+++
From: Roman Kagan
nbd_free_connect_thread leaks the channel object if it hasn't been
stolen.
Unref it and fix the leak.
Signed-off-by: Roman Kagan
---
block/nbd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/nbd.c b/block/nbd.c
index 1d4668d42d..739ae2941f 100644
---
The series substitutes "[PATCH v2 00/10] block/nbd: move connection code to
separate file"
Supersedes: <20210408140827.332915-1-vsement...@virtuozzo.com>
so it's called v3
block/nbd.c is overcomplicated. These series is a big refactoring, which
finally drops all the complications around drained
16.04.2021 10:11, Max Reitz wrote:
On 16.04.21 09:05, Max Reitz wrote:
On 15.04.21 20:46, Vladimir Sementsov-Ogievskiy wrote:
[...]
Note, that if cancelling all in-flight requests on target is wrong on mirror
cancel, we still don't have real bug, as the only implementation of
Currently in compare command metadata aio read blk_aio_preadv return
value ignored, consider it and complete the block accounting.
Signed-off-by: Gollu Appalanaidu
---
hw/block/nvme.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index
Currently LBAF formats are being intialized based on metadata
size if and only if nvme-ns "ms" parameter is non-zero value.
Since FormatNVM command being supported device parameter "ms"
may not be the criteria to initialize the supported LBAFs.
Signed-off-by: Gollu Appalanaidu
---
On 16.04.21 09:05, Max Reitz wrote:
On 15.04.21 20:46, Vladimir Sementsov-Ogievskiy wrote:
[...]
Note, that if cancelling all in-flight requests on target is wrong on
mirror cancel, we still don't have real bug, as the only
implementation of .bdrv_cancel_in_flight is stopping reconnect
On 15.04.21 20:46, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
Recently I've implemented fast-cancelling of mirror job: do
bdrv_cancel_in_flight() in mirror_cancel().
Now I'm in doubt: is it a correct thing? I heard, that mirror-cancel is
a kind of valid mirror completion..
Looking at
48 matches
Mail list logo
