Re: [SPDK] spdk_bdev_close() threading

[Stojaczyk, Dariusz <dariusz.stojaczyk at intel.com>]

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



> -----Original Message-----
> From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
> Sent: Thursday, March 28, 2019 3:42 PM
> To: Storage Performance Development Kit <spdk(a)lists.01.org>
> Subject: Re: [SPDK] spdk_bdev_close() threading
> 
> 
> 
> On 3/28/19, 3:20 AM, "SPDK on behalf of wuzhouhui" <spdk-
> bounces(a)lists.01.org on behalf of wuzhouhui14(a)mails.ucas.ac.cn> wrote:
> 
> 
>     > -----Original Messages-----
>     > From: "Stojaczyk, Dariusz" <dariusz.stojaczyk(a)intel.com>
>     > Sent Time: 2019-03-28 17:13:16 (Thursday)
>     > To: "Storage Performance Development Kit" <spdk(a)lists.01.org>
>     > Cc:
>     > Subject: [SPDK] spdk_bdev_close() threading
>     >
>     > I was surprised to see that bdev descriptors can be closed only from the
> same thread that opened them. Vhost doesn't respect this rule. As
> expected, I was able to trigger assert(desc->thread == spdk_get_thread())
> while closing a vhost scsi descriptor using the latest SPDK master. This could
> be probably fixed by always scheduling the spdk_bdev_close() to proper
> thread. Maybe vhost could even immediately assume the descriptor is closed
> and set its `desc` pointer to NULL without waiting for spdk_bdev_close() to
> be actually called. But why the descriptor needs to be closed from a specific
> thread in the first place? Would it be possible for spdk_bdev_close() to
> internally schedule itself on desc->thread?
>     >
>     Darek:
> 
>     The purpose spdk_bdev_close() running on the same thread with
> spdk_bdev_open() is a preparation of patch
> https://review.gerrithub.io/c/spdk/spdk/+/423369.
> 
>     Let spdk_bdev_close() to internally schedule itself on desc->thread looks
> good at the first sight for me.
> 
> I agree we can just have it internally schedule itself.  I think we'll still need to
> set desc->closed = true in the caller's thread through before we schedule it -
> otherwise we'll have a race with the _remove_notify path.

Okay, cool. Do we have any unit tests for that?

D.

> 
> -Jim
> 
> 
>     > A descriptor cannot be closed until all associated channel have been
> destroyed - that's what the bdev programming guide says. When there are
> multiple I/O channels, there has to be some multi-thread management
> involved. Also, those channels can't be closed until all their pending I/O has
> finished. So closing a descriptor will likely have the following flow:
>     >
>     > external request (e.g. bdev hotremove or some RPC) -> start draining I/O
> on all threads -> destroy each i/o channel after its pending i/o has finished ->
> on the last thread to destroy a channel schedule closing the desc to a proper
> thread -> close the desc
>     >
>     > This additional scheduling of spdk_bdev_close() looks completely
> unnecessary - it also forces the upper layer to maintain a pointer to the desc
> thread somewhere, because desc->thread is private to the bdev layer. So to
> repeat the question again -
>     > would it be possible for spdk_bdev_close() to internally schedule itself
> on desc->thread, so that spdk_bdev_close() can be called from any thread?
>     >
>     > D.
>     >
>     > _______________________________________________
>     > SPDK mailing list
>     > SPDK(a)lists.01.org
>     > https://lists.01.org/mailman/listinfo/spdk
>     _______________________________________________
>     SPDK mailing list
>     SPDK(a)lists.01.org
>     https://lists.01.org/mailman/listinfo/spdk
> 
> 
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk