From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============6145719855589929663=="
MIME-Version: 1.0
From: Stojaczyk, Dariusz <dariusz.stojaczyk at intel.com>
Subject: Re: [SPDK] spdk_bdev_close() threading
Date: Thu, 04 Apr 2019 22:09:36 +0000
Message-ID: <FBE7E039FA50BF47A673AD0BD3CD56A8462F9B88@HASMSX105.ger.corp.intel.com>
In-Reply-To: 2B3BE5D9-08D2-4170-806C-65E27C875EDB@intel.com
List-ID: <spdk@lists.01.org>
To: spdk@lists.01.org

--===============6145719855589929663==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable



> -----Original Message-----
> From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, Jame=
s 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
> =

> =

> =

> =EF=BB=BFOn 3/28/19, 7:56 AM, "SPDK on behalf of Stojaczyk, Dariusz" <spd=
k-
> 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->th=
read
> 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 =3D 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. m=
ultiple
> 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 wa=
nt
> bdev to call the user's remove callback after they've closed the descript=
or.
> 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 mus=
t 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 - meanin=
g 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 u=
ser
> 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 bd=
ev
> layer itself will actually create even more work (and trickier work) on t=
he
> 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

--===============6145719855589929663==--