Re: [SPDK] Question on LVS and hotplug

[Walker, Benjamin <benjamin.walker at intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

On Thu, 2018-09-13 at 00:44 +0000, Sablok, Kunal wrote:
> Hi,
> I have few questions on LVS.
> 
> 
> *        Consider LVS/LVOL is created over nvme_bdev device. When nvme_bdev is
> removed (hotplug), the notification goes to LVS where it calls its hotplug
> remove_cb function. As part of that function why LVS issues some read IOs?
> Since the drive getting removed (or physically removed) it can never serve
> IOs.

I just looked at the code and from what I see you're right - it tries to do a
clean shutdown of the logical volume store which flushes the metadata. That
clearly won't work, since the disk is gone.

> 
> *        Also if the IO (fired by LVS down in context of base bdev removal)
> fails, LVS never gets unloaded. Why LVS never gets unloaded in this case?
> 
> If I do some change to fail new IOs when nvme_bdev is removed from nvme_bdev
> layer, then I get issue where LVS never gets unloaded as IO has failed now (I
> am wondering when actual physical device gets removed then also IO could be
> failing).
> If I do further changes in LVS it started working fine with below changes:
> 
> In _vbdev_lvs_remove_cb()
>    if (lvserrno != 0) {
>                 SPDK_INFOLOG(SPDK_LOG_VBDEV_LVOL, "Could not remove lvol store
> bdev\n");
>                TAILQ_REMOVE(&g_spdk_lvol_pairs, lvs_bdev, lvol_stores);
>                 free(lvs_bdev);
> 
>         } else {
>                 TAILQ_REMOVE(&g_spdk_lvol_pairs, lvs_bdev, lvol_stores);
>                 free(lvs_bdev);
>         }

I agree - this function needs to change to always remove the lvs_bdev from the
list and release the memory.

> 
> 
> In _spdk_bs_load_ctx_fail(), change is do "_spdk_bs_free(ctx->bs);" everytime
> and comment out "if (ctx->is_load)"

This part is a bit trickier. There are two scenarios where an unload could fail.
One is that the disk is gone and flushing the metadata will never be possible.
The other is that the unload just happened to fail this one time due to a
transient error (out of memory probably) and should be attempted again. I think
we need to differentiate between the two in order to make this correct. There
are also a number of places where a failure isn't handled at all along the
unload path (which doesn't corrupt the blobstore, but does leak memory).

> 
> Could anybody please comment on this?
> 
> Regards,
> Kunal
> 
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk