On Wed, Dec 20, 2023 at 08:49:02PM -0500, Stefan Hajnoczi wrote: > The NBD clients list is currently accessed from both the export > AioContext and the main loop thread. When the AioContext lock is removed > there will be nothing protecting the clients list. > > Adding a lock around the clients list is tricky because NBDClient > structs are refcounted and may be freed from the export AioContext or > the main loop thread. nbd_export_request_shutdown() -> client_close() -> > nbd_client_put() is also tricky because the list lock would be held > while indirectly dropping references to NDBClients. > > A simpler approach is to only allow nbd_client_put() and client_close() > calls from the main loop thread. Then the NBD clients list is only > accessed from the main loop thread and no fancy locking is needed. > > nbd_trip() just needs to reschedule itself in the main loop AioContext > before calling nbd_client_put() and client_close(). This costs more CPU > cycles per NBD request but is needed for thread-safety when the > AioContext lock is removed.
Late review (now that this is already in), but this is a bit misleading. The CPU penalty is only incurred for NBD_CMD_DISC or after detection of a protocol error (that is, only when the connection is being shut down), and not on every NBD request. Thanks for working on this! > > Note that nbd_client_get() can still be called from either thread, so > make NBDClient->refcount atomic. > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > nbd/server.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
