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:
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:
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
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
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.
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.
SPDK mailing list
SPDK mailing list