Re: [Qemu-block] [PATCH 05/11] blockjob: separate monitor and blockjob APIs

On 09/05/2017 19:06, Jeff Cody wrote: >> Keep the two APIs separate in the blockjob.c file too. This will >> be useful when transitioning away from the AioContext lock, because >> there will be locking policies for the two categories, too---the >> monitor will have to call new

[Qemu-block] [PATCH v2 15/20] backup: Switch block_backup.h to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Continue by converting the public interface to backup jobs (no semantic change), including a change to CowRequest to track by bytes instead of cluster indices. Note that this does not

[Qemu-block] [PATCH v2 19/20] block: Minimize raw use of bds->total_sectors

bdrv_is_allocated_above() was relying on intermediate->total_sectors, which is a field that can have stale contents depending on the value of intermediate->has_variable_length. An audit shows that we are safe (we were first calling through bdrv_co_get_block_status() which in turn calls

[Qemu-block] [PATCH v2 10/20] mirror: Switch mirror_cow_align() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Convert another internal function (no semantic change), and add mirror_clip_bytes() as a counterpart to mirror_clip_sectors(). Some of the conversion is a bit tricky, requiring temporaries

[Qemu-block] [PATCH v2 20/20] block: Make bdrv_is_allocated_above() byte-based

We are gradually moving away from sector-based interfaces, towards byte-based. In the common case, allocation is unlikely to ever use values that are not naturally sector-aligned, but it is possible that byte-based values will let us be more precise about allocation at the end of an unaligned

[Qemu-block] [PATCH v2 18/20] block: Make bdrv_is_allocated() byte-based

We are gradually moving away from sector-based interfaces, towards byte-based. In the common case, allocation is unlikely to ever use values that are not naturally sector-aligned, but it is possible that byte-based values will let us be more precise about allocation at the end of an unaligned

[Qemu-block] [PATCH v2 17/20] backup: Switch backup_run() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Change the internal loop iteration of backups to track by bytes instead of sectors (although we are still guaranteed that we iterate by steps that are cluster-aligned). Signed-off-by: Eric

[Qemu-block] [PATCH v2 07/20] mirror: Switch MirrorBlockJob to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Continue by converting an internal structure (no semantic change), and all references to the buffer size. [checkpatch has a false positive on use of MIN() in this patch] Signed-off-by:

[Qemu-block] [PATCH v2 13/20] block: Drop unused bdrv_round_sectors_to_clusters()

Now that the last user [mirror_iteration()] has converted to using bytes, we no longer need a function to round sectors to clusters. Signed-off-by: Eric Blake Reviewed-by: John Snow --- v2: hoist to earlier series, no change --- include/block/block.h | 4

[Qemu-block] [PATCH v2 16/20] backup: Switch backup_do_cow() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Convert another internal function (no semantic change). Signed-off-by: Eric Blake Reviewed-by: John Snow --- v2: no change --- block/backup.c | 62

[Qemu-block] [PATCH v2 09/20] mirror: Update signature of mirror_clip_sectors()

Rather than having a void function that modifies its input in-place as the output, change the signature to reduce a layer of indirection and return the result. Suggested-by: John Snow Signed-off-by: Eric Blake --- v2: new patch --- block/mirror.c | 15

[Qemu-block] [PATCH v2 06/20] commit: Switch commit_run() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Change the internal loop iteration of committing to track by bytes instead of sectors (although we are still guaranteed that we iterate by steps that are sector-aligned). Signed-off-by:

[Qemu-block] [PATCH v2 14/20] backup: Switch BackupBlockJob to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Continue by converting an internal structure (no semantic change), and all references to tracking progress. Drop a redundant local variable bytes_per_cluster. Signed-off-by: Eric Blake

[Qemu-block] [PATCH v2 11/20] mirror: Switch mirror_do_read() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Convert another internal function (no semantic change). Signed-off-by: Eric Blake Reviewed-by: John Snow --- v2: rebase to earlier changes ---

[Qemu-block] [PATCH v2 04/20] stream: Switch stream_run() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Change the internal loop iteration of streaming to track by bytes instead of sectors (although we are still guaranteed that we iterate by steps that are sector-aligned). Signed-off-by:

[Qemu-block] [PATCH v2 08/20] mirror: Switch mirror_do_zero_or_discard() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Convert another internal function (no semantic change). Signed-off-by: Eric Blake Reviewed-by: John Snow --- v2: no change --- block/mirror.c | 20

[Qemu-block] [PATCH v2 05/20] commit: Switch commit_populate() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Start by converting an internal function (no semantic change). Signed-off-by: Eric Blake Reviewed-by: John Snow --- v2: no change ---

[Qemu-block] [PATCH v2 12/20] mirror: Switch mirror_iteration() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Change the internal loop iteration of mirroring to track by bytes instead of sectors (although we are still guaranteed that we iterate by steps that are both sector-aligned and multiples of

[Qemu-block] [PATCH v2 02/20] trace: Show blockjob actions via bytes, not sectors

Upcoming patches are going to switch to byte-based interfaces instead of sector-based. Even worse, trace_backup_do_cow_enter() had a weird mix of cluster and sector indices. The trace interface is low enough that there are no stability guarantees, and therefore nothing wrong with changing our

[Qemu-block] [PATCH v2 03/20] stream: Switch stream_populate() to byte-based

We are gradually converting to byte-based interfaces, as they are easier to reason about than sector-based. Start by converting an internal function (no semantic change). Signed-off-by: Eric Blake Reviewed-by: John Snow --- v2: no change ---

[Qemu-block] [PATCH v2 01/20] blockjob: Track job ratelimits via bytes, not sectors

The user interface specifies job rate limits in bytes/second. It's pointless to have our internal representation track things in sectors/second, particularly since we want to move away from sector-based interfaces. Fix up a doc typo found while verifying that the ratelimit code handles the

[Qemu-block] [PATCH v2 00/20] make bdrv_is_allocated[_above] byte-based

There are patches floating around to add NBD_CMD_BLOCK_STATUS, but NBD wants to report status on byte granularity (even if the reporting will probably be naturally aligned to sectors or even much higher levels). I've therefore started the task of converting our block status code to report at a

Re: [Qemu-block] [PATCH 16/17] block: Make bdrv_is_allocated() byte-based

On 04/19/2017 04:40 PM, John Snow wrote: > > > On 04/19/2017 05:12 PM, Eric Blake wrote: >> On 04/19/2017 03:32 PM, John Snow wrote: >> @@ -279,9 +280,9 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds) /* Skip unallocated sectors; intentionally

Re: [Qemu-block] [Qemu-devel] [PATCH 17/17] block: Make bdrv_is_allocated_above() byte-based

On 04/24/2017 08:48 PM, Eric Blake wrote: >> >> >>> -n = psectors_inter; >>> + offset + pnum_inter < (intermediate->total_sectors * >>> +BDRV_SECTOR_SIZE))) { >> >> Naive question: not worth using either bdrv_getlength for bytes, or the

Re: [Qemu-block] [Qemu-devel] [PATCH 5/7] curl: convert CURLAIOCB to byte values

On 05/10/2017 12:36 PM, Max Reitz wrote: > On a very surprised level: Why does curl return the content length as a > double?! I can't think of a reason where that might be a good idea. If > your integer no longer fits into a uin64_t, the double will be inexact, > so it pretty much is useless,

Re: [Qemu-block] [Qemu-devel] [PATCH] blockdev: use drained_begin/end for qmp_block_resize

On 05/10/2017 12:39 PM, John Snow wrote: > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1447551 > > If one tries to issue a block_resize while a guest is busy > accessing the disk, it is possible that qemu may deadlock > when invoking aio_poll from both the main loop and the iothread. > >

Re: [Qemu-block] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState

On 10.05.2017 16:32, Paolo Bonzini wrote: > Instead, put the CURLAIOCB on a wait list; curl_clean_state will > wake the corresponding coroutine. > > Because of CURL's callback-based structure, we cannot easily convert > everything to CoMutex/CoQueue; keeping the QemuMutex is simpler. > However,

Re: [Qemu-block] [PATCH 6/7] curl: convert readv to coroutines

On 10.05.2017 16:32, Paolo Bonzini wrote: > This is pretty simple. The bottom half goes away because, unlike > bdrv_aio_readv, coroutine-based read can return immediately without > yielding. However, for simplicity I kept the former bottom half > handler in a separate function. > >

[Qemu-block] [PATCH] blockdev: use drained_begin/end for qmp_block_resize

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1447551 If one tries to issue a block_resize while a guest is busy accessing the disk, it is possible that qemu may deadlock when invoking aio_poll from both the main loop and the iothread. Replace another instance of bdrv_drain_all that doesn't

Re: [Qemu-block] [PATCH 5/7] curl: convert CURLAIOCB to byte values

On 10.05.2017 16:32, Paolo Bonzini wrote: > This is in preparation for the conversion from bdrv_aio_readv to > bdrv_co_preadv, and it also requires changing some of the size_t values > to uint64_t. This was broken before for disks > 2TB, but now it would > break at 4GB. > > Signed-off-by: Paolo

Re: [Qemu-block] [PATCH 4/7] curl: split curl_find_state/curl_init_state

On 10.05.2017 16:32, Paolo Bonzini wrote: > If curl_easy_init fails, a CURLState is left with s->in_use = 1. Split > curl_init_state in two, so that we can distinguish the two failures and > call curl_clean_state if needed. > > While at it, simplify curl_find_state, removing a dummy loop. The >

Re: [Qemu-block] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex

On 10.05.2017 16:32, Paolo Bonzini wrote: > The curl driver has a ugly hack where, if it cannot find an empty CURLState, > it just uses aio_poll to wait for one to be empty. This is probably > buggy when used together with dataplane, and the simplest way to fix it > is to use coroutines instead.

Re: [Qemu-block] [PATCH 2/7] curl: never invoke callbacks with s->mutex held

On 10.05.2017 16:32, Paolo Bonzini wrote: > All curl callbacks go through curl_multi_do, and hence are called with > s->mutex held. Note that with comments, and make curl_read_cb drop the > lock before invoking the callback. > > Likewise for curl_find_buf, where the callback can be invoked by

Re: [Qemu-block] [PATCH 1/7] curl: strengthen assertion in curl_clean_state

On 10.05.2017 16:31, Paolo Bonzini wrote: > curl_clean_state should only be called after all AIOCBs have been > completed. This is not so obvious for the call from curl_detach_aio_context, > so assert that. > > Cc: qemu-sta...@nongnu.org > Cc: jc...@redhat.com > Signed-off-by: Paolo Bonzini

Re: [Qemu-block] [Qemu-devel] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

On 10.05.2017 17:55, Paolo Bonzini wrote: > > > On 10/05/2017 17:50, Thomas Huth wrote: >> We likely do not want to carry these legacy -drive options along forever. >> Let's emit a deprecation warning for the -drive options that have a >> replacement with the -device option, so that the

Re: [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote: > Since the last patch in v1 didn't work, I bit the bullet and converted > the whole thing to coroutines (patches 4-6). This in turns allows a more > elegant solution to wait for CURLStates to get free (patch 7). > > I tested this by

Re: [Qemu-block] [Qemu-devel] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

On 10/05/2017 17:50, Thomas Huth wrote: > We likely do not want to carry these legacy -drive options along forever. > Let's emit a deprecation warning for the -drive options that have a > replacement with the -device option, so that the (hopefully few) remaining > users are aware of this and can

[Qemu-block] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

We likely do not want to carry these legacy -drive options along forever. Let's emit a deprecation warning for the -drive options that have a replacement with the -device option, so that the (hopefully few) remaining users are aware of this and can adapt their scripts / behaviour. Signed-off-by:

Re: [Qemu-block] [Qemu-devel] [PATCH 17/17] block: Make bdrv_is_allocated_above() byte-based

On 04/24/2017 08:48 PM, Eric Blake wrote: > On 04/24/2017 06:06 PM, John Snow wrote: >> >> >> On 04/11/2017 06:29 PM, Eric Blake wrote: >>> We are gradually moving away from sector-based interfaces, towards >>> byte-based. In the common case, allocation is unlikely to ever use >>> values that are

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll Type: series Message-id: 20170510143205.32013-1-pbonz...@redhat.com === TEST SCRIPT BEGIN

Re: [Qemu-block] [Qemu-devel] [PATCH] mirror: add sync mode incremental to drive-mirror and blockdev-mirror

On 05/10/2017 05:00 PM, Stefan Hajnoczi wrote: > On Wed, May 10, 2017 at 03:25:31PM +0200, Denis V. Lunev wrote: >> On 05/09/2017 06:52 PM, Stefan Hajnoczi wrote: >>> On Mon, May 08, 2017 at 05:07:18PM -0400, John Snow wrote: On 05/08/2017 05:02 PM, Denis V. Lunev wrote: > On 05/08/2017

Re: [Qemu-block] [Qemu-devel] [PATCH] mirror: add sync mode incremental to drive-mirror and blockdev-mirror

On Wed, May 10, 2017 at 03:25:31PM +0200, Denis V. Lunev wrote: > On 05/09/2017 06:52 PM, Stefan Hajnoczi wrote: > > On Mon, May 08, 2017 at 05:07:18PM -0400, John Snow wrote: > >> > >> On 05/08/2017 05:02 PM, Denis V. Lunev wrote: > >>> On 05/08/2017 10:35 PM, Stefan Hajnoczi wrote: > On

[Qemu-block] [PATCH 5/7] curl: convert CURLAIOCB to byte values

This is in preparation for the conversion from bdrv_aio_readv to bdrv_co_preadv, and it also requires changing some of the size_t values to uint64_t. This was broken before for disks > 2TB, but now it would break at 4GB. Signed-off-by: Paolo Bonzini --- block/curl.c | 44

[Qemu-block] [PATCH 4/7] curl: split curl_find_state/curl_init_state

If curl_easy_init fails, a CURLState is left with s->in_use = 1. Split curl_init_state in two, so that we can distinguish the two failures and call curl_clean_state if needed. While at it, simplify curl_find_state, removing a dummy loop. The aio_poll loop is moved to the sole caller that needs

[Qemu-block] [PATCH 6/7] curl: convert readv to coroutines

This is pretty simple. The bottom half goes away because, unlike bdrv_aio_readv, coroutine-based read can return immediately without yielding. However, for simplicity I kept the former bottom half handler in a separate function. Signed-off-by: Paolo Bonzini ---

[Qemu-block] [PATCH 7/7] curl: do not do aio_poll when waiting for a free CURLState

Instead, put the CURLAIOCB on a wait list; curl_clean_state will wake the corresponding coroutine. Because of CURL's callback-based structure, we cannot easily convert everything to CoMutex/CoQueue; keeping the QemuMutex is simpler. However, CoQueue is a simple wrapper around a linked list, so we

[Qemu-block] [PATCH 1/7] curl: strengthen assertion in curl_clean_state

curl_clean_state should only be called after all AIOCBs have been completed. This is not so obvious for the call from curl_detach_aio_context, so assert that. Cc: qemu-sta...@nongnu.org Cc: jc...@redhat.com Signed-off-by: Paolo Bonzini --- block/curl.c | 5 + 1 file

[Qemu-block] [PATCH 2/7] curl: never invoke callbacks with s->mutex held

All curl callbacks go through curl_multi_do, and hence are called with s->mutex held. Note that with comments, and make curl_read_cb drop the lock before invoking the callback. Likewise for curl_find_buf, where the callback can be invoked by the caller. Cc: qemu-sta...@nongnu.org Cc:

[Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

Since the last patch in v1 didn't work, I bit the bullet and converted the whole thing to coroutines (patches 4-6). This in turns allows a more elegant solution to wait for CURLStates to get free (patch 7). I tested this by lowering CURL_NUM_STATES to 2. With this change, the buggy case

[Qemu-block] [PATCH 3/7] curl: avoid recursive locking of BDRVCURLState mutex

The curl driver has a ugly hack where, if it cannot find an empty CURLState, it just uses aio_poll to wait for one to be empty. This is probably buggy when used together with dataplane, and the simplest way to fix it is to use coroutines instead. A more immediate effect of the bug however is

Re: [Qemu-block] [Qemu-devel] [PATCH] mirror: add sync mode incremental to drive-mirror and blockdev-mirror

On 05/09/2017 06:52 PM, Stefan Hajnoczi wrote: > On Mon, May 08, 2017 at 05:07:18PM -0400, John Snow wrote: >> >> On 05/08/2017 05:02 PM, Denis V. Lunev wrote: >>> On 05/08/2017 10:35 PM, Stefan Hajnoczi wrote: On Thu, May 04, 2017 at 12:54:40PM +0200, Daniel Kucera wrote: Seems