| Message ID | 20180806141322.14427-1-berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: Release dirty entries with cache-clean-interval | expand |
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
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
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
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
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
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,
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(-)