[Devel] [PATCH RH9] dm-ploop: port the standby mode feature.

[Alexander Atanasov]

On 13.10.22 16:33, Kui Liu wrote:
> Hello,
> 
> First of all, please bear in mind that this patch is a port of a patchset currently present in VZ 7 kernel.
> Original patches were added just to address a problem that would only happen in our particular use case,
> where we need to use ploop devices as the back store for  iSCSI target, while ploop devices are backed
> by files stored in vstorage filesystem.


Ok, i don't quite get this yet. Since it is specific to vstorage can we 
check the queue on init and see if it is backed by that iSCSI target and 
set a static key or flag to enable the checks only if that is the case? 
check can be done via queue->disk->major/minor/diskname/fops/pick-one 
from a quick look.

[snip]

>      >> +    /* 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");
>      >> +}
> 
> 
>      What guarantees that we got EBUSY, EIO or ENOTCONN for the reasons
>      listed in the description - "This mode shows that a delta lease was
>      stolen and it is impossible to handle any requests." - what if we got
>      such an error for other reason? If the problem comes from nfs why not
>      suspend device mapper from there ?
> 
> [LIU]: I would assume vstorage filesystem guarantees that only EBUSY, EIO, or ENOTCONN
> will be returned when the described event happens, and it doesn't matter whether we could
> get these errors for other reasons.  And what matters is, in our use case, once the 3 errors are
> returned, the ploop device need to be flagged as 'standby mode' which needs to be passed to
>   iSCSI target driver.  As for what happens when used with other filesystems, we actually don't care,
>   however you can see that the changes don't affect ploop's normal behaviour either. >
>      Also do this errors actually reach dm-ploop ? From my recent tests i
>      observed that if you have and EIO the filesystem gets it and remounts RO
>      - it might not reach ploop code at all. Even if you suspend/resume the
>      queue it would not help with the RO fs.
> 
> [LIU]:  When used with vstorage filesystem, these errors do reach dm-ploop.  Again, we really
> don’t care about other filesystems as long as it doesn't break anything.

If there is no check for the specific target and some of the errors 
comes from other device - ploop will say it is into standby but no 
device will check the flags - so it will not be true. My point is that 
it is hard to say it won't break anything.

[snip]

>      >>        INIT_WORK(&ploop->event_work, do_ploop_event_work);
>      >>        init_completion(&ploop->inflight_bios_ref_comp);
>      >>    +    ploop->queue = ti->table->md->queue;
>      >
>      > This looks wrong, ploop struct alread have a ptr to ti.
>      > Tables are reloaded in some cases and it is possible that table/md/queue
>      > change.
>      >
>      >> +    blk_queue_flag_clear(QUEUE_FLAG_STANDBY, ploop->queue);
>      >> +
>      >>        for (i = 0; i < 2; i++) {
>      >>            release = i ? ploop_inflight_bios_ref_exit1 :
>      >>                      ploop_inflight_bios_ref_exit0;
>      >> diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
>      >> index 5d953278a976..2502718c131f 100644
>      >> --- a/drivers/md/dm-ploop.h
>      >> +++ b/drivers/md/dm-ploop.h
>      >> @@ -229,6 +229,8 @@ struct ploop {
>      >>         struct timer_list enospc_timer;
>      >>        bool event_enospc;
>      >> +
>      >> +    struct request_queue *queue;
>      >>    };

There is ploop->ti->table->md->queue, why not use it but cache queue ptr 
here ? Is it guaranteed that queue won't change and leave ploop with 
dangling ptr?

>      >>     struct ploop_rq {
[snip]

>      >> +#define blk_queue_standby(q)    test_bit(QUEUE_FLAG_STANDBY,
>      >> &(q)->queue_flags)
>      >>     extern void blk_set_pm_only(struct request_queue *q);
>      >>    extern void blk_clear_pm_only(struct request_queue *q);
> 
> 
>      Why introduce a new queue flag?
>      Why can't device mapper suspend be used ?
>      Why not use the existing ploop code to defer and retry pios?
> 
> [LIU]:  The original commit does give some explanation on why  introduce the new queue flag, because we need to
> transit to another state, aka standby, once the 3 errors are returned, then the state needs to be aware by iSCSI target
> driver.  while we cannot use the bio error code to pass the event to iSCSI target driver (adding new error code is not an
> option here  because it would break other ploop users) . Since the only data struct that are shared between dm-ploop
> and iSCSI target driver is 'struct request_queue', therefore use a new flag is the most convenient way, this is my understanding.
> 
> We can't use device mapper suspend flag because it is not aware by iSCSI target driver.

If i understand correct - no way to get from iSCSI back to device 
mapper. So it makes sense to use the shared queue.

> Defer and retry pios will be unnecessary once these errors is returned in our used case.


-- 
Regards,
Alexander Atanasov