Re: What prevents discarding a cluster during rewrite?

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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



> 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. Separate 
CoRwLock looks more feasible.

--
Best regards,
Vladimir