From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============2846420548047457270=="
MIME-Version: 1.0
From: Andrey Kuzmin <andrey.v.kuzmin at gmail.com>
Subject: Re: [SPDK] spdk_bdev_close() threading
Date: Thu, 28 Mar 2019 13:40:24 +0300
Message-ID: <CANvN+enN9nHji+tCu4AxyKvDW33XAR1agQozgFucx3G6xty+wg@mail.gmail.com>
In-Reply-To: FBE7E039FA50BF47A673AD0BD3CD56A8462F2A50@HASMSX105.ger.corp.intel.com
List-ID: <spdk@lists.01.org>
To: spdk@lists.01.org

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

On Thu, Mar 28, 2019, 12:13 Stojaczyk, Dariusz <dariusz.stojaczyk(a)intel.c=
om>
wrote:

> I was surprised to see that bdev descriptors can be closed only from the
> same thread that opened them.


Sounds counterintuitive since descriptor, contrary to io channel, may be
shared among multiple threads. Furthermore, I believe such a requirement,
if introduced, will break a lot more code than just vhost.

Vhost doesn't respect this rule. As expected, I was able to trigger
> assert(desc->thread =3D=3D spdk_get_thread()) while closing a vhost scsi
> descriptor using the latest SPDK master. This could be probably fixed by
> always scheduling the spdk_bdev_close() to proper thread. Maybe vhost cou=
ld
> even immediately assume the descriptor is closed and set its `desc` point=
er
> to NULL without waiting for spdk_bdev_close() to be actually called. But
> why the descriptor needs to be closed from a specific thread in the first
> place? Would it be possible for spdk_bdev_close() to internally schedule
> itself on desc->thread?
>
> A descriptor cannot be closed until all associated channel have been
> destroyed - that's what the bdev programming guide says. When there are
> multiple I/O channels, there has to be some multi-thread management
> involved. Also, those channels can't be closed until all their pending I/O
> has finished. So closing a descriptor will likely have the following flow:
>
> external request (e.g. bdev hotremove or some RPC) -> start draining I/O
> on all threads -> destroy each i/o channel after its pending i/o has
> finished -> on the last thread to destroy a channel schedule closing the
> desc to a proper thread -> close the desc
>

I believe this is what is very much already happening when bdev_close is
issued, although wrt the underlying io device (which is the actual
reference-counting entity for the i/o channels).

Regards,
Andrey

>
> This additional scheduling of spdk_bdev_close() looks completely
> unnecessary - it also forces the upper layer to maintain a pointer to the
> desc thread somewhere, because desc->thread is private to the bdev layer.
> So to repeat the question again -
> would it be possible for spdk_bdev_close() to internally schedule itself
> on desc->thread, so that spdk_bdev_close() can be called from any thread?
>
> D.
>
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
>

--===============2846420548047457270==--