All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval
@ 2018-08-06 14:13 Alberto Garcia
  2018-08-06 15:05 ` Kevin Wolf
  0 siblings, 1 reply; 6+ messages in thread
From: Alberto Garcia @ 2018-08-06 14:13 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf

The cache-clean-interval option is used to periodically release unused
entries from the L2 and refcount caches. Dirty cache entries are left
untouched, even if they are otherwise valid candidates for removal.

This patch allows releasing those entries by flushing them to disk
first.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-cache.c | 21 +++++++++++++++------
 block/qcow2.c       |  4 ++--
 block/qcow2.h       |  2 +-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index d9dafa31e5..0812164e46 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -46,6 +46,8 @@ struct Qcow2Cache {
     uint64_t                cache_clean_lru_counter;
 };
 
+static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i);
+
 static inline void *qcow2_cache_get_table_addr(Qcow2Cache *c, int table)
 {
     return (uint8_t *) c->table_array + (size_t) table * c->table_size;
@@ -86,26 +88,33 @@ static void qcow2_cache_table_release(Qcow2Cache *c, int i, int num_tables)
 #endif
 }
 
-static inline bool can_clean_entry(Qcow2Cache *c, int i)
+static inline bool can_clean_entry(BlockDriverState *bs, Qcow2Cache *c, int i)
 {
     Qcow2CachedTable *t = &c->entries[i];
-    return t->ref == 0 && !t->dirty && t->offset != 0 &&
-        t->lru_counter <= c->cache_clean_lru_counter;
+    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) {
+        return false;
+    }
+
+    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
+        return false;
+    }
+
+    return true;
 }
 
-void qcow2_cache_clean_unused(Qcow2Cache *c)
+void qcow2_cache_clean_unused(BlockDriverState *bs, Qcow2Cache *c)
 {
     int i = 0;
     while (i < c->size) {
         int to_clean = 0;
 
         /* Skip the entries that we don't need to clean */
-        while (i < c->size && !can_clean_entry(c, i)) {
+        while (i < c->size && !can_clean_entry(bs, c, i)) {
             i++;
         }
 
         /* And count how many we can clean in a row */
-        while (i < c->size && can_clean_entry(c, i)) {
+        while (i < c->size && can_clean_entry(bs, c, i)) {
             c->entries[i].offset = 0;
             c->entries[i].lru_counter = 0;
             i++;
diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..f44ce85a5b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -732,8 +732,8 @@ static void cache_clean_timer_cb(void *opaque)
 {
     BlockDriverState *bs = opaque;
     BDRVQcow2State *s = bs->opaque;
-    qcow2_cache_clean_unused(s->l2_table_cache);
-    qcow2_cache_clean_unused(s->refcount_block_cache);
+    qcow2_cache_clean_unused(bs, s->l2_table_cache);
+    qcow2_cache_clean_unused(bs, s->refcount_block_cache);
     timer_mod(s->cache_clean_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
               (int64_t) s->cache_clean_interval * 1000);
 }
diff --git a/block/qcow2.h b/block/qcow2.h
index 81b844e936..e8b390ba4b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -659,7 +659,7 @@ int qcow2_cache_set_dependency(BlockDriverState *bs, Qcow2Cache *c,
     Qcow2Cache *dependency);
 void qcow2_cache_depends_on_flush(Qcow2Cache *c);
 
-void qcow2_cache_clean_unused(Qcow2Cache *c);
+void qcow2_cache_clean_unused(BlockDriverState *bs, Qcow2Cache *c);
 int qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c);
 
 int qcow2_cache_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval
  2018-08-06 14:13 [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval Alberto Garcia
@ 2018-08-06 15:05 ` Kevin Wolf
  2018-08-06 15:20   ` Alberto Garcia
  0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2018-08-06 15:05 UTC (permalink / raw)
  To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz

Am 06.08.2018 um 16:13 hat Alberto Garcia geschrieben:
> The cache-clean-interval option is used to periodically release unused
> entries from the L2 and refcount caches. Dirty cache entries are left
> untouched, even if they are otherwise valid candidates for removal.
> 
> This patch allows releasing those entries by flushing them to disk
> first.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cache.c | 21 +++++++++++++++------
>  block/qcow2.c       |  4 ++--
>  block/qcow2.h       |  2 +-
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index d9dafa31e5..0812164e46 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -46,6 +46,8 @@ struct Qcow2Cache {
>      uint64_t                cache_clean_lru_counter;
>  };
>  
> +static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i);
> +
>  static inline void *qcow2_cache_get_table_addr(Qcow2Cache *c, int table)
>  {
>      return (uint8_t *) c->table_array + (size_t) table * c->table_size;
> @@ -86,26 +88,33 @@ static void qcow2_cache_table_release(Qcow2Cache *c, int i, int num_tables)
>  #endif
>  }
>  
> -static inline bool can_clean_entry(Qcow2Cache *c, int i)
> +static inline bool can_clean_entry(BlockDriverState *bs, Qcow2Cache *c, int i)
>  {
>      Qcow2CachedTable *t = &c->entries[i];
> -    return t->ref == 0 && !t->dirty && t->offset != 0 &&
> -        t->lru_counter <= c->cache_clean_lru_counter;
> +    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) {
> +        return false;
> +    }
> +
> +    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
> +        return false;
> +    }

We're not in coroutine context here, so qcow2_cache_entry_flush() will
be blocking. I don't think that's acceptable in a timer callback.

On the other hand, if we made it non-blocking by moving it into a
coroutine that could yield, we would have to consider races with other
parts of the code and at least take s->lock and implement
.bdrv_co_drain_begin/end callbacks.

Kevin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval
  2018-08-06 15:05 ` Kevin Wolf
@ 2018-08-06 15:20   ` Alberto Garcia
  2018-08-06 15:58     ` Kevin Wolf
  0 siblings, 1 reply; 6+ messages in thread
From: Alberto Garcia @ 2018-08-06 15:20 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz

On Mon 06 Aug 2018 05:05:41 PM CEST, Kevin Wolf wrote:
> Am 06.08.2018 um 16:13 hat Alberto Garcia geschrieben:
>> -static inline bool can_clean_entry(Qcow2Cache *c, int i)
>> +static inline bool can_clean_entry(BlockDriverState *bs, Qcow2Cache *c, int i)
>>  {
>>      Qcow2CachedTable *t = &c->entries[i];
>> -    return t->ref == 0 && !t->dirty && t->offset != 0 &&
>> -        t->lru_counter <= c->cache_clean_lru_counter;
>> +    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) {
>> +        return false;
>> +    }
>> +
>> +    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
>> +        return false;
>> +    }
>
> We're not in coroutine context here, so qcow2_cache_entry_flush() will
> be blocking. I don't think that's acceptable in a timer callback.
>
> On the other hand, if we made it non-blocking by moving it into a
> coroutine that could yield, we would have to consider races with other
> parts of the code and at least take s->lock and implement
> .bdrv_co_drain_begin/end callbacks.

Oh, I see... it's probably not worth complicating the code for this then.

Berto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval
  2018-08-06 15:20   ` Alberto Garcia
@ 2018-08-06 15:58     ` Kevin Wolf
  2018-08-09 13:09       ` Alberto Garcia
  0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2018-08-06 15:58 UTC (permalink / raw)
  To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz

Am 06.08.2018 um 17:20 hat Alberto Garcia geschrieben:
> On Mon 06 Aug 2018 05:05:41 PM CEST, Kevin Wolf wrote:
> > Am 06.08.2018 um 16:13 hat Alberto Garcia geschrieben:
> >> -static inline bool can_clean_entry(Qcow2Cache *c, int i)
> >> +static inline bool can_clean_entry(BlockDriverState *bs, Qcow2Cache *c, int i)
> >>  {
> >>      Qcow2CachedTable *t = &c->entries[i];
> >> -    return t->ref == 0 && !t->dirty && t->offset != 0 &&
> >> -        t->lru_counter <= c->cache_clean_lru_counter;
> >> +    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) {
> >> +        return false;
> >> +    }
> >> +
> >> +    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
> >> +        return false;
> >> +    }
> >
> > We're not in coroutine context here, so qcow2_cache_entry_flush() will
> > be blocking. I don't think that's acceptable in a timer callback.
> >
> > On the other hand, if we made it non-blocking by moving it into a
> > coroutine that could yield, we would have to consider races with other
> > parts of the code and at least take s->lock and implement
> > .bdrv_co_drain_begin/end callbacks.
> 
> Oh, I see... it's probably not worth complicating the code for this then.

On second thoughts, I actually think we can do without the drain
callbacks if we just add a bdrv_inc/dec_in_flight() pair around
qcow2_cache_clean_unused().

We'd still have to create a coroutine in cache_clean_timer_cb() and take
the lock, but that sounds more managable.

Kevin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval
  2018-08-06 15:58     ` Kevin Wolf
@ 2018-08-09 13:09       ` Alberto Garcia
  2018-08-09 13:30         ` Kevin Wolf
  0 siblings, 1 reply; 6+ messages in thread
From: Alberto Garcia @ 2018-08-09 13:09 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz

On Mon 06 Aug 2018 05:58:41 PM CEST, Kevin Wolf wrote:
> Am 06.08.2018 um 17:20 hat Alberto Garcia geschrieben:
>> On Mon 06 Aug 2018 05:05:41 PM CEST, Kevin Wolf wrote:
>> > Am 06.08.2018 um 16:13 hat Alberto Garcia geschrieben:
>> >> -static inline bool can_clean_entry(Qcow2Cache *c, int i)
>> >> +static inline bool can_clean_entry(BlockDriverState *bs, Qcow2Cache *c, int i)
>> >>  {
>> >>      Qcow2CachedTable *t = &c->entries[i];
>> >> -    return t->ref == 0 && !t->dirty && t->offset != 0 &&
>> >> -        t->lru_counter <= c->cache_clean_lru_counter;
>> >> +    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) {
>> >> +        return false;
>> >> +    }
>> >> +
>> >> +    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
>> >> +        return false;
>> >> +    }
>> >
>> > We're not in coroutine context here, so qcow2_cache_entry_flush() will
>> > be blocking. I don't think that's acceptable in a timer callback.
>> >
>> > On the other hand, if we made it non-blocking by moving it into a
>> > coroutine that could yield, we would have to consider races with other
>> > parts of the code and at least take s->lock and implement
>> > .bdrv_co_drain_begin/end callbacks.
>> 
>> Oh, I see... it's probably not worth complicating the code for this then.
>
> On second thoughts, I actually think we can do without the drain
> callbacks if we just add a bdrv_inc/dec_in_flight() pair around
> qcow2_cache_clean_unused().
>
> We'd still have to create a coroutine in cache_clean_timer_cb() and take
> the lock, but that sounds more managable.

When we're waiting for a cache entry to flush and the coroutine yields,
can the previous (already checked) cache entries become dirty?

Berto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval
  2018-08-09 13:09       ` Alberto Garcia
@ 2018-08-09 13:30         ` Kevin Wolf
  0 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2018-08-09 13:30 UTC (permalink / raw)
  To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz

Am 09.08.2018 um 15:09 hat Alberto Garcia geschrieben:
> On Mon 06 Aug 2018 05:58:41 PM CEST, Kevin Wolf wrote:
> > Am 06.08.2018 um 17:20 hat Alberto Garcia geschrieben:
> >> On Mon 06 Aug 2018 05:05:41 PM CEST, Kevin Wolf wrote:
> >> > Am 06.08.2018 um 16:13 hat Alberto Garcia geschrieben:
> >> >> -static inline bool can_clean_entry(Qcow2Cache *c, int i)
> >> >> +static inline bool can_clean_entry(BlockDriverState *bs, Qcow2Cache *c, int i)
> >> >>  {
> >> >>      Qcow2CachedTable *t = &c->entries[i];
> >> >> -    return t->ref == 0 && !t->dirty && t->offset != 0 &&
> >> >> -        t->lru_counter <= c->cache_clean_lru_counter;
> >> >> +    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) {
> >> >> +        return false;
> >> >> +    }
> >> >> +
> >> >> +    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
> >> >> +        return false;
> >> >> +    }
> >> >
> >> > We're not in coroutine context here, so qcow2_cache_entry_flush() will
> >> > be blocking. I don't think that's acceptable in a timer callback.
> >> >
> >> > On the other hand, if we made it non-blocking by moving it into a
> >> > coroutine that could yield, we would have to consider races with other
> >> > parts of the code and at least take s->lock and implement
> >> > .bdrv_co_drain_begin/end callbacks.
> >> 
> >> Oh, I see... it's probably not worth complicating the code for this then.
> >
> > On second thoughts, I actually think we can do without the drain
> > callbacks if we just add a bdrv_inc/dec_in_flight() pair around
> > qcow2_cache_clean_unused().
> >
> > We'd still have to create a coroutine in cache_clean_timer_cb() and take
> > the lock, but that sounds more managable.
> 
> When we're waiting for a cache entry to flush and the coroutine yields,
> can the previous (already checked) cache entries become dirty?

qcow2_cache_entry_mark_dirty() should only ever be called under s->lock,
so as long as you take the lock, I don't think so.

Kevin

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-08-09 13:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-06 14:13 [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval Alberto Garcia
2018-08-06 15:05 ` Kevin Wolf
2018-08-06 15:20   ` Alberto Garcia
2018-08-06 15:58     ` Kevin Wolf
2018-08-09 13:09       ` Alberto Garcia
2018-08-09 13:30         ` Kevin Wolf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.