Re: [Qemu-devel] [PATCH 3/5] qcow2: Employ metadata overlap checks

[Max Reitz]

Am 27.08.2013 13:32, schrieb Kevin Wolf:
> Am 26.08.2013 um 15:04 hat Max Reitz geschrieben:
> > The pre-write overlap check function is now called before most of the
> > qcow2 writes (aborting it on collision or other error).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/qcow2-cache.c    | 17 +++++++++++++++++
> >   block/qcow2-cluster.c  | 23 +++++++++++++++++++++++
> >   block/qcow2-snapshot.c | 24 ++++++++++++++++++++++++
> >   block/qcow2.c          | 38 +++++++++++++++++++++++++++++++++++++-
> >   4 files changed, 101 insertions(+), 1 deletion(-)
> > @@ -368,6 +384,13 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
> >                           &s->aes_encrypt_key);
> >       }
> >   
> > +    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> > +            ((cluster_offset >> 9) + n_start) << 9, n * BDRV_SECTOR_SIZE);
> Looks a bit overcomplicated, I'd like something like this better:
>
> cluster_offset + n_start * BDRV_SECTOR_SIZE
Yes, but this wouldn't correspond with the write call if (cluster_offset 
& ((1 << 9) - 1)) != 0. ;-)

Basically, I just wanted it to match exactly the write command.

> > +    if (ret) {
> > +        ret = (ret < 0) ? ret : -EIO;
> I wonder whether the -EIO logic should be moved into
> qcow2_pre_write_overlap_check(). Currently each single caller seems to
> have this check.
Seems reasonable. I didn't want to prevent the caller from receiving 
information about the exact overlap, but that could be achieved through 
an optional result pointer as well, I think.

> > +        goto out;
> > +    }
> > +
> >       BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
> >       ret = bdrv_co_writev(bs->file, (cluster_offset >> 9) + n_start, n, &qiov);
> >       if (ret < 0) {
> > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> > index 0caac90..6f69ecc 100644
> > --- a/block/qcow2-snapshot.c
> > +++ b/block/qcow2-snapshot.c
> > @@ -189,6 +189,15 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> >           return ret;
> >       }
> >   
> > +    /* The snapshot list position has not yet been updated, so these clusters
> > +     * must indeed be completely free */
> > +    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> > +                                        offset, s->nb_snapshots * sizeof(h));
> > +    if (ret) {
> > +        return (ret < 0) ? ret : -EIO;
> > +    }
> This doesn't check the full size. snapshots_size should have the right
> value.
Yes, you're right.

Max