Re: What prevents discarding a cluster during rewrite?

[Kevin Wolf]

Am 23.02.2021 um 16:23 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 
> 
> On 23.02.2021 13:35, Kevin Wolf wrote:
> > Am 22.02.2021 um 22:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 23.02.2021 00:30, Vladimir Sementsov-Ogievskiy wrote:
> > > > Thinking of how to prevent dereferencing to zero (and discard) of
> > > > host cluster during flush of compressed cache (which I'm working on
> > > > now), I have a question.. What prevents it for normal writes?
> > > 
> > > I have no idea about why didn't it fail for years.. May be, I'm
> > > missing something?
> > 
> > Ouch. I suppose the reason why we never ran into it is that afaik Linux
> > drains the queue before sending discard requests.
> > 
> > Of course, we still need to protect against this in QEMU. We can't just
> > unref a cluster that is still in use.
> > 
> > > I have idea of fixing: increase the refcount of host cluster before
> > > write to data_file (it's important to increase refacount in same
> > > s->lock critical section where we get host_offset) and dereference it
> > > after write.. It should help. Any thoughts?
> > 
> > It would cause metadata updates for rewrites. I don't know whether the
> > intermediate value would ever be written to disk, but at least we'd
> > rewrite the same refcounts as before. I don't think we want that.
> 
> Hmm, if that can provoke extra refcount cache flush that's bad..
> 
> May be we need something like of additional "dynamic" refcounts, so
> that total_refcount = normal_refcount + dynamic_refcount.. And for
> in-flight clusters dynamic_refcount is > 0. We can store dynamic
> refcounts in GHashTable(cluster -> refcount).

Do you think it's worth the complexity? The qcow2 driver is already
relatively complicated today.

> > Discard requests might be rare enough that not considering the cluster
> > at all could work. We could then take a reader CoRwlock during most
> > operations, and a writer lock for discard.
> > 
> > Actually, maybe s->lock should be this CoRwlock, and instead of dropping
> > it temporarily like we do now we would just upgrade and downgrade it as
> > needed. Maybe this would allow finer grained locking in other places,
> > too.
> 
> In this case many operations will be blocked during data writing, like
> allocation of another cluster.. That doesn't seem good.

You're right, that would be too restrictive.

> Separate CoRwLock looks more feasible.

Maybe that then.

Kevin