Re: [PATCH v2 for-7.1 9/9] nbd: document what is protected by the CoMutexes

[Paolo Bonzini]

On 4/16/22 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 14.04.2022 20:57, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   block/nbd.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 31c684772e..d0d94b40bd 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -81,12 +81,18 @@ typedef struct BDRVNBDState {
> >       NBDClientRequest requests[MAX_NBD_REQUESTS];
> >       QEMUTimer *reconnect_delay_timer;
> > +    /* Protects sending data on the socket.  */
> >       CoMutex send_mutex;
> > +
> > +    /*
> > +     * Protects receiving reply headers from the socket, as well as the
> > +     * fields reply and requests[].receiving
>
> I think, worth noting, that s->reply is used without mutex after 
> nbd_receive_replies() success and till setting s->reply.handle to 0 in 
> nbd_co_receive_one_chunk()..
>
> Should "s->reply.handle = 0" be done under mutex as well? And may be, in 
> same critical section as nbd_recv_coroutines_wake() ?

Could be an idea.  It could also be a store-release but no reason to be 
fancy:

diff --git a/block/nbd.c b/block/nbd.c
index 0bd9b674a9..cd760bfd50 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -149,11 +149,11 @@ static bool coroutine_fn 
nbd_recv_coroutine_wake_one(NBDClientRequest *req)
     return false;
 }

+/* Called with s->receive_mutex taken.  */
 static void coroutine_fn nbd_recv_coroutines_wake(BDRVNBDState *s)
 {
     int i;

-    QEMU_LOCK_GUARD(&s->receive_mutex);
     for (i = 0; i < MAX_NBD_REQUESTS; i++) {
         if (nbd_recv_coroutine_wake_one(&s->requests[i])) {
             return;
@@ -924,9 +924,11 @@ static coroutine_fn int nbd_co_receive_one_chunk(
         /* For assert at loop start in nbd_connection_entry */
         *reply = s->reply;
     }
-    s->reply.handle = 0;

-    nbd_recv_coroutines_wake(s);
+    WITH_QEMU_LOCK_GUARD(&s->receive_mutex) {
+        s->reply.handle = 0;
+        nbd_recv_coroutines_wake(s);
+    }

     return ret;
 }

Paolo

> > +     */
> >       CoMutex receive_mutex;
> > +    NBDReply reply;
> >       QEMUTimer *open_timer;
> > -    NBDReply reply;
> >       BlockDriverState *bs;
> >       /* Connection parameters */