Re: [Qemu-devel] [PATCH 4/9] block/qed: use buffer-based io

[Kevin Wolf <kwolf@redhat.com>]

Am 22.04.2019 um 16:58 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Move to _co_ versions of io functions qed_read_table() and
> qed_write_table(), as we use qemu_co_mutex_unlock()
> anyway.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qed-table.c | 12 +++++-------
>  block/qed.c       |  6 ++----
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/block/qed-table.c b/block/qed-table.c
> index c497bd4aec..cf30edd977 100644
> --- a/block/qed-table.c
> +++ b/block/qed-table.c
> @@ -21,22 +21,22 @@
>  /* Called with table_lock held.  */
>  static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table)
>  {
> -    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(
> -        qiov, table->offsets, s->header.cluster_size * s->header.table_size);
> +    unsigned int bytes = s->header.cluster_size * s->header.table_size;
> +
>      int noffsets;
>      int i, ret;
>  
>      trace_qed_read_table(s, offset, table);
>  
>      qemu_co_mutex_unlock(&s->table_lock);
> -    ret = bdrv_preadv(s->bs->file, offset, &qiov);
> +    ret = bdrv_co_pread(s->bs->file, offset, bytes, table->offsets, 0);

Careful! This function is not marked as coroutine_fn, and I remember
that there were some non-coroutine callers when I converted qed to
coroutines.

It looks like we're lucky and all callers have been converted to
coroutines meanwhile, but I would prefer if we added the coroutine_fn
marker everywhere where we rely on it now to document this fact.

>      qemu_co_mutex_lock(&s->table_lock);
>      if (ret < 0) {
>          goto out;
>      }
>  
>      /* Byteswap offsets */
> -    noffsets = qiov.size / sizeof(uint64_t);
> +    noffsets = bytes / sizeof(uint64_t);
>      for (i = 0; i < noffsets; i++) {
>          table->offsets[i] = le64_to_cpu(table->offsets[i]);
>      }
> @@ -66,7 +66,6 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
>      unsigned int sector_mask = BDRV_SECTOR_SIZE / sizeof(uint64_t) - 1;
>      unsigned int start, end, i;
>      QEDTable *new_table;
> -    QEMUIOVector qiov;
>      size_t len_bytes;
>      int ret;
>  
> @@ -79,7 +78,6 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
>      len_bytes = (end - start) * sizeof(uint64_t);
>  
>      new_table = qemu_blockalign(s->bs, len_bytes);
> -    qemu_iovec_init_buf(&qiov, new_table->offsets, len_bytes);
>  
>      /* Byteswap table */
>      for (i = start; i < end; i++) {
> @@ -91,7 +89,7 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
>      offset += start * sizeof(uint64_t);
>  
>      qemu_co_mutex_unlock(&s->table_lock);
> -    ret = bdrv_pwritev(s->bs->file, offset, &qiov);
> +    ret = bdrv_co_pwrite(s->bs->file, offset, len_bytes, new_table->offsets, 0);
>      qemu_co_mutex_lock(&s->table_lock);
>      trace_qed_write_table_cb(s, table, flush, ret);
>      if (ret < 0) {

Same for the callers of this function.

Kevin