Re: [SPDK] spdk_bdev_close() threading

[Harris, James R <james.r.harris at intel.com>]

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



On 4/4/19, 3:09 PM, "SPDK on behalf of Stojaczyk, Dariusz" <spdk-bounces(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 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

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 the 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 remove notification).

To prevent these two threads from racing to call spdk_bdev_close(), the caller 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 doesn'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 rather avoid adding it if possible.  This stuff is inherently difficult to really to test and ensure we get right.  This patch may only be about 120 lines 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