| Message ID | 20221014122210.47888-1-jinpu.wang@ionos.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC] md-bitmap: reuse former bitmap chunk size on resizing. | expand |
On Fri, Oct 14, 2022 at 5:22 AM Jack Wang <jinpu.wang@ionos.com> wrote: > > From: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> > > On bitmap resizing, attempt to reuse the former chunk size (if any) > in order to preserve the, ev. on purpose set, chunk size resolution. Could you please explain more on why we would rather keep the chunk size instead of recalculating it? Thanks, Song > > Signed-off-by: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/md/md-cluster.c | 3 ++- > drivers/md/raid1.c | 3 ++- > drivers/md/raid10.c | 12 ++++++++---- > drivers/md/raid5.c | 3 ++- > 4 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index 10e0c5381d01..9929631bdc94 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -604,7 +604,8 @@ static int process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg) > case BITMAP_RESIZE: > if (le64_to_cpu(msg->high) != mddev->pers->size(mddev, 0, 0)) > ret = md_bitmap_resize(mddev->bitmap, > - le64_to_cpu(msg->high), 0, 0); > + le64_to_cpu(msg->high), > + mddev->bitmap_info.chunksize, 0); > break; > default: > ret = -1; > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index 05d8438cfec8..8f1d25064a53 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -3225,7 +3225,8 @@ static int raid1_resize(struct mddev *mddev, sector_t sectors) > mddev->array_sectors > newsize) > return -EINVAL; > if (mddev->bitmap) { > - int ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); > + int ret = md_bitmap_resize(mddev->bitmap, newsize, > + mddev->bitmap_info.chunksize, 0); > if (ret) > return ret; > } > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 3aa8b6e11d58..8f0453b6e0c6 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -4346,7 +4346,8 @@ static int raid10_resize(struct mddev *mddev, sector_t sectors) > mddev->array_sectors > size) > return -EINVAL; > if (mddev->bitmap) { > - int ret = md_bitmap_resize(mddev->bitmap, size, 0, 0); > + int ret = md_bitmap_resize(mddev->bitmap, size, > + mddev->bitmap_info.chunksize, 0); > if (ret) > return ret; > } > @@ -4618,7 +4619,8 @@ static int raid10_start_reshape(struct mddev *mddev) > newsize = raid10_size(mddev, 0, conf->geo.raid_disks); > > if (!mddev_is_clustered(mddev)) { > - ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); > + ret = md_bitmap_resize(mddev->bitmap, newsize, > + mddev->bitmap_info.chunksize, 0); > if (ret) > goto abort; > else > @@ -4640,13 +4642,15 @@ static int raid10_start_reshape(struct mddev *mddev) > MD_FEATURE_RESHAPE_ACTIVE)) || (oldsize == newsize)) > goto out; > > - ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); > + ret = md_bitmap_resize(mddev->bitmap, newsize, > + mddev->bitmap_info.chunksize, 0); > if (ret) > goto abort; > > ret = md_cluster_ops->resize_bitmaps(mddev, newsize, oldsize); > if (ret) { > - md_bitmap_resize(mddev->bitmap, oldsize, 0, 0); > + md_bitmap_resize(mddev->bitmap, oldsize, > + mddev->bitmap_info.chunksize, 0); > goto abort; > } > } > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index 7b820b81d8c2..bff7d3d779ae 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -8372,7 +8372,8 @@ static int raid5_resize(struct mddev *mddev, sector_t sectors) > mddev->array_sectors > newsize) > return -EINVAL; > if (mddev->bitmap) { > - int ret = md_bitmap_resize(mddev->bitmap, sectors, 0, 0); > + int ret = md_bitmap_resize(mddev->bitmap, sectors, > + mddev->bitmap_info.chunksize, 0); > if (ret) > return ret; > } > -- > 2.34.1 >
On Thu, Oct 20, 2022 at 10:26 PM Florian-Ewald Müller <florian-ewald.mueller@ionos.com> wrote: > > Hello Song, > > Thank you for taking this patch into consideration. > This patch was indeed meant only as a RFC. > > We have, sometimes, the situation that a md-raid1 device of e.g. size=128G, created > with bitmap=internal and bitmap-chunk=256M, needs to be resized to (let's say) size=8T. > > We originally set the bitmap-chunk=256M because we found it a good compromise between performance and resync speed. > With that bitmap-chunk=256G and the starting size=128G, the md-bitmap code will use only 512 bits fitting into a page (4K). > > When resizing to size=8T, the md-bitmap code will attempt to reuse the space available (4k) and calculate a bitmap-chunk=2G. > We found here that such a huge bitmap-chunk (2G) is not so suitable for an eventual, later resync process. > Also taking into consideration that IOPS issued on the now resized device won't increase/differ significantly. > > This is a particular usage scenario and the effect of this patch may, indeed, not be optimal in some other use case. > > Best regards, > Florian > > resent with plain text, so it's available on mainlist > > On Thu, Oct 20, 2022 at 9:22 PM Song Liu <song@kernel.org> wrote: >> >> On Fri, Oct 14, 2022 at 5:22 AM Jack Wang <jinpu.wang@ionos.com> wrote: >> > >> > From: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> >> > >> > On bitmap resizing, attempt to reuse the former chunk size (if any) >> > in order to preserve the, ev. on purpose set, chunk size resolution. >> >> Could you please explain more on why we would rather keep the chunk size >> instead of recalculating it? >> >> Thanks, >> Song >> >> > >> > Signed-off-by: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> >> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> >> > --- >> > drivers/md/md-cluster.c | 3 ++- >> > drivers/md/raid1.c | 3 ++- >> > drivers/md/raid10.c | 12 ++++++++---- >> > drivers/md/raid5.c | 3 ++- >> > 4 files changed, 14 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c >> > index 10e0c5381d01..9929631bdc94 100644 >> > --- a/drivers/md/md-cluster.c >> > +++ b/drivers/md/md-cluster.c >> > @@ -604,7 +604,8 @@ static int process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg) >> > case BITMAP_RESIZE: >> > if (le64_to_cpu(msg->high) != mddev->pers->size(mddev, 0, 0)) >> > ret = md_bitmap_resize(mddev->bitmap, >> > - le64_to_cpu(msg->high), 0, 0); >> > + le64_to_cpu(msg->high), >> > + mddev->bitmap_info.chunksize, 0); >> > break; >> > default: >> > ret = -1; >> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c >> > index 05d8438cfec8..8f1d25064a53 100644 >> > --- a/drivers/md/raid1.c >> > +++ b/drivers/md/raid1.c >> > @@ -3225,7 +3225,8 @@ static int raid1_resize(struct mddev *mddev, sector_t sectors) >> > mddev->array_sectors > newsize) >> > return -EINVAL; >> > if (mddev->bitmap) { >> > - int ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); >> > + int ret = md_bitmap_resize(mddev->bitmap, newsize, >> > + mddev->bitmap_info.chunksize, 0); >> > if (ret) >> > return ret; >> > } >> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c >> > index 3aa8b6e11d58..8f0453b6e0c6 100644 >> > --- a/drivers/md/raid10.c >> > +++ b/drivers/md/raid10.c >> > @@ -4346,7 +4346,8 @@ static int raid10_resize(struct mddev *mddev, sector_t sectors) >> > mddev->array_sectors > size) >> > return -EINVAL; >> > if (mddev->bitmap) { >> > - int ret = md_bitmap_resize(mddev->bitmap, size, 0, 0); >> > + int ret = md_bitmap_resize(mddev->bitmap, size, >> > + mddev->bitmap_info.chunksize, 0); >> > if (ret) >> > return ret; >> > } >> > @@ -4618,7 +4619,8 @@ static int raid10_start_reshape(struct mddev *mddev) >> > newsize = raid10_size(mddev, 0, conf->geo.raid_disks); >> > >> > if (!mddev_is_clustered(mddev)) { >> > - ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); >> > + ret = md_bitmap_resize(mddev->bitmap, newsize, >> > + mddev->bitmap_info.chunksize, 0); >> > if (ret) >> > goto abort; >> > else >> > @@ -4640,13 +4642,15 @@ static int raid10_start_reshape(struct mddev *mddev) >> > MD_FEATURE_RESHAPE_ACTIVE)) || (oldsize == newsize)) >> > goto out; >> > >> > - ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); >> > + ret = md_bitmap_resize(mddev->bitmap, newsize, >> > + mddev->bitmap_info.chunksize, 0); >> > if (ret) >> > goto abort; >> > >> > ret = md_cluster_ops->resize_bitmaps(mddev, newsize, oldsize); >> > if (ret) { >> > - md_bitmap_resize(mddev->bitmap, oldsize, 0, 0); >> > + md_bitmap_resize(mddev->bitmap, oldsize, >> > + mddev->bitmap_info.chunksize, 0); >> > goto abort; >> > } >> > } >> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c >> > index 7b820b81d8c2..bff7d3d779ae 100644 >> > --- a/drivers/md/raid5.c >> > +++ b/drivers/md/raid5.c >> > @@ -8372,7 +8372,8 @@ static int raid5_resize(struct mddev *mddev, sector_t sectors) >> > mddev->array_sectors > newsize) >> > return -EINVAL; >> > if (mddev->bitmap) { >> > - int ret = md_bitmap_resize(mddev->bitmap, sectors, 0, 0); >> > + int ret = md_bitmap_resize(mddev->bitmap, sectors, >> > + mddev->bitmap_info.chunksize, 0); >> > if (ret) >> > return ret; >> > } >> > -- >> > 2.34.1 >> >
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c index 10e0c5381d01..9929631bdc94 100644 --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -604,7 +604,8 @@ static int process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg) case BITMAP_RESIZE: if (le64_to_cpu(msg->high) != mddev->pers->size(mddev, 0, 0)) ret = md_bitmap_resize(mddev->bitmap, - le64_to_cpu(msg->high), 0, 0); + le64_to_cpu(msg->high), + mddev->bitmap_info.chunksize, 0); break; default: ret = -1; diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index 05d8438cfec8..8f1d25064a53 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -3225,7 +3225,8 @@ static int raid1_resize(struct mddev *mddev, sector_t sectors) mddev->array_sectors > newsize) return -EINVAL; if (mddev->bitmap) { - int ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); + int ret = md_bitmap_resize(mddev->bitmap, newsize, + mddev->bitmap_info.chunksize, 0); if (ret) return ret; } diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 3aa8b6e11d58..8f0453b6e0c6 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -4346,7 +4346,8 @@ static int raid10_resize(struct mddev *mddev, sector_t sectors) mddev->array_sectors > size) return -EINVAL; if (mddev->bitmap) { - int ret = md_bitmap_resize(mddev->bitmap, size, 0, 0); + int ret = md_bitmap_resize(mddev->bitmap, size, + mddev->bitmap_info.chunksize, 0); if (ret) return ret; } @@ -4618,7 +4619,8 @@ static int raid10_start_reshape(struct mddev *mddev) newsize = raid10_size(mddev, 0, conf->geo.raid_disks); if (!mddev_is_clustered(mddev)) { - ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); + ret = md_bitmap_resize(mddev->bitmap, newsize, + mddev->bitmap_info.chunksize, 0); if (ret) goto abort; else @@ -4640,13 +4642,15 @@ static int raid10_start_reshape(struct mddev *mddev) MD_FEATURE_RESHAPE_ACTIVE)) || (oldsize == newsize)) goto out; - ret = md_bitmap_resize(mddev->bitmap, newsize, 0, 0); + ret = md_bitmap_resize(mddev->bitmap, newsize, + mddev->bitmap_info.chunksize, 0); if (ret) goto abort; ret = md_cluster_ops->resize_bitmaps(mddev, newsize, oldsize); if (ret) { - md_bitmap_resize(mddev->bitmap, oldsize, 0, 0); + md_bitmap_resize(mddev->bitmap, oldsize, + mddev->bitmap_info.chunksize, 0); goto abort; } } diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 7b820b81d8c2..bff7d3d779ae 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -8372,7 +8372,8 @@ static int raid5_resize(struct mddev *mddev, sector_t sectors) mddev->array_sectors > newsize) return -EINVAL; if (mddev->bitmap) { - int ret = md_bitmap_resize(mddev->bitmap, sectors, 0, 0); + int ret = md_bitmap_resize(mddev->bitmap, sectors, + mddev->bitmap_info.chunksize, 0); if (ret) return ret; }