Re: [SPDK] spdk_bdev_close() threading

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

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



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.  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.

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.

-Jim