Fix bitmap offset calculations

PATCH Applied Goldwyn Rodrigues 3 participants 4 messages Mar 24, 2015 – Apr 1, 2015 MD RAID

lore

Share & tools

lore

Goldwyn Rodrigues

author

2015-03-24 16:29 UTC #0 lore raw

PATCH 1/1 Fix bitmap offset calculations

The calculations of bitmap offset is incorrect with respect to bits to bytes conversion.

Also, remove an irrelevant duplicate message.

Signed-off-by Goldwyn Rodrigues <[email protected]>

---

drivers/md/bitmap.c +3 -4

index ac79fef..e98db04 100644
@@ -575,7 +575,9 @@ re_read:
 
 		sector_div(bm_blocks,
 			   bitmap->mddev->bitmap_info.chunksize >> 9);
-		bm_blocks = bm_blocks << 3;
+		/* bits to bytes */
+		bm_blocks = ((bm_blocks+7) >> 3) + sizeof(bitmap_super_t);
+		/* to 4k blocks */
 		bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
 		bitmap->mddev->bitmap_info.offset += bitmap->cluster_slot * (bm_blocks << 3);
 		pr_info("%s:%d bm slot: %d offset: %llu\n", __func__, __LINE__,

@@ -672,9 +674,6 @@ out:
 			goto out_no_sb;
 		}
 		bitmap->cluster_slot = md_cluster_ops->slot_number(bitmap->mddev);
-		pr_info("%s:%d bm slot: %d offset: %llu\n", __func__, __LINE__,
-			bitmap->cluster_slot,
-			(unsigned long long)bitmap->mddev->bitmap_info.offset);
 		goto re_read;
 	}

Does it means if some one use write-intent bitmap without this patch, he may end-up with some unsync stripes after system crash or power failure and RAID resynchronization ? It seems the bitmaps does not record correct address of unsynced blocks due to this bug.

Would you please verify this.

Quoted 38 lines · NeilBrown

Quoted 23 lines · On Tue, 24 Mar 2015 11:29:05 -0500 Goldwyn Rodrigues <[email protected]> wrote:

The calculations of bitmap offset is incorrect with respect to bits to bytes conversion.

Also, remove an irrelevant duplicate message.

Signed-off-by Goldwyn Rodrigues <[email protected]>

---

drivers/md/bitmap.c +3 -1

index ac79fef..e98db04 100644
@@ -575,7 +575,9 @@ re_read:
 
              sector_div(bm_blocks,
                         bitmap->mddev->bitmap_info.chunksize >> 9);
-             bm_blocks = bm_blocks << 3;
+             /* bits to bytes */
+             bm_blocks = ((bm_blocks+7) >> 3) + sizeof(bitmap_super_t);
+             /* to 4k blocks */
              bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
              bitmap->mddev->bitmap_info.offset += bitmap->cluster_slot * (bm_blocks << 3);
              pr_info("%s:%d bm slot: %d offset: %llu\n", __func__, __LINE__,

drivers/md/bitmap.c out: · line 679

Quoted 9 lines

drivers/md/bitmap.c +0 -3

@@ -672,9 +674,6 @@ out:
                      goto out_no_sb;
              }
              bitmap->cluster_slot = md_cluster_ops->slot_number(bitmap->mddev);
-             pr_info("%s:%d bm slot: %d offset: %llu\n", __func__, __LINE__,
-                     bitmap->cluster_slot,
-                     (unsigned long long)bitmap->mddev->bitmap_info.offset);
              goto re_read;
      }

Applied, thanks.

NeilBrown

Goldwyn Rodrigues

author

2015-04-01 14:14 UTC #3 lore raw

Hi Alireza,

Quoted 6 lines · Alireza Haghdoost

Does it means if some one use write-intent bitmap without this patch, he may end-up with some unsync stripes after system crash or power failure and RAID resynchronization ? It seems the bitmaps does not record correct address of unsynced blocks due to this bug.
Would you please verify this.

This is for clustered md effort only (which is in Neil's md/for-next tree). The regular md is unaffected.

In a clustered environment, different nodes use different bitmaps. While it worked for bitmaps smaller than a page (which is again a co-incidence), it did not work well for bitmaps which spanned multiple pages. Each node in the cluster has different start offsets, and the earlier calculation was incorrect because of conversion from bits to bytes was inverted.

If you were able to assemble on different nodes, you should be fine with respect to synchronization of unsynced blocks after a failure.

HTH,

Quoted 40 lines · NeilBrown

Quoted 38 lines · On Tue, 24 Mar 2015 11:29:05 -0500 Goldwyn Rodrigues <[email protected]> wrote:

Quoted 24 lines

The calculations of bitmap offset is incorrect with respect to bits to bytes conversion.

Also, remove an irrelevant duplicate message.

Signed-off-by Goldwyn Rodrigues <[email protected]>

---

drivers/md/bitmap.c +3 -1

index ac79fef..e98db04 100644
@@ -575,7 +575,9 @@ re_read:
 
               sector_div(bm_blocks,
                          bitmap->mddev->bitmap_info.chunksize >> 9);
-             bm_blocks = bm_blocks << 3;
+             /* bits to bytes */
+             bm_blocks = ((bm_blocks+7) >> 3) + sizeof(bitmap_super_t);
+             /* to 4k blocks */
               bm_blocks = DIV_ROUND_UP_SECTOR_T(bm_blocks, 4096);
               bitmap->mddev->bitmap_info.offset += bitmap->cluster_slot * (bm_blocks << 3);
               pr_info("%s:%d bm slot: %d offset: %llu\n", __func__, __LINE__,

drivers/md/bitmap.c out: · line 679

Quoted 9 lines

drivers/md/bitmap.c +0 -3

@@ -672,9 +674,6 @@ out:
                       goto out_no_sb;
               }
               bitmap->cluster_slot = md_cluster_ops->slot_number(bitmap->mddev);
-             pr_info("%s:%d bm slot: %d offset: %llu\n", __func__, __LINE__,
-                     bitmap->cluster_slot,
-                     (unsigned long long)bitmap->mddev->bitmap_info.offset);
               goto re_read;
       }

Applied, thanks.

NeilBrown

Signature · 1 line

Goldwyn