On 02.11.2022 12:19, Konstantin Khorenko wrote:
On 02.11.2022 03:32, Kui Liu wrote:
-----Original Message-----
From: Alexander Atanasov <alexander.atana...@virtuozzo.com>
Date: Tuesday, 1 November 2022 at 11:57 PM
To: Konstantin Khorenko <khore...@virtuozzo.com>, "Denis V. Lunev"
<d...@virtuozzo.com>, Devel <devel@openvz.org>
Cc: Kui Liu <kui....@acronis.com>, Alexey Kuznetsov <kuz...@acronis.com>
Subject: Re: [PATCH vz9 v2] ploop: port and fix the standby mode feature.
On 1.11.22 17:27, Konstantin Khorenko wrote:
> On 01.11.2022 08:36, Alexander Atanasov wrote:
>> On 31.10.22 21:27, Denis V. Lunev wrote:
>>> On 10/31/22 19:12, Alexander Atanasov wrote:
[snip]
>>>> @@ -406,6 +408,14 @@ static int ploop_ctr(struct dm_target *ti,
>>>> unsigned int argc, char **argv)
>>>> ti->private = ploop;
>>>> ploop->ti = ti;
>>>> + if (blk_queue_standby_en(ploop_blk_queue(ploop))) {
>>>> + blk_queue_flag_clear(QUEUE_FLAG_STANDBY,
>>>> ploop_blk_queue(ploop));
>>>> + if (static_branch_unlikely(&ploop_standby_check)) {
>>> this point should NOT be reached. If it is reached - SCST has violated
>>> the protocol at my opinion
>>
>> If any other device wants to user the flag and it sets it before _ctr
it
>> will catch it and enable the key. I can change it to a WARN_ON if _en
is
>> set but key is not.
>
> 1. i think the behavior should be the same both in vz7 and vz9.
> In vz7 we don't have a semantic "well, under certain circumstances
> if you set QUEUE_FLAG_STANDBY_EN queue bit, it will automatically enable
> the global static key", so let's not implement this logic in vz9 as
well.
the difference between ploop and dm-ploop is how the queue is created.
vz7 ploop creates the queue itself , so no one can pass the flag
vz9 receives an already created queue - so we must take care of the
flags, how to do so is still under question. But since the creation of
devices is different we can not keep it 1:1.
> 2. i do not think we need to clear QUEUE_FLAG_STANDBY bit in the request
> queue, because it should be zeroed on allocation:
>
> struct request_queue *blk_alloc_queue(int node_id)
> {
> ...
> q = kmem_cache_alloc_node(blk_requestq_cachep,
> GFP_KERNEL | __GFP_ZERO, node_id);
>
>
For both 1. and 2. - we do not know where the queue comes from - with
device mapper stacking devices one over other i think we might receive a
queue that is from another device and it can have the _en flag set.
Thinking about this
ploop0 over scst
ploop1 over ploop0
i have to verify if this is possible but i think it is.
If a queue gets moved it might not be freshly allocated and have the bit
set, so to be safe clear the flag - this is from the original patch and
i guess that is why it was there.
[LIU]: QUEUE_FLAG_STANDBY bit MUST be cleared here, as it is the only place
where
the ploop device recovers from standby mode. In our use case, once a ploop
device enters standby
mode, the userspace will initiate recovery by replacing the top delta file
without destroying
the device, which is why the bit is clear in 'repalce_delta' in vz7.
And in vz9, replace delta is achieved by table reload, which reallocates a new
ploop instance,
but keeps underlying mapped_device unchanged, thus request_queue unchanged,
so we have
to clear the bit here.
Sasha, Liu, thank you for the explanation, now the necessity to drop the
QUEUE_FLAG_STANDBY flag is
clear to me.
Let's do the following:
* if we found QUEUE_FLAG_STANDBY set, we
- clear the QUEUE_FLAG_STANDBY flag
- if the ploop_standby_check static key is FALSE,
- switch ploop_standby_check static key to TRUE
- print a WARN_ON "XXX"
- if the QUEUE_FLAG_STANDBY_ON is not set
- set the QUEUE_FLAG_STANDBY_ON flag
- print a WARN_ON "YYY"
* if we found QUEUE_FLAG_STANDBY unset
- if (the QUEUE_FLAG_STANDBY_ON is set &&
ploop_standby_check static key is FALSE)
- switch ploop_standby_check static key to TRUE
- print a WARN_ON "ZZZ"
JFYI: had a talk with Sasha, decided:
if we detect an inconsistency with QUEUE_FLAG_STANDBY{,_EN} bits / global
static key,
- issue a warning
- do not touch any bits / static key value
Rationale: in case we don't have SCST-based ploops over vStorage and
somehow we missed the fact that some other code used same bits QUEUE_FLAG_STANDBY{,_EN}, we will not
break anything.
Well, this is not a robust solution though - in this case SCST will rewrite QUEUE_FLAG_STANDBY{,_EN}
bits anyway, but
it looks like it does not matter much if we fix inconsistency or not - the
things will go wrong anyway.
Messages XXX/YYY/ZZZ should be different so we can fully understand the
situation upon a message.
If we receive a queue with the flag set and the static key not set
there are some options:
- enable the key and go on
- print a warning and clear the _en bit
- other ?
> 3. BTW, we talk here about standby mode for dm-ploop,
> and in vz9 we have dm-qcow2 as well (which is going to be default
BTW),
> so should we introduce the standby mode for it as well?
If it is required for dm-qcow2 too we should think how to implement it.
[LIU] I think it depends whether we are going to use dm-qcow2 instead of ploop
in SCST. I don’t see
why we need to move to dm-qcow2 unless ploop can and will be completely
replaced by dm-qcow2.
Well, in vz9 we'll have both dm-qcow2 and dm-ploop supported with default
dm-qcow2.
In vz10 we will probably drop dm-ploop, at least - we will try hard.
So please decide on your side when you are going to start supporting dm-qcow2
for iSCSI implementation.
Thank you.
--
Konstantin
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
