qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v10 8/9] qcow2: skip writing zero buffers to emp


From: Anton Nefedov
Subject: Re: [Qemu-devel] [PATCH v10 8/9] qcow2: skip writing zero buffers to empty COW areas
Date: Thu, 13 Dec 2018 13:57:49 +0000

On 13/12/2018 3:02 PM, Vladimir Sementsov-Ogievskiy wrote:
> 03.12.2018 13:14, Anton Nefedov wrote:
>> If COW areas of the newly allocated clusters are zeroes on the backing image,
>> efficient bdrv_write_zeroes(flags=BDRV_REQ_ALLOCATE) can be used on the whole
>> cluster instead of writing explicit zero buffers later in perform_cow().
>>
> 
> [...]
> 
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2015,6 +2015,11 @@ static bool merge_cow(uint64_t offset, unsigned bytes,
>>                continue;
>>            }
>>    
>> +        /* If COW regions are handled already, skip this too */
>> +        if (m->skip_cow) {
>> +            continue;
>> +        }
>> +
>>            /* The data (middle) region must be immediately after the
>>             * start region */
>>            if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
>> @@ -2040,6 +2045,68 @@ static bool merge_cow(uint64_t offset, unsigned bytes,
>>        return false;
>>    }
>>    
>> +static bool is_unallocated(BlockDriverState *bs, int64_t offset, int64_t 
>> bytes)
>> +{
>> +    int64_t nr;
>> +    return !bytes ||
>> +        (!bdrv_is_allocated_above(bs, NULL, offset, bytes, &nr) && nr == 
>> bytes);
> 
> hm, nr may be < bytes if it is up to file length. And we lose this case, 
> when, it
> may be considered as unallocated too.
> 
> Doesn't harm, however.
> 

Thanks guys for your review.

This case I think is too rare to care about.

>> +}
>> +
>> +static bool is_zero_cow(BlockDriverState *bs, QCowL2Meta *m)
>> +{
>> +    /* This check is designed for optimization shortcut so it must be
>> +     * efficient.
>> +     * Instead of is_zero(), use is_unallocated() as it is faster (but not
>> +     * as accurate and can result in false negatives). */
> 
> But, in case of allocated zeros, we'll read them anyway (as part of COW 
> process),
> so, it may be handled in the same way too. May be not here, but after read we 
> can
> check for zeroes, and again effectively write zeros to the whole cluster.
> 
> Again it may be done separately, I don't sure it worth doing.
> 

Detecting zeroes after we read them (and not here) might make sense;
performance gain should be about the same (minus some CPU to check the
read buffer for zeroes).

The question is how frequent it might hit:
  - raw backing image with holes
  - qcow2 backing image with sub-cluster zero areas
    (e.g. smaller cluster size)
  - backing image contains lots of space with explicit zeroes
    (e.g. guest FS with 'shred' extensively used to delete files)

None of these probably occur that frequent.
But might be a next step and a separate series.

>> +    return is_unallocated(bs, m->offset + m->cow_start.offset,
>> +                          m->cow_start.nb_bytes) &&
>> +           is_unallocated(bs, m->offset + m->cow_end.offset,
>> +                          m->cow_end.nb_bytes);
>> +}
>> +
>> +static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    QCowL2Meta *m;
>> +
>> +    if (!(bs->file->bs->supported_zero_flags & BDRV_REQ_ALLOCATE)) {
>> +        return 0;
>> +    }
>> +
>> +    if (bs->encrypted) {
>> +        return 0;
>> +    }
>> +
>> +    for (m = l2meta; m != NULL; m = m->next) {
>> +        int ret;
>> +
>> +        if (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
>> +            continue;
>> +        }
>> +
>> +        if (!is_zero_cow(bs, m)) {
>> +            continue;
>> +        }
> 
> pre_write_overlap_check should be here
> 

Existing pre_write_overlap_check in qcow2_co_pwritev() should cover it,
since the check aligns the range to cluster boundaries.

However I think I missed a comment about it here. For suspicious
readers :) and just in case someone starts moving this code around.

I propose:

diff --git a/block/qcow2.c b/block/qcow2.c
index 027188a1a3..b3b3124083 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2088,6 +2088,9 @@ static int handle_alloc_space(BlockDriverState 
*bs, QCowL2Meta *l2meta)
              continue;
          }

+        /* Conventional place for qcow2_pre_write_overlap_check() but 
in this
+           case it is already done for these clusters */
+
          BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
          /* instead of writing zero COW buffers,
             efficiently zero out the whole clusters */


>> +
>> +        BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
>> +        /* instead of writing zero COW buffers,
>> +           efficiently zero out the whole clusters */
>> +        ret = bdrv_co_pwrite_zeroes(bs->file, m->alloc_offset,
>> +                                    m->nb_clusters * s->cluster_size,
>> +                                    BDRV_REQ_ALLOCATE);
>> +        if (ret < 0) {
>> +            if (ret != -ENOTSUP && ret != -EAGAIN) {
>> +                return ret;
>> +            }
>> +            continue;
>> +        }
>> +
>> +        trace_qcow2_skip_cow(qemu_coroutine_self(), m->offset, 
>> m->nb_clusters);
>> +        m->skip_cow = true;
>> +    }
>> +    return 0;
>> +}
>> +
> 
> 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]