[PATCH 00/10] block/badblocks: fix badblocks setting error

[PATCH 00/10] block/badblocks: fix badblocks setting error

[linan666]

From: Li Nan <linan122@huawei.com>

The patch series fix the bug of setting badblocks, which may not match
expectations in complex scenarios. Optimize the code to make it more
readable at the same time.

Li Nan (10):
  block/badblocks: only set bb->changed when badblocks changes
  block/badblocks: fix badblocks loss when badblocks combine
  block/badblocks: fix badblocks overlap
  block/badblocks: fix the bug of reverse order
  block/badblocks: fix ack set fail in badblocks_set
  block/badblocks: check bb->count instead of 'hi > lo'
  block/badblocks: factor out a helper to merge badblocks
  block/badblocks: factor out a helper to combine badblocks
  block/badblocks: factor out a helper to create badblocks
  block/badblocks: try to merge badblocks as much as possible

 block/badblocks.c | 300 +++++++++++++++++++++++++++++-----------------
 1 file changed, 187 insertions(+), 113 deletions(-)

-- 
2.31.1

[PATCH 01/10] block/badblocks: only set bb->changed when badblocks changes

[linan666]

From: Li Nan <linan122@huawei.com>

bb->changed and unacked_exist is set and badblocks_update_acked() is
involked even if no badblocks changes in badblocks_set(). Only update
them when badblocks changes.

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 1b4caa42c5f1..7e6ebe2ac12c 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -166,6 +166,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	int lo, hi;
 	int rv = 0;
 	unsigned long flags;
+	bool changed = false;
 
 	if (bb->shift < 0)
 		/* badblocks are disabled */
@@ -229,6 +230,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 				s = a + BB_MAX_LEN;
 			}
 			sectors = e - s;
+			changed = true;
 		}
 	}
 	if (sectors && hi < bb->count) {
@@ -259,6 +261,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			sectors = e - s;
 			lo = hi;
 			hi++;
+			changed = true;
 		}
 	}
 	if (sectors == 0 && hi < bb->count) {
@@ -277,6 +280,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			memmove(p + hi, p + hi + 1,
 				(bb->count - hi - 1) * 8);
 			bb->count--;
+			changed = true;
 		}
 	}
 	while (sectors) {
@@ -299,14 +303,17 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			p[hi] = BB_MAKE(s, this_sectors, acknowledged);
 			sectors -= this_sectors;
 			s += this_sectors;
+			changed = true;
 		}
 	}
 
-	bb->changed = true;
-	if (!acknowledged)
-		bb->unacked_exist = true;
-	else
-		badblocks_update_acked(bb);
+	if (changed) {
+		bb->changed = changed;
+		if (!acknowledged)
+			bb->unacked_exist = true;
+		else
+			badblocks_update_acked(bb);
+	}
 	write_sequnlock_irqrestore(&bb->lock, flags);
 
 	return rv;
-- 
2.31.1

[PATCH 02/10] block/badblocks: fix badblocks loss when badblocks combine

[linan666]

From: Li Nan <linan122@huawei.com>

badblocks will loss if we set it as below:

  # echo 1 1 > bad_blocks
  # echo 3 1 > bad_blocks
  # echo 1 5 > bad_blocks
  # cat bad_blocks
    1 3

In badblocks_set(), if there is an intersection between p[lo] and p[hi],
we will combine them. The end of new badblocks is p[hi]'s end now. but
p[lo] may cross p[hi] and new end should be the larger of p[lo] and p[hi].

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 7e6ebe2ac12c..2c2ef8284a3f 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -267,16 +267,14 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	if (sectors == 0 && hi < bb->count) {
 		/* we might be able to combine lo and hi */
 		/* Note: 's' is at the end of 'lo' */
-		sector_t a = BB_OFFSET(p[hi]);
-		int lolen = BB_LEN(p[lo]);
-		int hilen = BB_LEN(p[hi]);
-		int newlen = lolen + hilen - (s - a);
+		sector_t a = BB_OFFSET(p[lo]);
+		int newlen = max(s, BB_OFFSET(p[hi]) + BB_LEN(p[hi])) - a;
 
-		if (s >= a && newlen < BB_MAX_LEN) {
+		if (s >= BB_OFFSET(p[hi]) && newlen < BB_MAX_LEN) {
 			/* yes, we can combine them */
 			int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
 
-			p[lo] = BB_MAKE(BB_OFFSET(p[lo]), newlen, ack);
+			p[lo] = BB_MAKE(a, newlen, ack);
 			memmove(p + hi, p + hi + 1,
 				(bb->count - hi - 1) * 8);
 			bb->count--;
-- 
2.31.1

[PATCH 03/10] block/badblocks: fix badblocks overlap

[linan666]

From: Li Nan <linan122@huawei.com>

badblocks will overlap if we set it as below:

  # echo 0 1 > bad_blocks
  # echo 3 1 > bad_blocks
  # echo 5 1 > bad_blocks
  # echo 0 512 > bad_blocks
  # cat bad_blocks
    0 512
    5 1

The reason for this is we only combine two badblocks once even if it
should be combined many times.

Fix it by classifying combine:
  1)lo contain hi
      just remove hi, and check next hi.
  2)lo intersects with hi
      newlen > BB_MAX_LEN, set lo to MAX, and the remaining is hi.
      newlen <= BB_MAX_LEN, remain unchanged, 2-in-1.

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 2c2ef8284a3f..f34351b59414 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -245,6 +245,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			/* merging is possible */
 			if (e <= s + sectors) {
 				/* full overlap */
+
 				e = s + sectors;
 				ack = acknowledged;
 			} else
@@ -267,17 +268,35 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	if (sectors == 0 && hi < bb->count) {
 		/* we might be able to combine lo and hi */
 		/* Note: 's' is at the end of 'lo' */
-		sector_t a = BB_OFFSET(p[lo]);
-		int newlen = max(s, BB_OFFSET(p[hi]) + BB_LEN(p[hi])) - a;
-
-		if (s >= BB_OFFSET(p[hi]) && newlen < BB_MAX_LEN) {
-			/* yes, we can combine them */
-			int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
-
-			p[lo] = BB_MAKE(a, newlen, ack);
-			memmove(p + hi, p + hi + 1,
-				(bb->count - hi - 1) * 8);
-			bb->count--;
+		sector_t loa = BB_OFFSET(p[lo]), hia = BB_OFFSET(p[hi]);
+		sector_t hie = hia + BB_LEN(p[hi]);
+		int newlen = max(s, hie) - loa;
+		int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
+
+		if (s >= hia) {
+			while (s >= hie) {
+				/* lo contains hi, just remove hi */
+				memmove(p + hi, p + hi + 1,
+					(bb->count - hi - 1) * 8);
+				bb->count--;
+				if (hi >= bb->count)
+					break;
+				hia = BB_OFFSET(p[hi]);
+				hie = hia + BB_LEN(p[hi]);
+			}
+			if (s >= hia && hi < bb->count) {
+				if (newlen > BB_MAX_LEN) {
+					p[lo] = BB_MAKE(loa, BB_MAX_LEN, ack);
+					p[hi] = BB_MAKE(loa + BB_MAX_LEN,
+							newlen - BB_MAX_LEN,
+							BB_ACK(p[hi]));
+				} else {
+					p[lo] = BB_MAKE(loa, newlen, ack);
+					memmove(p + hi, p + hi + 1,
+						(bb->count - hi - 1) * 8);
+					bb->count--;
+				}
+			}
 			changed = true;
 		}
 	}
-- 
2.31.1

[PATCH 04/10] block/badblocks: fix the bug of reverse order

[linan666]

From: Li Nan <linan122@huawei.com>

Order of badblocks will be reversed if we set a large area at once. 'hi'
remains unchanged while adding continuous badblocks is wrong, the next
setting is greater than 'hi', it should be added to the next position.
Let 'hi' +1 each cycle.

  # echo 0 2048 > bad_blocks
  # cat bad_blocks
    1536 512
    1024 512
    512 512
    0 512

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/badblocks.c b/block/badblocks.c
index f34351b59414..11e3a3ae2c72 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -320,6 +320,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			p[hi] = BB_MAKE(s, this_sectors, acknowledged);
 			sectors -= this_sectors;
 			s += this_sectors;
+			hi++;
 			changed = true;
 		}
 	}
-- 
2.31.1

[PATCH 05/10] block/badblocks: fix ack set fail in badblocks_set

[linan666]

From: Li Nan <linan122@huawei.com>

If we try to set ack for a BB_MAX_LEN badblocks, it will return
0(success) but did not set ack at all in badblocks_set(). Check ack
before setting to fix it, and do not set badblocks already exist.

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 11e3a3ae2c72..c11eb869f2f3 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -219,18 +219,18 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			if (e < s + sectors)
 				e = s + sectors;
 			if (e - a <= BB_MAX_LEN) {
-				p[lo] = BB_MAKE(a, e-a, ack);
 				s = e;
 			} else {
 				/* does not all fit in one range,
 				 * make p[lo] maximal
 				 */
-				if (BB_LEN(p[lo]) != BB_MAX_LEN)
-					p[lo] = BB_MAKE(a, BB_MAX_LEN, ack);
 				s = a + BB_MAX_LEN;
 			}
+			if (s - a != BB_LEN(p[lo]) || ack != BB_ACK(p[lo])) {
+				p[lo] = BB_MAKE(a, s - a, ack);
+				changed = true;
+			}
 			sectors = e - s;
-			changed = true;
 		}
 	}
 	if (sectors && hi < bb->count) {
-- 
2.31.1

[PATCH 06/10] block/badblocks: check bb->count instead of 'hi > lo'

[linan666]

From: Li Nan <linan122@huawei.com>

'hi > lo' only holds when bb->count is 0. Check it is complicated after
dichotomy. Check bb->count instead. And this will make future fix more
convenient.

No functional change intended.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 155 +++++++++++++++++++++++-----------------------
 1 file changed, 79 insertions(+), 76 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index c11eb869f2f3..3cb8513cbd7f 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -187,28 +187,32 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	p = bb->page;
 	lo = 0;
 	hi = bb->count;
-	/* Find the last range that starts at-or-before 's' */
-	while (hi - lo > 1) {
-		int mid = (lo + hi) / 2;
-		sector_t a = BB_OFFSET(p[mid]);
-
-		if (a <= s)
-			lo = mid;
-		else
-			hi = mid;
-	}
-	if (hi > lo && BB_OFFSET(p[lo]) > s)
-		hi = lo;
+	if (bb->count) {
+		sector_t a;
+		sector_t e;
+		int ack;
+
+		/* Find the last range that starts at-or-before 's' */
+		while (hi - lo > 1) {
+			int mid = (lo + hi) / 2;
+
+			a = BB_OFFSET(p[mid]);
+			if (a <= s)
+				lo = mid;
+			else
+				hi = mid;
+		}
 
-	if (hi > lo) {
 		/* we found a range that might merge with the start
 		 * of our new range
 		 */
-		sector_t a = BB_OFFSET(p[lo]);
-		sector_t e = a + BB_LEN(p[lo]);
-		int ack = BB_ACK(p[lo]);
+		a = BB_OFFSET(p[lo]);
+		e = a + BB_LEN(p[lo]);
+		ack = BB_ACK(p[lo]);
 
-		if (e >= s) {
+		if (a > s) {
+			hi = lo;
+		} else if (e >= s) {
 			/* Yes, we can merge with a previous range */
 			if (s == a && s + sectors >= e)
 				/* new range covers old */
@@ -232,72 +236,71 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			}
 			sectors = e - s;
 		}
-	}
-	if (sectors && hi < bb->count) {
-		/* 'hi' points to the first range that starts after 's'.
-		 * Maybe we can merge with the start of that range
-		 */
-		sector_t a = BB_OFFSET(p[hi]);
-		sector_t e = a + BB_LEN(p[hi]);
-		int ack = BB_ACK(p[hi]);
-
-		if (a <= s + sectors) {
-			/* merging is possible */
-			if (e <= s + sectors) {
-				/* full overlap */
-
-				e = s + sectors;
-				ack = acknowledged;
-			} else
-				ack = ack && acknowledged;
-
-			a = s;
-			if (e - a <= BB_MAX_LEN) {
-				p[hi] = BB_MAKE(a, e-a, ack);
-				s = e;
-			} else {
-				p[hi] = BB_MAKE(a, BB_MAX_LEN, ack);
-				s = a + BB_MAX_LEN;
+		if (sectors && hi < bb->count) {
+			/* 'hi' points to the first range that starts after 's'.
+			 * Maybe we can merge with the start of that range
+			 */
+			a = BB_OFFSET(p[hi]);
+			e = a + BB_LEN(p[hi]);
+			ack = BB_ACK(p[hi]);
+
+			if (a <= s + sectors) {
+				/* merging is possible */
+				if (e <= s + sectors) {
+					/* full overlap */
+					e = s + sectors;
+					ack = acknowledged;
+				} else
+					ack = ack && acknowledged;
+
+				a = s;
+				if (e - a <= BB_MAX_LEN) {
+					p[hi] = BB_MAKE(a, e-a, ack);
+					s = e;
+				} else {
+					p[hi] = BB_MAKE(a, BB_MAX_LEN, ack);
+					s = a + BB_MAX_LEN;
+				}
+				sectors = e - s;
+				lo = hi;
+				hi++;
+				changed = true;
 			}
-			sectors = e - s;
-			lo = hi;
-			hi++;
-			changed = true;
 		}
-	}
-	if (sectors == 0 && hi < bb->count) {
-		/* we might be able to combine lo and hi */
-		/* Note: 's' is at the end of 'lo' */
-		sector_t loa = BB_OFFSET(p[lo]), hia = BB_OFFSET(p[hi]);
-		sector_t hie = hia + BB_LEN(p[hi]);
-		int newlen = max(s, hie) - loa;
-		int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
-
-		if (s >= hia) {
-			while (s >= hie) {
-				/* lo contains hi, just remove hi */
-				memmove(p + hi, p + hi + 1,
-					(bb->count - hi - 1) * 8);
-				bb->count--;
-				if (hi >= bb->count)
-					break;
-				hia = BB_OFFSET(p[hi]);
-				hie = hia + BB_LEN(p[hi]);
-			}
-			if (s >= hia && hi < bb->count) {
-				if (newlen > BB_MAX_LEN) {
-					p[lo] = BB_MAKE(loa, BB_MAX_LEN, ack);
-					p[hi] = BB_MAKE(loa + BB_MAX_LEN,
-							newlen - BB_MAX_LEN,
-							BB_ACK(p[hi]));
-				} else {
-					p[lo] = BB_MAKE(loa, newlen, ack);
+		if (sectors == 0 && hi < bb->count) {
+			/* we might be able to combine lo and hi */
+			/* Note: 's' is at the end of 'lo' */
+			sector_t loa = BB_OFFSET(p[lo]), hia = BB_OFFSET(p[hi]);
+			sector_t hie = hia + BB_LEN(p[hi]);
+			int newlen = max(s, hie) - loa;
+
+			ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
+			if (s >= hia) {
+				while (s >= hie) {
+					/* lo contains hi, just remove hi */
 					memmove(p + hi, p + hi + 1,
 						(bb->count - hi - 1) * 8);
 					bb->count--;
+					if (hi >= bb->count)
+						break;
+					hia = BB_OFFSET(p[hi]);
+					hie = hia + BB_LEN(p[hi]);
+				}
+				if (s >= hia && hi < bb->count) {
+					if (newlen > BB_MAX_LEN) {
+						p[lo] = BB_MAKE(loa, BB_MAX_LEN, ack);
+						p[hi] = BB_MAKE(loa + BB_MAX_LEN,
+								newlen - BB_MAX_LEN,
+								BB_ACK(p[hi]));
+					} else {
+						p[lo] = BB_MAKE(loa, newlen, ack);
+						memmove(p + hi, p + hi + 1,
+							(bb->count - hi - 1) * 8);
+						bb->count--;
+					}
 				}
+				changed = true;
 			}
-			changed = true;
 		}
 	}
 	while (sectors) {
-- 
2.31.1

[PATCH 07/10] block/badblocks: factor out a helper to merge badblocks

[linan666]

From: Li Nan <linan122@huawei.com>

Add a helper badblocks_merge() to merge badblocks, it makes code more
readable. No functional change.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 143 +++++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 64 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 3cb8513cbd7f..f498fae201a1 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -144,6 +144,80 @@ static void badblocks_update_acked(struct badblocks *bb)
 		bb->unacked_exist = false;
 }
 
+/*
+ * try to merge new range with lo and hi.
+ * if s is within lo, merge with lo.
+ * if s + sectors > start of hi, merge with hi.
+ *
+ * Return:
+ *  Number of merged sectors
+ */
+static int badblocks_merge(struct badblocks *bb, sector_t s, int sectors,
+			int acknowledged, int *lo, int *hi, bool *changed)
+{
+	u64 *p = bb->page;
+	sector_t a = BB_OFFSET(p[*lo]);
+	sector_t e = a + BB_LEN(p[*lo]);
+	int merged_sectors = 0, ack = acknowledged;
+
+	if (a > s) {
+		*hi = *lo;
+	} else if (e >= s) {
+		/* we can merge with a previous range */
+		if (s > a || s + sectors < e)
+			ack = ack && BB_ACK(p[*lo]);
+
+		if (e < s + sectors)
+			e = s + sectors;
+		if (e - a <= BB_MAX_LEN) {
+			s = e;
+		} else {
+			/*
+			 * does not all fit in one range,
+			 * make p[lo] maximal
+			 */
+			s = a + BB_MAX_LEN;
+		}
+		if (s - a != BB_LEN(p[*lo]) || ack != BB_ACK(p[*lo])) {
+			p[*lo] = BB_MAKE(a, s - a, ack);
+			*changed = true;
+		}
+		merged_sectors += sectors - e + s;
+		sectors = e - s;
+	}
+	if (sectors && *hi < bb->count) {
+		/*
+		 * 'hi' points to the first range that starts after 's'.
+		 * Maybe we can merge with the start of that range
+		 */
+		a = BB_OFFSET(p[*hi]);
+		e = a + BB_LEN(p[*hi]);
+		ack = acknowledged;
+
+		if (a <= s + sectors) {
+			/* merging is possible */
+			if (e <= s + sectors) {
+				/* full overlap */
+				e = s + sectors;
+			} else
+				ack = ack && BB_ACK(p[*hi]);
+
+			a = s;
+			if (e - a <= BB_MAX_LEN)
+				s = e;
+			else
+				s = a + BB_MAX_LEN;
+			p[*hi] = BB_MAKE(a, s-a, ack);
+
+			merged_sectors += sectors - e + s;
+			*changed = true;
+			*lo = *hi;
+			*hi += 1;
+		}
+	}
+	return merged_sectors;
+}
+
 /**
  * badblocks_set() - Add a range of bad blocks to the table.
  * @bb:		the badblocks structure that holds all badblock information
@@ -191,6 +265,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		sector_t a;
 		sector_t e;
 		int ack;
+		int merged_sectors;
 
 		/* Find the last range that starts at-or-before 's' */
 		while (hi - lo > 1) {
@@ -203,70 +278,10 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 				hi = mid;
 		}
 
-		/* we found a range that might merge with the start
-		 * of our new range
-		 */
-		a = BB_OFFSET(p[lo]);
-		e = a + BB_LEN(p[lo]);
-		ack = BB_ACK(p[lo]);
-
-		if (a > s) {
-			hi = lo;
-		} else if (e >= s) {
-			/* Yes, we can merge with a previous range */
-			if (s == a && s + sectors >= e)
-				/* new range covers old */
-				ack = acknowledged;
-			else
-				ack = ack && acknowledged;
-
-			if (e < s + sectors)
-				e = s + sectors;
-			if (e - a <= BB_MAX_LEN) {
-				s = e;
-			} else {
-				/* does not all fit in one range,
-				 * make p[lo] maximal
-				 */
-				s = a + BB_MAX_LEN;
-			}
-			if (s - a != BB_LEN(p[lo]) || ack != BB_ACK(p[lo])) {
-				p[lo] = BB_MAKE(a, s - a, ack);
-				changed = true;
-			}
-			sectors = e - s;
-		}
-		if (sectors && hi < bb->count) {
-			/* 'hi' points to the first range that starts after 's'.
-			 * Maybe we can merge with the start of that range
-			 */
-			a = BB_OFFSET(p[hi]);
-			e = a + BB_LEN(p[hi]);
-			ack = BB_ACK(p[hi]);
-
-			if (a <= s + sectors) {
-				/* merging is possible */
-				if (e <= s + sectors) {
-					/* full overlap */
-					e = s + sectors;
-					ack = acknowledged;
-				} else
-					ack = ack && acknowledged;
-
-				a = s;
-				if (e - a <= BB_MAX_LEN) {
-					p[hi] = BB_MAKE(a, e-a, ack);
-					s = e;
-				} else {
-					p[hi] = BB_MAKE(a, BB_MAX_LEN, ack);
-					s = a + BB_MAX_LEN;
-				}
-				sectors = e - s;
-				lo = hi;
-				hi++;
-				changed = true;
-			}
-		}
+		merged_sectors = badblocks_merge(bb, s, sectors, acknowledged,
+						 &lo, &hi, &changed);
+		s += merged_sectors;
+		sectors -= merged_sectors;
 		if (sectors == 0 && hi < bb->count) {
 			/* we might be able to combine lo and hi */
 			/* Note: 's' is at the end of 'lo' */
-- 
2.31.1

[PATCH 08/10] block/badblocks: factor out a helper to combine badblocks

[linan666]

From: Li Nan <linan122@huawei.com>

Add a helper badblocks_combine() to combine badblocks, it makes code more
readable. No functional change.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 87 ++++++++++++++++++++++++++---------------------
 1 file changed, 48 insertions(+), 39 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index f498fae201a1..c87c68d4bcac 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -218,6 +218,51 @@ static int badblocks_merge(struct badblocks *bb, sector_t s, int sectors,
 	return merged_sectors;
 }
 
+/*
+ * try to combine lo and hi(lo + 1) if lo intersects with hi
+ */
+static void badblocks_combine(struct badblocks *bb, int lo)
+{
+	u64 *p = bb->page;
+	sector_t loe = BB_OFFSET(p[lo]) + BB_LEN(p[lo]);
+	int hi = lo + 1;
+
+	if (hi >= bb->count)
+		return;
+	/* we might be able to combine lo and hi */
+
+	if (loe >= BB_OFFSET(p[hi])) {
+		sector_t loa = BB_OFFSET(p[lo]), hia = BB_OFFSET(p[hi]);
+		sector_t hie = hia + BB_LEN(p[hi]);
+		int newlen = max(loe, hie) - loa;
+		int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
+
+		while (loe >= hie) {
+			/* lo contains hi, just remove hi */
+			memmove(p + hi, p + hi + 1,
+				(bb->count - hi - 1) * 8);
+			bb->count--;
+			if (hi >= bb->count)
+				break;
+			hia = BB_OFFSET(p[hi]);
+			hie = hia + BB_LEN(p[hi]);
+		}
+		if (loe >= hia && hi < bb->count) {
+			if (newlen > BB_MAX_LEN) {
+				p[lo] = BB_MAKE(loa, BB_MAX_LEN, ack);
+				p[hi] = BB_MAKE(loa + BB_MAX_LEN,
+						newlen - BB_MAX_LEN,
+						BB_ACK(p[hi]));
+			} else {
+				p[lo] = BB_MAKE(loa, newlen, ack);
+				memmove(p + hi, p + hi + 1,
+					(bb->count - hi - 1) * 8);
+				bb->count--;
+			}
+		}
+	}
+}
+
 /**
  * badblocks_set() - Add a range of bad blocks to the table.
  * @bb:		the badblocks structure that holds all badblock information
@@ -262,16 +307,13 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	lo = 0;
 	hi = bb->count;
 	if (bb->count) {
-		sector_t a;
-		sector_t e;
-		int ack;
 		int merged_sectors;
 
 		/* Find the last range that starts at-or-before 's' */
 		while (hi - lo > 1) {
 			int mid = (lo + hi) / 2;
+			int a = BB_OFFSET(p[mid]);
 
-			a = BB_OFFSET(p[mid]);
 			if (a <= s)
 				lo = mid;
 			else
@@ -282,41 +324,8 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 						 &lo, &hi, &changed);
 		s += merged_sectors;
 		sectors -= merged_sectors;
-		if (sectors == 0 && hi < bb->count) {
-			/* we might be able to combine lo and hi */
-			/* Note: 's' is at the end of 'lo' */
-			sector_t loa = BB_OFFSET(p[lo]), hia = BB_OFFSET(p[hi]);
-			sector_t hie = hia + BB_LEN(p[hi]);
-			int newlen = max(s, hie) - loa;
-
-			ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
-			if (s >= hia) {
-				while (s >= hie) {
-					/* lo contains hi, just remove hi */
-					memmove(p + hi, p + hi + 1,
-						(bb->count - hi - 1) * 8);
-					bb->count--;
-					if (hi >= bb->count)
-						break;
-					hia = BB_OFFSET(p[hi]);
-					hie = hia + BB_LEN(p[hi]);
-				}
-				if (s >= hia && hi < bb->count) {
-					if (newlen > BB_MAX_LEN) {
-						p[lo] = BB_MAKE(loa, BB_MAX_LEN, ack);
-						p[hi] = BB_MAKE(loa + BB_MAX_LEN,
-								newlen - BB_MAX_LEN,
-								BB_ACK(p[hi]));
-					} else {
-						p[lo] = BB_MAKE(loa, newlen, ack);
-						memmove(p + hi, p + hi + 1,
-							(bb->count - hi - 1) * 8);
-						bb->count--;
-					}
-				}
-				changed = true;
-			}
-		}
+		if (sectors == 0)
+			badblocks_combine(bb, lo);
 	}
 	while (sectors) {
 		/* didn't merge (it all).
-- 
2.31.1

[PATCH 09/10] block/badblocks: factor out a helper to create badblocks

[linan666]

From: Li Nan <linan122@huawei.com>

Add a helper badblocks_create() to create badblocks, it makes code more
readable. No functional change.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 65 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index c87c68d4bcac..bb0324b66f57 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -263,6 +263,46 @@ static void badblocks_combine(struct badblocks *bb, int lo)
 	}
 }
 
+/*
+ * creat new badblocks if it can't merge with existing region
+ *
+ * Return:
+ *  0: success
+ *  1: failed to set badblocks (out of space)
+ */
+static int badblocks_create(struct badblocks *bb, sector_t s, sector_t sectors,
+			int hi, int acknowledged, bool *changed)
+{
+	u64 *p = bb->page;
+	int rv = 0;
+
+	while (sectors) {
+		int this_sectors = sectors;
+
+		/* didn't merge (it all).
+		 * Need to add a range just before 'hi'
+		 */
+		if (bb->count >= MAX_BADBLOCKS) {
+			/* No room for more */
+			rv = 1;
+			break;
+		}
+
+		memmove(p + hi + 1, p + hi,
+			(bb->count - hi) * 8);
+		bb->count++;
+
+		if (this_sectors > BB_MAX_LEN)
+			this_sectors = BB_MAX_LEN;
+		p[hi] = BB_MAKE(s, this_sectors, acknowledged);
+		sectors -= this_sectors;
+		s += this_sectors;
+		hi++;
+		*changed = true;
+	}
+	return rv;
+}
+
 /**
  * badblocks_set() - Add a range of bad blocks to the table.
  * @bb:		the badblocks structure that holds all badblock information
@@ -327,30 +367,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		if (sectors == 0)
 			badblocks_combine(bb, lo);
 	}
-	while (sectors) {
-		/* didn't merge (it all).
-		 * Need to add a range just before 'hi'
-		 */
-		if (bb->count >= MAX_BADBLOCKS) {
-			/* No room for more */
-			rv = 1;
-			break;
-		} else {
-			int this_sectors = sectors;
-
-			memmove(p + hi + 1, p + hi,
-				(bb->count - hi) * 8);
-			bb->count++;
-
-			if (this_sectors > BB_MAX_LEN)
-				this_sectors = BB_MAX_LEN;
-			p[hi] = BB_MAKE(s, this_sectors, acknowledged);
-			sectors -= this_sectors;
-			s += this_sectors;
-			hi++;
-			changed = true;
-		}
-	}
+	rv = badblocks_create(bb, s, sectors, hi, acknowledged, &changed);
 
 	if (changed) {
 		bb->changed = changed;
-- 
2.31.1

[PATCH 10/10] block/badblocks: try to merge badblocks as much as possible

[linan666]

From: Li Nan <linan122@huawei.com>

If we set a new badblocks, we first merge it with existing region, then
try to combine lo and hi. If there are still badblocks need to be set,
create it. It is a bad way when setting a laget number of badblocks. for
example, it will become chaotic if we set as below:

  # echo 1 1 > bad_blocks
  # echo 512 1 > bad_blocks
  # echo 0 513 > bad_blocks
  # cat bad_blocks
    0 512
    512 1
    512 1

Fix it by trying to merge as much as possible. If we have merged any
badblocks, retry to merge next sectors. Do not check sectors while
combining, we should combine lo and hi each sycle.

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index bb0324b66f57..7e6fce10c82d 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -347,8 +347,6 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	lo = 0;
 	hi = bb->count;
 	if (bb->count) {
-		int merged_sectors;
-
 		/* Find the last range that starts at-or-before 's' */
 		while (hi - lo > 1) {
 			int mid = (lo + hi) / 2;
@@ -360,12 +358,19 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 				hi = mid;
 		}
 
-		merged_sectors = badblocks_merge(bb, s, sectors, acknowledged,
-						 &lo, &hi, &changed);
-		s += merged_sectors;
-		sectors -= merged_sectors;
-		if (sectors == 0)
+		while (sectors) {
+			int merged_sectors;
+
+			merged_sectors = badblocks_merge(bb, s, sectors, acknowledged,
+							 &lo, &hi, &changed);
+			/* can't merge, break to create */
+			if (!merged_sectors)
+				break;
+
+			s += merged_sectors;
+			sectors -= merged_sectors;
 			badblocks_combine(bb, lo);
+		}
 	}
 	rv = badblocks_create(bb, s, sectors, hi, acknowledged, &changed);
 
-- 
2.31.1