| Message ID | 20220414175756.671165-7-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: actually make s->state thread-safe | expand |
On Thu, Apr 14, 2022 at 07:57:53PM +0200, Paolo Bonzini wrote: > Prepare for the next patch, so that the diff is less confusing. > > nbd_client_connecting is moved closer to the definition point. > > nbd_client_connecting_wait() is kept only for the reconnection > logic; when it is used to check if a request has to be reissued, > use the renamed function nbd_client_will_reconnect(). In the > next patch, the two cases will have different locking requirements. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/nbd.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> Yes, this split makes the next patch easier to read ;)
14.04.2022 20:57, Paolo Bonzini wrote: > Prepare for the next patch, so that the diff is less confusing. > > nbd_client_connecting is moved closer to the definition point. Amm. To usage-point you mean? The original idea was to keep simple state-reading helpers definitions together :) > > nbd_client_connecting_wait() is kept only for the reconnection > logic; when it is used to check if a request has to be reissued, > use the renamed function nbd_client_will_reconnect(). In the > next patch, the two cases will have different locking requirements. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org> > --- > block/nbd.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/block/nbd.c b/block/nbd.c > index a2414566d1..37d466e435 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -254,18 +254,15 @@ static void open_timer_init(BDRVNBDState *s, uint64_t expire_time_ns) > timer_mod(s->open_timer, expire_time_ns); > } > > -static bool nbd_client_connecting(BDRVNBDState *s) > -{ > - NBDClientState state = qatomic_load_acquire(&s->state); > - return state == NBD_CLIENT_CONNECTING_WAIT || > - state == NBD_CLIENT_CONNECTING_NOWAIT; > -} > - > static bool nbd_client_connecting_wait(BDRVNBDState *s) > { > return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT; > } > > +static bool nbd_client_will_reconnect(BDRVNBDState *s) > +{ > + return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT; > +} > /* > * Update @bs with information learned during a completed negotiation process. > * Return failure if the server's advertised options are incompatible with the > @@ -355,6 +352,13 @@ int coroutine_fn nbd_co_do_establish_connection(BlockDriverState *bs, > return 0; > } > > +static bool nbd_client_connecting(BDRVNBDState *s) > +{ > + NBDClientState state = qatomic_load_acquire(&s->state); > + return state == NBD_CLIENT_CONNECTING_WAIT || > + state == NBD_CLIENT_CONNECTING_NOWAIT; > +} > + > /* Called with s->requests_lock taken. */ > static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s) > { > @@ -1190,7 +1194,7 @@ static int coroutine_fn nbd_co_request(BlockDriverState *bs, NBDRequest *request > error_free(local_err); > local_err = NULL; > } > - } while (ret < 0 && nbd_client_connecting_wait(s)); > + } while (ret < 0 && nbd_client_will_reconnect(s)); > > return ret ? ret : request_ret; > } > @@ -1249,7 +1253,7 @@ static int coroutine_fn nbd_client_co_preadv(BlockDriverState *bs, int64_t offse > error_free(local_err); > local_err = NULL; > } > - } while (ret < 0 && nbd_client_connecting_wait(s)); > + } while (ret < 0 && nbd_client_will_reconnect(s)); > > return ret ? ret : request_ret; > } > @@ -1407,7 +1411,7 @@ static int coroutine_fn nbd_client_co_block_status( > error_free(local_err); > local_err = NULL; > } > - } while (ret < 0 && nbd_client_connecting_wait(s)); > + } while (ret < 0 && nbd_client_will_reconnect(s)); > > if (ret < 0 || request_ret < 0) { > return ret ? ret : request_ret;
On 4/16/22 14:37, Vladimir Sementsov-Ogievskiy wrote: > 14.04.2022 20:57, Paolo Bonzini wrote: >> Prepare for the next patch, so that the diff is less confusing. >> >> nbd_client_connecting is moved closer to the definition point. > > Amm. To usage-point you mean? > The original idea was to keep simple state-reading helpers definitions > together :) Yes and it makes sense. The new idea is to keep requests_lock functions together instead. :) Paolo >> >> nbd_client_connecting_wait() is kept only for the reconnection >> logic; when it is used to check if a request has to be reissued, >> use the renamed function nbd_client_will_reconnect(). In the >> next patch, the two cases will have different locking requirements. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org> > >> --- >> block/nbd.c | 24 ++++++++++++++---------- >> 1 file changed, 14 insertions(+), 10 deletions(-) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index a2414566d1..37d466e435 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -254,18 +254,15 @@ static void open_timer_init(BDRVNBDState *s, >> uint64_t expire_time_ns) >> timer_mod(s->open_timer, expire_time_ns); >> } >> -static bool nbd_client_connecting(BDRVNBDState *s) >> -{ >> - NBDClientState state = qatomic_load_acquire(&s->state); >> - return state == NBD_CLIENT_CONNECTING_WAIT || >> - state == NBD_CLIENT_CONNECTING_NOWAIT; >> -} >> - >> static bool nbd_client_connecting_wait(BDRVNBDState *s) >> { >> return qatomic_load_acquire(&s->state) == >> NBD_CLIENT_CONNECTING_WAIT; >> } >> +static bool nbd_client_will_reconnect(BDRVNBDState *s) >> +{ >> + return qatomic_load_acquire(&s->state) == >> NBD_CLIENT_CONNECTING_WAIT; >> +} >> /* >> * Update @bs with information learned during a completed >> negotiation process. >> * Return failure if the server's advertised options are >> incompatible with the >> @@ -355,6 +352,13 @@ int coroutine_fn >> nbd_co_do_establish_connection(BlockDriverState *bs, >> return 0; >> } >> +static bool nbd_client_connecting(BDRVNBDState *s) >> +{ >> + NBDClientState state = qatomic_load_acquire(&s->state); >> + return state == NBD_CLIENT_CONNECTING_WAIT || >> + state == NBD_CLIENT_CONNECTING_NOWAIT; >> +} >> + >> /* Called with s->requests_lock taken. */ >> static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s) >> { >> @@ -1190,7 +1194,7 @@ static int coroutine_fn >> nbd_co_request(BlockDriverState *bs, NBDRequest *request >> error_free(local_err); >> local_err = NULL; >> } >> - } while (ret < 0 && nbd_client_connecting_wait(s)); >> + } while (ret < 0 && nbd_client_will_reconnect(s)); >> return ret ? ret : request_ret; >> } >> @@ -1249,7 +1253,7 @@ static int coroutine_fn >> nbd_client_co_preadv(BlockDriverState *bs, int64_t offse >> error_free(local_err); >> local_err = NULL; >> } >> - } while (ret < 0 && nbd_client_connecting_wait(s)); >> + } while (ret < 0 && nbd_client_will_reconnect(s)); >> return ret ? ret : request_ret; >> } >> @@ -1407,7 +1411,7 @@ static int coroutine_fn nbd_client_co_block_status( >> error_free(local_err); >> local_err = NULL; >> } >> - } while (ret < 0 && nbd_client_connecting_wait(s)); >> + } while (ret < 0 && nbd_client_will_reconnect(s)); >> if (ret < 0 || request_ret < 0) { >> return ret ? ret : request_ret; > >
diff --git a/block/nbd.c b/block/nbd.c index a2414566d1..37d466e435 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -254,18 +254,15 @@ static void open_timer_init(BDRVNBDState *s, uint64_t expire_time_ns) timer_mod(s->open_timer, expire_time_ns); } -static bool nbd_client_connecting(BDRVNBDState *s) -{ - NBDClientState state = qatomic_load_acquire(&s->state); - return state == NBD_CLIENT_CONNECTING_WAIT || - state == NBD_CLIENT_CONNECTING_NOWAIT; -} - static bool nbd_client_connecting_wait(BDRVNBDState *s) { return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT; } +static bool nbd_client_will_reconnect(BDRVNBDState *s) +{ + return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT; +} /* * Update @bs with information learned during a completed negotiation process. * Return failure if the server's advertised options are incompatible with the @@ -355,6 +352,13 @@ int coroutine_fn nbd_co_do_establish_connection(BlockDriverState *bs, return 0; } +static bool nbd_client_connecting(BDRVNBDState *s) +{ + NBDClientState state = qatomic_load_acquire(&s->state); + return state == NBD_CLIENT_CONNECTING_WAIT || + state == NBD_CLIENT_CONNECTING_NOWAIT; +} + /* Called with s->requests_lock taken. */ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s) { @@ -1190,7 +1194,7 @@ static int coroutine_fn nbd_co_request(BlockDriverState *bs, NBDRequest *request error_free(local_err); local_err = NULL; } - } while (ret < 0 && nbd_client_connecting_wait(s)); + } while (ret < 0 && nbd_client_will_reconnect(s)); return ret ? ret : request_ret; } @@ -1249,7 +1253,7 @@ static int coroutine_fn nbd_client_co_preadv(BlockDriverState *bs, int64_t offse error_free(local_err); local_err = NULL; } - } while (ret < 0 && nbd_client_connecting_wait(s)); + } while (ret < 0 && nbd_client_will_reconnect(s)); return ret ? ret : request_ret; } @@ -1407,7 +1411,7 @@ static int coroutine_fn nbd_client_co_block_status( error_free(local_err); local_err = NULL; } - } while (ret < 0 && nbd_client_connecting_wait(s)); + } while (ret < 0 && nbd_client_will_reconnect(s)); if (ret < 0 || request_ret < 0) { return ret ? ret : request_ret;
Prepare for the next patch, so that the diff is less confusing. nbd_client_connecting is moved closer to the definition point. nbd_client_connecting_wait() is kept only for the reconnection logic; when it is used to check if a request has to be reissued, use the renamed function nbd_client_will_reconnect(). In the next patch, the two cases will have different locking requirements. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/nbd.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)