Re: [Qemu-devel] [PATCH] qcow2: Release dirty entries with cache-clean-interval

[Kevin Wolf <kwolf@redhat.com>]

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