* [Drbd-dev] [PATCH] drbd: fix a race condition in update_sync_bits() and receive_bitmap()
@ 2021-09-10 5:40 Rui Xu
2021-09-13 2:27 ` [Drbd-dev] [PATCH v2] " Rui Xu
0 siblings, 1 reply; 6+ messages in thread
From: Rui Xu @ 2021-09-10 5:40 UTC (permalink / raw)
To: philipp.reisner, drbd-dev; +Cc: dongsheng.yang
There is a race condition in update_sync_bits() and receive_bitmap(),
please consider this scenario:
Primary: node-3, Secondary node-1, node-2
(1) network failure happend on node-1.
(2) node-1 network recovery.
(3) node-1 connect to node-2, and start resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) before resync in (3) finished, node-1 connect to node-3 and start
resync.(node-1 is PauseSyncTarget, node-3 is PauseSyncSource)
When node-1(SyncTarget) is resync with node-2(SyncSource), node-1 may
set bitmap for node-3 in receive_resync_read()->drbd_set_all_out_of_sync(),
and clear the bimap for node-3 when got P_PEERS_IN_SYNC from node-2.
Then there is a possibility scenario as below:
thread:ack_receiver (node-1) thread:receiver (node-1)
update_sync_bits() receive_bitmap()
set the rs_is_done to 1 set the bitmap for node-3
set the repl_state to PauseSyncTarget
set RS_DONE flag
it will lead the reysnc of node-1 and node-3 to finish in an unexpected way, so
we need to determine the is_sync_target_state before getting the bitmap total
weight in update_sync_bits.
---
drbd/drbd_actlog.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drbd/drbd_actlog.c b/drbd/drbd_actlog.c
index 841e5149..3d2fd399 100644
--- a/drbd/drbd_actlog.c
+++ b/drbd/drbd_actlog.c
@@ -1044,11 +1044,11 @@ static bool lazy_bitmap_update_due(struct drbd_peer_device *peer_device)
}
static void maybe_schedule_on_disk_bitmap_update(struct drbd_peer_device *peer_device,
- bool rs_done)
+ bool rs_done, bool is_sync_target)
{
if (rs_done) {
if (peer_device->connection->agreed_pro_version <= 95 ||
- is_sync_target_state(peer_device, NOW))
+ is_sync_target)
set_bit(RS_DONE, &peer_device->flags);
/* If sync source: rather wait for explicit notification via
@@ -1105,11 +1105,12 @@ static int update_sync_bits(struct drbd_peer_device *peer_device,
}
if (count) {
if (mode == SET_IN_SYNC) {
+ bool is_sync_target = is_sync_target_state(peer_device, NOW);
unsigned long still_to_go = drbd_bm_total_weight(peer_device);
bool rs_is_done = (still_to_go <= peer_device->rs_failed);
drbd_advance_rs_marks(peer_device, still_to_go);
if (cleared || rs_is_done)
- maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done);
+ maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done, is_sync_target);
} else if (mode == RECORD_RS_FAILED) {
peer_device->rs_failed += count;
} else /* if (mode == SET_OUT_OF_SYNC) */ {
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Drbd-dev] [PATCH v2] drbd: fix a race condition in update_sync_bits() and receive_bitmap()
2021-09-10 5:40 [Drbd-dev] [PATCH] drbd: fix a race condition in update_sync_bits() and receive_bitmap() Rui Xu
@ 2021-09-13 2:27 ` Rui Xu
2021-09-13 15:21 ` Joel Colledge
0 siblings, 1 reply; 6+ messages in thread
From: Rui Xu @ 2021-09-13 2:27 UTC (permalink / raw)
To: philipp.reisner, drbd-dev; +Cc: dongsheng.yang
There is a race condition in update_sync_bits() and receive_bitmap(),
please consider this scenario:
Primary: node-3, Secondary node-1, node-2
(1) network failure happend on node-1.
(2) node-1 network recovery.
(3) node-1 connect to node-2, and start resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) before resync in (3) finished, node-1 connect to node-3 and start
resync.(node-1 is PauseSyncTarget, node-3 is PauseSyncSource)
When node-1(SyncTarget) is resync with node-2(SyncSource), node-1 may
set bitmap for node-3 in receive_resync_read()->drbd_set_all_out_of_sync(),
and clear the bitmap for node-3 when got P_PEERS_IN_SYNC from node-2.
Then there is a possibility scenario as below:
thread:ack_receiver (node-1) thread:receiver (node-1)
update_sync_bits() receive_bitmap()
set the rs_is_done to 1
set the bitmap for node-3
set the repl_state to PauseSyncTarget
set RS_DONE flag
it will lead the reysnc of node-1 and node-3 to finish in an unexpected way, so
we need to determine the is_sync_target_state before getting the bitmap total
weight in update_sync_bits.
---
changelog:
-v1: fix typo in commit message
drbd/drbd_actlog.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drbd/drbd_actlog.c b/drbd/drbd_actlog.c
index 841e5149..3d2fd399 100644
--- a/drbd/drbd_actlog.c
+++ b/drbd/drbd_actlog.c
@@ -1044,11 +1044,11 @@ static bool lazy_bitmap_update_due(struct drbd_peer_device *peer_device)
}
static void maybe_schedule_on_disk_bitmap_update(struct drbd_peer_device *peer_device,
- bool rs_done)
+ bool rs_done, bool is_sync_target)
{
if (rs_done) {
if (peer_device->connection->agreed_pro_version <= 95 ||
- is_sync_target_state(peer_device, NOW))
+ is_sync_target)
set_bit(RS_DONE, &peer_device->flags);
/* If sync source: rather wait for explicit notification via
@@ -1105,11 +1105,12 @@ static int update_sync_bits(struct drbd_peer_device *peer_device,
}
if (count) {
if (mode == SET_IN_SYNC) {
+ bool is_sync_target = is_sync_target_state(peer_device, NOW);
unsigned long still_to_go = drbd_bm_total_weight(peer_device);
bool rs_is_done = (still_to_go <= peer_device->rs_failed);
drbd_advance_rs_marks(peer_device, still_to_go);
if (cleared || rs_is_done)
- maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done);
+ maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done, is_sync_target);
} else if (mode == RECORD_RS_FAILED) {
peer_device->rs_failed += count;
} else /* if (mode == SET_OUT_OF_SYNC) */ {
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Drbd-dev] [PATCH v2] drbd: fix a race condition in update_sync_bits() and receive_bitmap()
2021-09-13 2:27 ` [Drbd-dev] [PATCH v2] " Rui Xu
@ 2021-09-13 15:21 ` Joel Colledge
2021-09-15 11:07 ` [Drbd-dev] [PATCH v3] " Rui Xu
0 siblings, 1 reply; 6+ messages in thread
From: Joel Colledge @ 2021-09-13 15:21 UTC (permalink / raw)
To: Rui Xu; +Cc: Philipp Reisner, dongsheng.yang, drbd-dev
Thanks for the contribution. The problem description and patch look valid to me.
Please add a brief comment to the code explaining why it is important
to evaluate is_sync_target_state at that point.
Here is a cleaned up version of your commit message. Is it still
correct? In particular, I am not sure what the consequences of the bug
are. "resync finishes in an unexpected way" is vague. I wrote "even
though bitmap bits are still set" but maybe there are other effects
that I have not thought of.
drbd: fix a race condition in update_sync_bits() and receive_bitmap()
There was a race condition involving update_sync_bits() and
receive_bitmap(). Consider this scenario:
Primary: node-3, Secondary node-1, node-2
(1) Network failure occurs on node-1
(2) node-1 network recovers
(3) node-1 connects to node-2, and starts resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) Before resync in (3) finishes, node-1 connects to node-3 and starts
resync (node-1 is PausedSyncT, node-3 is PausedSyncS)
The following sequence can occur on node-1 while it is syncing from
node-2:
* ack_receiver thread processes P_PEERS_IN_SYNC
* ack_receiver: call update_sync_bits()
* ack_receiver: clear the last bitmap bits for node-3
* ack_receiver: set rs_is_done to 1
* receiver thread processes P_*BITMAP
* receiver: call receive_bitmap()
* receiver: set bitmap bits for node-3
* receiver: set the repl_state towards node-3 to PausedSyncT
* ack_receiver: set RS_DONE flag
This causes the resync from node-3 to node-1 to finish even though
bitmap bits are still set. Fix this by evaluating is_sync_target_state
before getting the bitmap total weight in update_sync_bits.
Signed-off-by: Rui Xu <rui.xu@easystack.cn>
Signed-off-by: Joel Colledge <joel.colledge@linbit.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Drbd-dev] [PATCH v3] drbd: fix a race condition in update_sync_bits() and receive_bitmap()
2021-09-13 15:21 ` Joel Colledge
@ 2021-09-15 11:07 ` Rui Xu
2021-09-21 14:14 ` Joel Colledge
0 siblings, 1 reply; 6+ messages in thread
From: Rui Xu @ 2021-09-15 11:07 UTC (permalink / raw)
To: philipp.reisner, drbd-dev, joel.colledge; +Cc: Rui Xu, dongsheng.yang
There was a race condition involving update_sync_bits() and
receive_bitmap(). Consider this scenario:
Primary: node-3, Secondary: node-1, node-2
(1) Network failure occurs on node-1
(2) node-1 network recovers
(3) node-1 connects to node-2, and starts resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) Before resync in (3) finishes, node-1 connects to node-3 and starts
resync (node-1 is PausedSyncT, node-3 is PausedSyncS)
The following sequence can occur on node-1 while it is syncing from
node-2:
* ack_receiver thread processes P_PEERS_IN_SYNC
* ack_receiver: call update_sync_bits()
* ack_receiver: clear the last bitmap bits for node-3
* ack_receiver: set rs_is_done to 1
* receiver thread processes P_*BITMAP
* receiver: call receive_bitmap()
* receiver: set bitmap bits for node-3
* receiver: set the repl_state towards node-3 to PausedSyncT
* ack_receiver: set RS_DONE flag
This causes the resync from node-3 to node-1 to finish even though
bitmap bits are still set, and node-1 will finish the resync as
PausedSyncT, but the resync from node-2 to node-1 is still ongoing,
at last, it will finish as a unstable resync, and leaving the disk
state end into INCONSISTENT finally.
Fix this by evaluating is_sync_target_state before getting the bitmap
total weight in update_sync_bits.
Signed-off-by: Rui Xu <rui.xu@easystack.cn>
Signed-off-by: Joel Colledge <joel.colledge@linbit.com>
---
changelog:
-From v1: fix typo in commit message
-From v2: add a brief comment to the code
and added the bug description in commit message
drbd/drbd_actlog.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drbd/drbd_actlog.c b/drbd/drbd_actlog.c
index 841e5149..01e64823 100644
--- a/drbd/drbd_actlog.c
+++ b/drbd/drbd_actlog.c
@@ -1044,11 +1044,11 @@ static bool lazy_bitmap_update_due(struct drbd_peer_device *peer_device)
}
static void maybe_schedule_on_disk_bitmap_update(struct drbd_peer_device *peer_device,
- bool rs_done)
+ bool rs_done, bool is_sync_target)
{
if (rs_done) {
if (peer_device->connection->agreed_pro_version <= 95 ||
- is_sync_target_state(peer_device, NOW))
+ is_sync_target)
set_bit(RS_DONE, &peer_device->flags);
/* If sync source: rather wait for explicit notification via
@@ -1105,11 +1105,18 @@ static int update_sync_bits(struct drbd_peer_device *peer_device,
}
if (count) {
if (mode == SET_IN_SYNC) {
+ /* Evaluate is_sync_target_state before getting the bm total
+ * weight to avoid a scenario: still_to_go is 0 when sync
+ * state is not sync_target_state, but sync state change to
+ * sync_target_state later and finish the resync even though
+ * bitmap bits are still set.
+ */
+ bool is_sync_target = is_sync_target_state(peer_device, NOW);
unsigned long still_to_go = drbd_bm_total_weight(peer_device);
bool rs_is_done = (still_to_go <= peer_device->rs_failed);
drbd_advance_rs_marks(peer_device, still_to_go);
if (cleared || rs_is_done)
- maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done);
+ maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done, is_sync_target);
} else if (mode == RECORD_RS_FAILED) {
peer_device->rs_failed += count;
} else /* if (mode == SET_OUT_OF_SYNC) */ {
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-23 11:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-10 5:40 [Drbd-dev] [PATCH] drbd: fix a race condition in update_sync_bits() and receive_bitmap() Rui Xu
2021-09-13 2:27 ` [Drbd-dev] [PATCH v2] " Rui Xu
2021-09-13 15:21 ` Joel Colledge
2021-09-15 11:07 ` [Drbd-dev] [PATCH v3] " Rui Xu
2021-09-21 14:14 ` Joel Colledge
2021-09-23 11:18 ` Joel Colledge
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox