[Devel] [PATCH vz9 v2] ploop: port and fix the standby mode feature.

[Konstantin Khorenko]

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]

>>> @@ -1163,6 +1166,26 @@ static void ploop_queue_resubmit(struct pio *pio)
>>>        queue_work(ploop->wq, &ploop->worker);
>>>    }
>>> +static void ploop_check_standby_mode(struct ploop *ploop, long res)
>>> +{
>>> +    struct request_queue *q = ploop_blk_queue(ploop);
>>> +    int prev;
>>> +
>>> +    if (!blk_queue_standby_en(q))
>>> +        return;
>>> +
>>> +    /* move to standby if delta lease was stolen or mount is gone */
>>> +    if (res != -EBUSY && res != -ENOTCONN && res != -EIO)
>>> +        return;
>>> +
>>> +    spin_lock_irq(&q->queue_lock);
>>> +    prev = blk_queue_flag_test_and_set(QUEUE_FLAG_STANDBY, q);
>>> +    spin_unlock_irq(&q->queue_lock);
>>> +
>>> +    if (!prev)
>>> +        pr_info("ploop: switch into standby mode\n");

i think we should have same messages both in vz7 and vz9.

In vz7 it is:
         if (!prev)
                 printk("ploop%d was switched into "
                        "the standby mode\n", plo->index);

Well, i understand that ploop id part of the message has changed - it's ok,
but i suggest to keep the rest of the message the same.

Or if we want to change them - to change them in both versions synchronously.

>>
>> some ploop identifier is needed here to determine device name
> 
> I'll add the device name here. dm-ploop logs without device name
> everywhere - i'll check that too.
> 

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

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


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?