On Thu, Oct 6, 2016 at 9:50 PM, NeilBrown [off-list ref] wrote:

On Tue, Sep 06 2016, Tomasz Majchrzak wrote:

quoted

Current bad block clear implementation assumes the range to clear
overlaps with at least one bad block already stored. If given range to
clear precedes first bad block in a list, the first entry is incorrectly
updated.

In the original md context, it would only ever be called on a block that
was already in the list.
But you are right that it is best not to assume this, and to code more
safely.

quoted

Check not only if stored block end is past clear block end but also if
stored block start is before clear block end.

Signed-off-by: Tomasz Majchrzak <redacted>

Dan Williams seems to have taken responsibility for this code through
his nvdimm tree, so I've added him to 'cc' in the hope that he looks at
this (I wonder if he is even on linux-block ....)

I am, but I missed this, thanks for the cc.

quoted

---
 block/badblocks.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 7be53cb..b2ffcc7 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c

@@ -354,7 +354,8 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
               * current range.  Earlier ranges could also overlap,
               * but only this one can overlap the end of the range.
               */
-             if (BB_OFFSET(p[lo]) + BB_LEN(p[lo]) > target) {
+             if ((BB_OFFSET(p[lo]) + BB_LEN(p[lo]) > target) &&
+                 (BB_OFFSET(p[lo]) <= target)) {

hmmm..
'target' is the sector just beyond the set of sectors to remove from the
list.
BB_OFFSET(p[lo]) is the first sector in a range that was found in the
list.
If these are equal, then are aren't clearing anything in this range.
So I would have '<', not '<='.

I don't think this makes the code wrong as we end up assigning to p[lo]
the value that is already there.  But it might be confusing.

quoted

                      /* Partial overlap, leave the tail of this range */
                      int ack = BB_ACK(p[lo]);
                      sector_t a = BB_OFFSET(p[lo]);

@@ -377,7 +378,8 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
                      lo--;
              }
              while (lo >= 0 &&
-                    BB_OFFSET(p[lo]) + BB_LEN(p[lo]) > s) {
+                    (BB_OFFSET(p[lo]) + BB_LEN(p[lo]) > s) &&
+                    (BB_OFFSET(p[lo]) <= target)) {

Ditto.

But the code is, I think, correct. Just not how I would have written it.
So

 Acked-by: NeilBrown [off-list ref]

I agree with the comments to change "<=" to "<".  Tomasz, care to
re-send with those changes?