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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@redhat.com>
>> ---
>>   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