[PATCH v2] drbd: fix throttling on newly created DM backing devices

[PATCH v2] drbd: fix throttling on newly created DM backing devices

[Imre Palik]

From: "Palik, Imre" <imrep@amazon.de>

If the drbd backing device is a new device mapper device (e.g., a
dm-linear mapping of an existing block device that contains data), the
counters are initially 0 even though the device contains useful
data. This causes throttling until something accesses the drbd device
or the backing device.

The patch disables throttling, as long as only resync is responsible
for disk activity on a freshly created device.

Reported-by: Mikhail Sugakov <msugakov@amazon.de>
Cc: Matt Wilson <msw@amazon.com>
Signed-off-by: Imre Palik <imrep@amazon.de>
---
 drivers/block/drbd/drbd_main.c     |    3 ++-
 drivers/block/drbd/drbd_receiver.c |    3 ++-
 drivers/block/drbd/drbd_state.c    |    2 +-
 drivers/block/drbd/drbd_worker.c   |    2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 9b465bb..f4e3944 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -1966,6 +1966,7 @@ void drbd_init_set_defaults(struct drbd_device *device)
 	device->resync_wenr = LC_FREE;
 	device->peer_max_bio_size = DRBD_MAX_BIO_SIZE_SAFE;
 	device->local_max_bio_size = DRBD_MAX_BIO_SIZE_SAFE;
+	device->rs_last_events = -1;
 }
 
 void drbd_device_cleanup(struct drbd_device *device)
@@ -1985,7 +1986,7 @@ void drbd_device_cleanup(struct drbd_device *device)
 	device->rs_start     =
 	device->rs_total     =
 	device->rs_failed    = 0;
-	device->rs_last_events = 0;
+	device->rs_last_events = -1;
 	device->rs_last_sect_ev = 0;
 	for (i = 0; i < DRBD_SYNC_MARKS; i++) {
 		device->rs_mark_left[i] = 0;
diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index 9342b8d..d33355c 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -2482,7 +2482,8 @@ bool drbd_rs_c_min_rate_throttle(struct drbd_device *device)
 			atomic_read(&device->rs_sect_ev);
 
 	if (atomic_read(&device->ap_actlog_cnt)
-	    || !device->rs_last_events || curr_events - device->rs_last_events > 64) {
+		|| device->rs_last_events < 0
+		|| curr_events - device->rs_last_events > 64) {
 		unsigned long rs_left;
 		int i;
 
diff --git a/drivers/block/drbd/drbd_state.c b/drivers/block/drbd/drbd_state.c
index c35c0f0..8789f8a 100644
--- a/drivers/block/drbd/drbd_state.c
+++ b/drivers/block/drbd/drbd_state.c
@@ -1072,7 +1072,7 @@ __drbd_set_state(struct drbd_device *device, union drbd_state ns,
 
 		set_ov_position(device, ns.conn);
 		device->rs_start = now;
-		device->rs_last_events = 0;
+		device->rs_last_events = -1;
 		device->rs_last_sect_ev = 0;
 		device->ov_last_oos_size = 0;
 		device->ov_last_oos_start = 0;
diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
index 50776b3..e2db4e4 100644
--- a/drivers/block/drbd/drbd_worker.c
+++ b/drivers/block/drbd/drbd_worker.c
@@ -1743,7 +1743,7 @@ void drbd_start_resync(struct drbd_device *device, enum drbd_conns side)
 		device->rs_failed    = 0;
 		device->rs_paused    = 0;
 		device->rs_same_csum = 0;
-		device->rs_last_events = 0;
+		device->rs_last_events = -1;
 		device->rs_last_sect_ev = 0;
 		device->rs_total     = tw;
 		device->rs_start     = now;
-- 
1.7.9.5

Re: [PATCH v2] drbd: fix throttling on newly created DM backing devices

[Lars Ellenberg]

On Thu, Sep 04, 2014 at 09:59:09PM +0200, Imre Palik wrote:
> From: "Palik, Imre" <imrep@amazon.de>
> 
> If the drbd backing device is a new device mapper device (e.g., a
> dm-linear mapping of an existing block device that contains data), the
> counters are initially 0 even though the device contains useful
> data. This causes throttling until something accesses the drbd device
> or the backing device.
> 
> The patch disables throttling, as long as only resync is responsible
> for disk activity on a freshly created device.

That does not change a thing, actually it makes matters worse,
if for some reason "curr_events" becomes negative, which can easily
happen... wrap arounds and all ... 

> 
> +	device->rs_last_events = -1;

>  	if (atomic_read(&device->ap_actlog_cnt)
> -	    || !device->rs_last_events || curr_events - device->rs_last_events > 64) {
> +		|| device->rs_last_events < 0
> +		|| curr_events - device->rs_last_events > 64) {
>  		unsigned long rs_left;

No, what I meant is something more like this.
I still need to review if the ->ldev->backing_bdev-> ... deref
is always safe to do. But I think it is.

commit c90cac67d6c12dcf3945f67a28718c40b644cc32
Author: Lars Ellenberg <lars.ellenberg@linbit.com>
Date:   Thu Sep 4 13:15:45 2014 +0200

    drbd: fix resync throttling initialization
    
    If for some reason DRBD resync was the only activity on a backend
    device, drbd_rs_c_min_rate_throttle() would mistakenly decide that it is
    still initialization time, and keep throttling the resync.
    
    This patch explicitly initializes ->rs_last_events to the current
    backend event counters, and drops the rs_last_events == 0 from the
    throttle condition.
    
    Reported-by: Mikhail Sugakov <msugakov@amazon.de>

diff --git a/drbd/drbd_receiver.c b/drbd/drbd_receiver.c
index 7376edd..6da9d25 100644
--- a/drbd/drbd_receiver.c
+++ b/drbd/drbd_receiver.c
@@ -2867,7 +2867,7 @@ bool drbd_rs_c_min_rate_throttle(struct drbd_device *device)
 		    - atomic_read(&device->rs_sect_ev);
 
 	if (atomic_read(&device->ap_actlog_cnt)
-	|| !device->rs_last_events || curr_events - device->rs_last_events > 64) {
+	|| curr_events - device->rs_last_events > 64) {
 		unsigned long rs_left;
 		int i;
 
diff --git a/drbd/drbd_state.c b/drbd/drbd_state.c
index bdad488..1df1a63 100644
--- a/drbd/drbd_state.c
+++ b/drbd/drbd_state.c
@@ -1078,7 +1078,6 @@ __drbd_set_state(struct drbd_device *device, union drbd_state ns,
 
 		set_ov_position(device, ns.conn);
 		device->rs_start = now;
-		device->rs_last_events = 0;
 		device->rs_last_sect_ev = 0;
 		device->ov_last_oos_size = 0;
 		device->ov_last_oos_start = 0;
diff --git a/drbd/drbd_worker.c b/drbd/drbd_worker.c
index 0f11292..43dd841 100644
--- a/drbd/drbd_worker.c
+++ b/drbd/drbd_worker.c
@@ -1630,6 +1630,7 @@ void drbd_rs_controller_reset(struct drbd_device *device)
 	atomic_set(&device->rs_sect_in, 0);
 	atomic_set(&device->rs_sect_ev, 0);
 	device->rs_in_flight = 0;
+	device->rs_last_events = drbd_backing_bdev_events(device->ldev->backing_bdev->bd_contains->bd_disk);
 
 	/* Updating the RCU protected object in place is necessary since
 	   this function gets called from atomic context.
@@ -1776,7 +1777,6 @@ void drbd_start_resync(struct drbd_device *device, enum drbd_conns side)
 		device->rs_failed    = 0;
 		device->rs_paused    = 0;
 		device->rs_same_csum = 0;
-		device->rs_last_events = 0;
 		device->rs_last_sect_ev = 0;
 		device->rs_total     = tw;
 		device->rs_start     = now;