qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [PATCH v2 2/4] qcow2: add qcow2_cache_discard


From: Pavel Butsykin
Subject: Re: [Qemu-block] [PATCH v2 2/4] qcow2: add qcow2_cache_discard
Date: Thu, 22 Jun 2017 16:55:23 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1

On 22.06.2017 01:29, Max Reitz wrote:
On 2017-06-13 14:16, Pavel Butsykin wrote:
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 thecache with the data in the file.

Signed-off-by: Pavel Butsykin <address@hidden>
---
  block/qcow2-cache.c    | 21 +++++++++++++++++++++
  block/qcow2-refcount.c |  5 +++++
  block/qcow2.h          |  1 +
  3 files changed, 27 insertions(+)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 1d25147392..7931edf237 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -411,3 +411,24 @@ void qcow2_cache_entry_mark_dirty(BlockDriverState *bs, 
Qcow2Cache *c,
      assert(c->entries[i].offset != 0);
      c->entries[i].dirty = true;
  }
+
+void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset)
+{
+    int i;
+
+    for (i = 0; i < c->size; i++) {
+        if (c->entries[i].offset == offset) {
+            goto found; /* table offset */
+        }
+    }
+    return;
+
+found:
+    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 7c06061aae..576ab551d6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -767,6 +767,11 @@ 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]) {
+            qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);

I don't like this very much. It works, but it feels bad.

Would it be possible to store this refblock's offset somewhere and only
put it back if @offset is equal to that?

+            qcow2_cache_discard(bs, s->refcount_block_cache, offset);
+
+            qcow2_cache_discard(bs, s->l2_table_cache, offset);
+

So you're blindly calling qcow2_cache_discard() on @offset because
@offset may be pointing to a refblock or an L2 table? Right, that works,
but it still deserves a comment, I think (that we have no idea whether
@offset actually points to any of these refcount structures, but that we
also just don't have to care).

Looks OK to me, functionally, but I'd at least like to have that comment.


Hmm.. We can split qcow2_cache_discard() and kill two birds with one
stone.

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);
}

....

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);
    }
...

Max

              update_refcount_discard(bs, cluster_offset, s->cluster_size);
          }
      }
diff --git a/block/qcow2.h b/block/qcow2.h
index 1801dc30dc..07faa6dc78 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -597,5 +597,6 @@ 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_discard(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset);
#endif






reply via email to

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