Re: [Qemu-devel] [PATCH 06/27] block/parallels: provide _co_readv routine for parallels format driver

[Denis V. Lunev]

On 22/04/15 15:41, Stefan Hajnoczi wrote:
> On Wed, Mar 11, 2015 at 01:28:00PM +0300, Denis V. Lunev wrote:
> > Main approach is taken from qcow2_co_readv.
> >
> > The patch drops coroutine lock for the duration of IO operation and
> > peforms normal scatter-gather IO using standard QEMU backend.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > Reviewed-by: Roman Kagan <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> > CC: Stefan Hajnoczi <address@hidden>
> > ---
> >   block/parallels.c | 46 +++++++++++++++++++++++++++-------------------
> >   1 file changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/block/parallels.c b/block/parallels.c
> > index b469984..64b169b 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -186,37 +186,45 @@ static int64_t coroutine_fn 
> > parallels_co_get_block_status(BlockDriverState *bs,
> >           BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> >   }
> >   
> > -static int parallels_read(BlockDriverState *bs, int64_t sector_num,
> > -                    uint8_t *buf, int nb_sectors)
> > +static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
> > +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> >   {
> >       BDRVParallelsState *s = bs->opaque;
> > +    uint64_t bytes_done = 0;
> > +    QEMUIOVector hd_qiov;
> > +    int ret = 0;
> >   
> > +    qemu_iovec_init(&hd_qiov, qiov->niov);
> > +
> > +    qemu_co_mutex_lock(&s->lock);
> >       while (nb_sectors > 0) {
> >           int64_t position = seek_to_sector(s, sector_num);
> >           int n = cluster_remainder(s, sector_num, nb_sectors);
> > -        if (position >= 0) {
> > -            int ret = bdrv_read(bs->file, position, buf, n);
> > +        int nbytes = n << BDRV_SECTOR_BITS;
> > +
> > +        if (position < 0) {
> > +            qemu_iovec_memset(qiov, bytes_done, 0, nbytes);
> > +        } else {
> > +            qemu_iovec_reset(&hd_qiov);
> > +            qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
> > +
> > +            qemu_co_mutex_unlock(&s->lock);
> > +            ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
> > +            qemu_co_mutex_lock(&s->lock);
> > +
> >               if (ret < 0) {
> > -                return ret;
> > +                goto fail;
> The lock is never unlocked if the goto is taken.

nice catch! We have missed that...

> >               }
> > -        } else {
> > -            memset(buf, 0, n << BDRV_SECTOR_BITS);
> >           }
> > +
> >           nb_sectors -= n;
> >           sector_num += n;
> > -        buf += n << BDRV_SECTOR_BITS;
> > +        bytes_done += nbytes;
> >       }
> > -    return 0;
> > -}
> > -
> > -static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t 
> > sector_num,
> > -                                          uint8_t *buf, int nb_sectors)
> > -{
> > -    int ret;
> > -    BDRVParallelsState *s = bs->opaque;
> > -    qemu_co_mutex_lock(&s->lock);
> > -    ret = parallels_read(bs, sector_num, buf, nb_sectors);
> >       qemu_co_mutex_unlock(&s->lock);
> > +
> > +fail:
> > +    qemu_iovec_destroy(&hd_qiov);
> >       return ret;
> >   }