diff mbox

[v5,2/4] qcow2: add qcow2_cache_discard

Message ID 20170712114700.20008-3-pbutsykin@virtuozzo.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Butsykin July 12, 2017, 11:46 a.m. UTC 
Whenever l2/refcount table clusters are discarded from the file we can
automatically drop unnecessary content of the cache tables. This reduces
the chance of eviction useful cache data and eliminates inconsistent data
in the cache with the data in the file.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cache.c    | 26 ++++++++++++++++++++++++++
 block/qcow2-refcount.c | 14 ++++++++++++++
 block/qcow2.h          |  3 +++
 3 files changed, 43 insertions(+)

Comments

Kevin Wolf July 12, 2017, 2:45 p.m. UTC | #1 
Am 12.07.2017 um 13:46 hat Pavel Butsykin geschrieben:
> Whenever l2/refcount table clusters are discarded from the file we can
> automatically drop unnecessary content of the cache tables. This reduces
> the chance of eviction useful cache data and eliminates inconsistent data
> in the cache with the data in the file.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cache.c    | 26 ++++++++++++++++++++++++++
>  block/qcow2-refcount.c | 14 ++++++++++++++
>  block/qcow2.h          |  3 +++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 1d25147392..75746a7f43 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -411,3 +411,29 @@ void qcow2_cache_entry_mark_dirty(BlockDriverState *bs, Qcow2Cache *c,
>      assert(c->entries[i].offset != 0);
>      c->entries[i].dirty = true;
>  }
> +
> +void *qcow2_cache_is_table_offset(BlockDriverState *bs, Qcow2Cache *c,
> +                                  uint64_t offset)
> +{
> +    int i;
> +
> +    for (i = 0; i < c->size; i++) {
> +        if (c->entries[i].offset == offset) {
> +            return qcow2_cache_get_table_addr(bs, c, i);
> +        }
> +    }
> +    return NULL;
> +}
> +
> +void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table)
> +{
> +    int i = qcow2_cache_get_table_idx(bs, c, table);
> +
> +    assert(c->entries[i].ref == 0);
> +
> +    c->entries[i].offset = 0;
> +    c->entries[i].lru_counter = 0;
> +    c->entries[i].dirty = false;
> +
> +    qcow2_cache_table_release(bs, c, i, 1);
> +}
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index c9b0dcb4f3..8050db4544 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -862,6 +862,20 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>          s->set_refcount(refcount_block, block_index, refcount);
>  
>          if (refcount == 0 && s->discard_passthrough[type]) {
> +            void *table;
> +
> +            table = qcow2_cache_is_table_offset(bs, s->refcount_block_cache,
> +                                                offset);
> +            if (table != NULL) {
> +                qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
> +                qcow2_cache_discard(bs, s->refcount_block_cache, table);
> +            }
> +
> +            table = qcow2_cache_is_table_offset(bs, s->l2_table_cache, offset);
> +            if (table != NULL) {
> +                qcow2_cache_discard(bs, s->l2_table_cache, table);
> +            }
> +
>              update_refcount_discard(bs, cluster_offset, s->cluster_size);
>          }
>      }

This is not wrong, but wouldn't actually even refcount == 0 be enough as
a condition to evict the cluster from the cache? I don't think it really
matters whether we actually send a discard request or not for the image
file.

Kevin
Pavel Butsykin July 13, 2017, 5:18 p.m. UTC | #2 
On 12.07.2017 17:45, Kevin Wolf wrote:
> Am 12.07.2017 um 13:46 hat Pavel Butsykin geschrieben:
>> Whenever l2/refcount table clusters are discarded from the file we can
>> automatically drop unnecessary content of the cache tables. This reduces
>> the chance of eviction useful cache data and eliminates inconsistent data
>> in the cache with the data in the file.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-cache.c    | 26 ++++++++++++++++++++++++++
>>   block/qcow2-refcount.c | 14 ++++++++++++++
>>   block/qcow2.h          |  3 +++
>>   3 files changed, 43 insertions(+)
>>
>> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
>> index 1d25147392..75746a7f43 100644
>> --- a/block/qcow2-cache.c
>> +++ b/block/qcow2-cache.c
>> @@ -411,3 +411,29 @@ void qcow2_cache_entry_mark_dirty(BlockDriverState *bs, Qcow2Cache *c,
>>       assert(c->entries[i].offset != 0);
>>       c->entries[i].dirty = true;
>>   }
>> +
>> +void *qcow2_cache_is_table_offset(BlockDriverState *bs, Qcow2Cache *c,
>> +                                  uint64_t offset)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < c->size; i++) {
>> +        if (c->entries[i].offset == offset) {
>> +            return qcow2_cache_get_table_addr(bs, c, i);
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table)
>> +{
>> +    int i = qcow2_cache_get_table_idx(bs, c, table);
>> +
>> +    assert(c->entries[i].ref == 0);
>> +
>> +    c->entries[i].offset = 0;
>> +    c->entries[i].lru_counter = 0;
>> +    c->entries[i].dirty = false;
>> +
>> +    qcow2_cache_table_release(bs, c, i, 1);
>> +}
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index c9b0dcb4f3..8050db4544 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -862,6 +862,20 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>>           s->set_refcount(refcount_block, block_index, refcount);
>>   
>>           if (refcount == 0 && s->discard_passthrough[type]) {
>> +            void *table;
>> +
>> +            table = qcow2_cache_is_table_offset(bs, s->refcount_block_cache,
>> +                                                offset);
>> +            if (table != NULL) {
>> +                qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> +                qcow2_cache_discard(bs, s->refcount_block_cache, table);
>> +            }
>> +
>> +            table = qcow2_cache_is_table_offset(bs, s->l2_table_cache, offset);
>> +            if (table != NULL) {
>> +                qcow2_cache_discard(bs, s->l2_table_cache, table);
>> +            }
>> +
>>               update_refcount_discard(bs, cluster_offset, s->cluster_size);
>>           }
>>       }
> 
> This is not wrong, but wouldn't actually even refcount == 0 be enough as
> a condition to evict the cluster from the cache? I don't think it really
> matters whether we actually send a discard request or not for the image
> file.

Yes, you're right. Will fix.

> Kevin
>
diff mbox

Patch

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 1d25147392..75746a7f43 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -411,3 +411,29 @@  void qcow2_cache_entry_mark_dirty(BlockDriverState *bs, Qcow2Cache *c,
     assert(c->entries[i].offset != 0);
     c->entries[i].dirty = true;
 }
+
+void *qcow2_cache_is_table_offset(BlockDriverState *bs, Qcow2Cache *c,
+                                  uint64_t offset)
+{
+    int i;
+
+    for (i = 0; i < c->size; i++) {
+        if (c->entries[i].offset == offset) {
+            return qcow2_cache_get_table_addr(bs, c, i);
+        }
+    }
+    return NULL;
+}
+
+void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table)
+{
+    int i = qcow2_cache_get_table_idx(bs, c, table);
+
+    assert(c->entries[i].ref == 0);
+
+    c->entries[i].offset = 0;
+    c->entries[i].lru_counter = 0;
+    c->entries[i].dirty = false;
+
+    qcow2_cache_table_release(bs, c, i, 1);
+}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c9b0dcb4f3..8050db4544 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -862,6 +862,20 @@  static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
         s->set_refcount(refcount_block, block_index, refcount);
 
         if (refcount == 0 && s->discard_passthrough[type]) {
+            void *table;
+
+            table = qcow2_cache_is_table_offset(bs, s->refcount_block_cache,
+                                                offset);
+            if (table != NULL) {
+                qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
+                qcow2_cache_discard(bs, s->refcount_block_cache, table);
+            }
+
+            table = qcow2_cache_is_table_offset(bs, s->l2_table_cache, offset);
+            if (table != NULL) {
+                qcow2_cache_discard(bs, s->l2_table_cache, table);
+            }
+
             update_refcount_discard(bs, cluster_offset, s->cluster_size);
         }
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index 96a8d43c17..52c374e9ed 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -649,6 +649,9 @@  int qcow2_cache_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
 int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
     void **table);
 void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
+void *qcow2_cache_is_table_offset(BlockDriverState *bs, Qcow2Cache *c,
+                                  uint64_t offset);
+void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table);
 
 /* qcow2-bitmap.c functions */
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,