| Message ID | 20250302043905.95887-1-glass.su@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] md/md-bitmap: fix wrong bitmap_limit for clustermd when write sb | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_14-PR | success | PR summary |
| mdraidci/vmtest-md-6_14-VM_Test-0 | success | Logs for per-patch-testing |
Hi Glass, The code looks good to me. The commit log needs to be revised. On 3/2/25 12:39, Su Yue wrote: > In clustermd, Separate write-intent-bitmaps are used for each cluster > node: Change 'Separate' to lowercase 'separate'. > > 0 4k 8k 12k > ------------------------------------------------------------------- > | idle | md super | bm super [0] + bits | > | bm bits[0, contd] | bm super[1] + bits | bm bits[1, contd] | > | bm super[2] + bits | bm bits [2, contd] | bm super[3] + bits | > | bm bits [3, contd] | | | > > So in node 1, pg_index in __write_sb_page() could equal to > bitmap->storage.file_pages. Then bitmap_limit will be calculated to > 0. md_super_write() will be called with 0 size. > That means node the first 4k sb area of node 1 will never be updated > through filemap_write_page(). "That means node the ...", should remove the word 'node'? - Heming > This bug causes hang of mdadm/clustermd_tests/01r1_Grow_resize. > > Here use (pg_index % bitmap->storage.file_pages) to calculation of > bitmap_limit correct. > > Fixes: ab99a87542f1 ("md/md-bitmap: fix writing non bitmap pages") > Signed-off-by: Su Yue <glass.su@suse.com> > --- > drivers/md/md-bitmap.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c > index 23c09d22fcdb..9ae6cc8e30cb 100644 > --- a/drivers/md/md-bitmap.c > +++ b/drivers/md/md-bitmap.c > @@ -426,8 +426,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap, > struct block_device *bdev; > struct mddev *mddev = bitmap->mddev; > struct bitmap_storage *store = &bitmap->storage; > - unsigned int bitmap_limit = (bitmap->storage.file_pages - pg_index) << > - PAGE_SHIFT; > + unsigned long num_pages = bitmap->storage.file_pages; > + unsigned int bitmap_limit = (num_pages - pg_index % num_pages) << PAGE_SHIFT; > loff_t sboff, offset = mddev->bitmap_info.offset; > sector_t ps = pg_index * PAGE_SIZE / SECTOR_SIZE; > unsigned int size = PAGE_SIZE; > @@ -436,7 +436,7 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap, > > bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev; > /* we compare length (page numbers), not page offset. */ > - if ((pg_index - store->sb_index) == store->file_pages - 1) { > + if ((pg_index - store->sb_index) == num_pages - 1) { > unsigned int last_page_size = store->bytes & (PAGE_SIZE - 1); > > if (last_page_size == 0)
> On Mar 3, 2025, at 11:08, Heming Zhao <heming.zhao@suse.com> wrote: > > Hi Glass, > > The code looks good to me. > The commit log needs to be revised. > > On 3/2/25 12:39, Su Yue wrote: >> In clustermd, Separate write-intent-bitmaps are used for each cluster >> node: > > Change 'Separate' to lowercase 'separate'. >> 0 4k 8k 12k >> ------------------------------------------------------------------- >> | idle | md super | bm super [0] + bits | >> | bm bits[0, contd] | bm super[1] + bits | bm bits[1, contd] | >> | bm super[2] + bits | bm bits [2, contd] | bm super[3] + bits | >> | bm bits [3, contd] | | | >> So in node 1, pg_index in __write_sb_page() could equal to >> bitmap->storage.file_pages. Then bitmap_limit will be calculated to >> 0. md_super_write() will be called with 0 size. >> That means node the first 4k sb area of node 1 will never be updated >> through filemap_write_page(). > > "That means node the ...", should remove the word 'node'? Thanks. Fixed in v3. — Su > > - Heming > >> This bug causes hang of mdadm/clustermd_tests/01r1_Grow_resize. >> Here use (pg_index % bitmap->storage.file_pages) to calculation of >> bitmap_limit correct. >> Fixes: ab99a87542f1 ("md/md-bitmap: fix writing non bitmap pages") >> Signed-off-by: Su Yue <glass.su@suse.com> >> --- >> drivers/md/md-bitmap.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c >> index 23c09d22fcdb..9ae6cc8e30cb 100644 >> --- a/drivers/md/md-bitmap.c >> +++ b/drivers/md/md-bitmap.c >> @@ -426,8 +426,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap, >> struct block_device *bdev; >> struct mddev *mddev = bitmap->mddev; >> struct bitmap_storage *store = &bitmap->storage; >> - unsigned int bitmap_limit = (bitmap->storage.file_pages - pg_index) << >> - PAGE_SHIFT; >> + unsigned long num_pages = bitmap->storage.file_pages; >> + unsigned int bitmap_limit = (num_pages - pg_index % num_pages) << PAGE_SHIFT; >> loff_t sboff, offset = mddev->bitmap_info.offset; >> sector_t ps = pg_index * PAGE_SIZE / SECTOR_SIZE; >> unsigned int size = PAGE_SIZE; >> @@ -436,7 +436,7 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap, >> bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev; >> /* we compare length (page numbers), not page offset. */ >> - if ((pg_index - store->sb_index) == store->file_pages - 1) { >> + if ((pg_index - store->sb_index) == num_pages - 1) { >> unsigned int last_page_size = store->bytes & (PAGE_SIZE - 1); >> if (last_page_size == 0) >
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index 23c09d22fcdb..9ae6cc8e30cb 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c @@ -426,8 +426,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap, struct block_device *bdev; struct mddev *mddev = bitmap->mddev; struct bitmap_storage *store = &bitmap->storage; - unsigned int bitmap_limit = (bitmap->storage.file_pages - pg_index) << - PAGE_SHIFT; + unsigned long num_pages = bitmap->storage.file_pages; + unsigned int bitmap_limit = (num_pages - pg_index % num_pages) << PAGE_SHIFT; loff_t sboff, offset = mddev->bitmap_info.offset; sector_t ps = pg_index * PAGE_SIZE / SECTOR_SIZE; unsigned int size = PAGE_SIZE; @@ -436,7 +436,7 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap, bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev; /* we compare length (page numbers), not page offset. */ - if ((pg_index - store->sb_index) == store->file_pages - 1) { + if ((pg_index - store->sb_index) == num_pages - 1) { unsigned int last_page_size = store->bytes & (PAGE_SIZE - 1); if (last_page_size == 0)
In clustermd, Separate write-intent-bitmaps are used for each cluster node: 0 4k 8k 12k ------------------------------------------------------------------- | idle | md super | bm super [0] + bits | | bm bits[0, contd] | bm super[1] + bits | bm bits[1, contd] | | bm super[2] + bits | bm bits [2, contd] | bm super[3] + bits | | bm bits [3, contd] | | | So in node 1, pg_index in __write_sb_page() could equal to bitmap->storage.file_pages. Then bitmap_limit will be calculated to 0. md_super_write() will be called with 0 size. That means node the first 4k sb area of node 1 will never be updated through filemap_write_page(). This bug causes hang of mdadm/clustermd_tests/01r1_Grow_resize. Here use (pg_index % bitmap->storage.file_pages) to calculation of bitmap_limit correct. Fixes: ab99a87542f1 ("md/md-bitmap: fix writing non bitmap pages") Signed-off-by: Su Yue <glass.su@suse.com> --- drivers/md/md-bitmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)