Re: [Qemu-devel] [PATCH v2 2/3] block/fleecing-filter: new filter driver for fleecing

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

03.07.2018 14:15, Kevin Wolf wrote:
> Am 02.07.2018 um 14:09 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.06.2018 20:40, Eric Blake wrote:
>>> On 06/29/2018 12:30 PM, John Snow wrote:
>>>>
>>>> On 06/29/2018 11:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> We need to synchronize backup job with reading from fleecing image
>>>>> like it was done in block/replication.c.
>>>>>
>>>>> Otherwise, the following situation is theoretically possible:
>>>>>
>>>>> 1. client start reading
>>>>> 2. client understand, that there is no corresponding cluster in
>>>>>      fleecing image
>>>> I don't think the client refocuses the read, but QEMU does. (the client
>>>> has no idea if it is reading from the fleecing node or the backing
>>>> file.)
>>>>
>>>> ... but understood:
>>>>
>>>>> 3. client is going to read from backing file (i.e. active image)
>>>>> 4. guest writes to active image
>>>> My question here is if QEMU will allow reads and writes to interleave in
>>>> general. If the client is reading from the backing file, the active
>>>> image, will QEMU allow a write to modify that data before we're done
>>>> getting that data?
>>>>
>>>>> 5. this write is stopped by backup(sync=none) and cluster is copied to
>>>>>      fleecing image
>>>>> 6. guest write continues...
>>>>> 7. and client reads _new_ (or partly new) date from active image
>>>>>
>>>> I can't answer this for myself one way or the other right now, but I
>>>> imagine you observed a failure in practice in your test setups, which
>>>> motivated this patch?
>>>>
>>>> A test or some proof would help justification for this patch. It would
>>>> also help prove that it solves what it claims to!
>>> In fact, do we really need a new filter, or do we just need to make the
>>> "sync":"none" blockdev-backup job take the appropriate synchronization
>>> locks?
>>>
>> How? We'll need additional mechanism like serializing requests.. Or a way to
>> reuse serializing requests. Using backup internal synchronization looks
>> simpler, and it is already used in block replication.
> But it also just an ugly hack

agree.

>   that fixes one special case and leaves
> everything else broken. replication is usually not a good example for
> anything. It always gives me bad surprises when I have to look at it.
>
> We'll have to figure out where to fix this problem (or what it really
> is, once you look more than just at fleecing), but I think requiring the
> user to add a filter driver to work around missing serialisation in
> other code, and corrupting their image if they forget to, is not a
> reasonable solution.
>
> I see at least two things wrong in this context:
>
> * The permissions don't seem to match reality. The NBD server
>    unconditionally shares PERM_WRITE, which is wrong in this case. The
>    client wants to see a point-in-time snapshot that never changes. This
>    should become an option so that it can be properly reflected in the
>    permissions used.
>
> * Once we have proper permissions, the fleecing setup breaks down
>    because the guest needs PERM_WRITE on the backing file, but the
>    fleecing overlay allows that only if the NBD client allows it (which
>    it doesn't for fleecing).
>
>    Now we can implement an exception right into backup that installs a
>    backup filter driver between source and target if the source is the
>    backing file of the target. The filter driver would be similar to the
>    commit filter driver in that it simply promises !PERM_WRITE to its
>    parents, but allows PERM_WRITE on the source because it has installed
>    the before_write_notifier that guarantees this condition.
>
>    All writes to the target that are made by the backup job in this setup
>    (including before_write_notifier writes) need to be marked as
>    serialising so that any concurrent reads are completed first.
>
> And if we decide to add a target filter to backup, we should probably at
> the same time use a filter driver for intercepting source writes instead
> of using before_write_notifier.

and this is the point, where we can drop backup job at all from fleecing 
scheme, as actually backup(sync=none) == such special filter driver

>
> Max, I think you intended to make both source and target children of the
> same block job node (or at least for mirror). But wouldn't that create
> loops in a setup like this? I think we may need two filters that are
> only connected through the block job, but not with block graph edges.
>
> Kevin

-- 
Best regards,
Vladimir