Re: [SPDK] spdk_bdev_close() threading

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

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



> -----Original Message-----
> From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Andrey Kuzmin
> Sent: Thursday, March 28, 2019 11:40 AM
> To: Storage Performance Development Kit <spdk(a)lists.01.org>
> Subject: Re: [SPDK] spdk_bdev_close() threading
> 
> On Thu, Mar 28, 2019, 12:13 Stojaczyk, Dariusz <dariusz.stojaczyk(a)intel.com>
> wrote:
> 
> > I was surprised to see that bdev descriptors can be closed only from the
> > same thread that opened them.
> 
> 
> Sounds counterintuitive since descriptor, contrary to io channel, may be
> shared among multiple threads. Furthermore, I believe such a requirement,
> if introduced, will break a lot more code than just vhost.
> 
> 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?
> >
> > 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
> >
> 
> I believe this is what is very much already happening when bdev_close is
> issued, although wrt the underlying io device (which is the actual
> reference-counting entity for the i/o channels).

That's true. With the current implementation you can even close the descriptor before putting any io channels. The bdev will be unregistered on the proper thread once the last io channel is destroyed. It's not safe to rely on this behavior, but that's actually how vhost block hotremove works.

D.

> 
> Regards,
> Andrey
> 
> >
> > 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