> -----Original Message-----
> From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
> Sent: Thursday, March 28, 2019 9:41 PM
> To: Storage Performance Development Kit <spdk(a)lists.01.org>
> Subject: Re: [SPDK] spdk_bdev_close() threading
> 
> 
> 
> On 3/28/19, 7:56 AM, "SPDK on behalf of Stojaczyk, Dariusz" <spdk-
> bounces(a)lists.01.org on behalf of dariusz.stojaczyk(a)intel.com> wrote:
> 
> <snip>
> 
>     >
>     >     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?
> 
> Not really.  We have unit tests for simulating multi-thread cases (i.e. multiple
> spdk_thread objects) but it's done from a single pthread.
> 
> Thinking about this more though - how do we avoid this race?  We don't want
> bdev to call the user's remove callback after they've closed the descriptor.
> We ensure this today since the remove_callback gets called on the same
> thread as the descriptor was opened.  User will have to be aware that if the
> descriptor was opened on thread A, and is closed on thread B, that it must be
> ready to handle a remove callback on thread A at any point up until
> spdk_bdev_close() returns on thread B.  That sounds messy.

If the application opens a desc on thread A and closes on B, it needs to
handle multi-threading anyway. Generally, you could still open and close the
descriptor on a single thread if you want to keep things simple.

>  A typical first
> reaction to getting a remove callback is to close the descriptor - meaning the
> user has to add its own locking to ensure it doesn't try to close the same
> descriptor on two different threads.

But how we could possibly avoid that with multi-threading?

> 
> What if we just add a helper function (spdk_bdev_desc_get_thread?) to
> return the thread that the descriptor was opened on.  Then at least the user
> doesn't have to keep track of where it was opened itself.  This may still add
> some work on the caller side, but I think trying to push this into the bdev
> layer itself will actually create even more work (and trickier work) on the
> caller side to handle the hot remove race.

I managed to push it into the bdev layer with barely 70 lines of code:
https://review.gerrithub.io/c/spdk/spdk/+/450112

It doesn't affect any use cases which close the descriptor on the same
thread that opened it - it only lifts those single thread limitations.

D.

> 
> -Jim
> 
> 
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk