From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============3413308549872174494=="
MIME-Version: 1.0
From: Harris, James R <james.r.harris at intel.com>
Subject: Re: [SPDK] spdk_bdev_close() threading
Date: Thu, 04 Apr 2019 23:35:17 +0000
Message-ID: <24E34F29-CFE1-453A-984E-ABBDB7B5CEAC@intel.com>
In-Reply-To: FBE7E039FA50BF47A673AD0BD3CD56A8462F9B88@HASMSX105.ger.corp.intel.com
List-ID: <spdk@lists.01.org>
To: spdk@lists.01.org

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



=EF=BB=BFOn 4/4/19, 3:09 PM, "SPDK on behalf of Stojaczyk, Dariusz" <spdk-b=
ounces(a)lists.01.org on behalf of dariusz.stojaczyk(a)intel.com> wrote:

<snip>

    > =

    > 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 desc=
riptor.
    > 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 clos=
e 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 - me=
aning 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 t=
he user
    > doesn't have to keep track of where it was opened itself.  This may s=
till add
    > some work on the caller side, but I think trying to push this into th=
e 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

This doesn't handle the race I mentioned above.  Consider this case:

Thread A opens a bdev.
Thread B later decides it's going to close the bdev for some reason.  Maybe=
 the vhost session, nvmf subsystem, or iscsi target node is being deleted. =
 There may be other reasons for user applications (outside of SPDK) using t=
he bdev layer.  So thread B calls spdk_bdev_close().
At the same time, that bdev is hot-removed.  Thread A will then callback to=
 the user that the bdev was hot removed.  This thread calls spdk_bdev_close=
() to close the descriptor (which is a normal reaction to getting a hot rem=
ove notification).

To prevent these two threads from racing to call spdk_bdev_close(), the cal=
ler needs to add locking or some other kind of synchronization.  This is on=
 top of the extra locking added in this patch.

I guess I don't understand why this can't be handled in vhost.  Have vhost =
send a message to the thread that constructed the SCSI device when it wants=
 to destruct it (which closes the underlying bdevs).  Then that thread does=
n't have to worry about any races - if the bdev was hotremoved by time the =
message arrived, it just doesn't call spdk_bdev_close().

I know there are some cases where we have to do locking in SPDK.  But I'd r=
ather avoid adding it if possible.  This stuff is inherently difficult to r=
eally to test and ensure we get right.  This patch may only be about 120 li=
nes of code, but we don't have any tests to make sure it's correct.

-Jim

    =

    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
    _______________________________________________
    SPDK mailing list
    SPDK(a)lists.01.org
    https://lists.01.org/mailman/listinfo/spdk
    =



--===============3413308549872174494==--