| Message ID | 20240810020854.797814-2-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | md/md-bitmap: introduce bitmap_operations | expand |
On Sat, 10 Aug 2024 10:08:29 +0800 Yu Kuai <yukuai1@huaweicloud.com> wrote: > From: Yu Kuai <yukuai3@huawei.com> > > The structure is empty for now, and will be used in later patches to > merge in bitmap operations, prepare to implement a new lock free > bitmap in the fulture. HI Kuai, typo: future I think that as "later" you meant next patches in this patchset, right? If yes, Please you "next" instead. At this point bringing "lock free implementation in the future" looks like totally unnecessary. It may happen or may not. Eventually, You can mention it in cover letter. What we have now is the most important. This is just a code rework. The main goal of this (as you mentioned in second patch): "So that the implementation won't be exposed, and it'll be possible to invent a new bitmap by replacing bitmap_operations." but please mention that you are not going to implement second mechanism to not confuse reader. You need it to improve code maintainability mainly I think. I would mention something about common "struct _ops" pattern (the special structure to keep related operations together) that it is going to follow now. For me, this one can be easy merged with the patch 2, so we will got struct + usage in one patch. Anyway, LGTM. Thanks, Mariusz
Hi, 在 2024/08/13 14:58, Mariusz Tkaczyk 写道: > On Sat, 10 Aug 2024 10:08:29 +0800 > Yu Kuai <yukuai1@huaweicloud.com> wrote: > >> From: Yu Kuai <yukuai3@huawei.com> >> >> The structure is empty for now, and will be used in later patches to >> merge in bitmap operations, prepare to implement a new lock free >> bitmap in the fulture. > > > HI Kuai, > > typo: future > > I think that as "later" you meant next patches in this patchset, right? If yes, > Please you "next" instead. > > At this point bringing "lock free implementation in the future" looks like > totally unnecessary. It may happen or may not. Eventually, You can mention it in > cover letter. What we have now is the most important. > > This is just a code rework. The main goal of this (as you mentioned in second > patch): > > "So that the implementation won't be exposed, and it'll be possible > to invent a new bitmap by replacing bitmap_operations." > > but please mention that you are not going to implement second mechanism to not > confuse reader. You need it to improve code maintainability mainly I think. > > I would mention something about common "struct _ops" pattern (the special > structure to keep related operations together) that it is going to follow now. > > For me, this one can be easy merged with the patch 2, so we will got struct + > usage in one patch. Thanks for the review. Got it. BTW, v2 will look totally different with v1. You can leave v1 for now. :) Kuai > > Anyway, LGTM. > > Thanks, > Mariusz > > . >
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index 08232d8dc815..d9838fbcc9d7 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c @@ -2707,3 +2707,11 @@ const struct attribute_group md_bitmap_group = { .name = "bitmap", .attrs = md_bitmap_attrs, }; + +static struct bitmap_operations bitmap_ops = { +}; + +void mddev_set_bitmap_ops(struct mddev *mddev) +{ + mddev->bitmap_ops = &bitmap_ops; +} diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h index bb9eb418780a..992feaf66d47 100644 --- a/drivers/md/md-bitmap.h +++ b/drivers/md/md-bitmap.h @@ -234,7 +234,11 @@ struct bitmap { int cluster_slot; /* Slot offset for clustered env */ }; +struct bitmap_operations { +}; + /* the bitmap API */ +void mddev_set_bitmap_ops(struct mddev *mddev); /* these are used only by md/bitmap */ struct bitmap *md_bitmap_create(struct mddev *mddev, int slot); diff --git a/drivers/md/md.c b/drivers/md/md.c index d3a837506a36..f23138dd96a7 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -772,6 +772,7 @@ int mddev_init(struct mddev *mddev) mddev->resync_min = 0; mddev->resync_max = MaxSector; mddev->level = LEVEL_NONE; + mddev_set_bitmap_ops(mddev); INIT_WORK(&mddev->sync_work, md_start_sync); INIT_WORK(&mddev->del_work, mddev_delayed_delete); diff --git a/drivers/md/md.h b/drivers/md/md.h index a0d6827dced9..e56193f71ab4 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -536,6 +536,7 @@ struct mddev { int sync_checkers; /* # of threads checking writes_pending */ struct bitmap *bitmap; /* the bitmap for the device */ + struct bitmap_operations *bitmap_ops; struct { struct file *file; /* the bitmap file */ loff_t offset; /* offset from superblock of