On Fri, 22 Nov 2024 09:13:18 +0800
Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> 在 2024/11/21 16:15, Mariusz Tkaczyk 写道:
> > On Thu, 21 Nov 2024 09:25:50 +0800
> > Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> >
> >>> BitmapUnknown should be used only if we failed to parse bitmap setting in
> >>> cmdline. Otherwise first and default value should be always BitmapNone
> >>> because data access is always highest priority and dropping bitmap is
> >>> always safe. We can print warning in config parse failed or bitmap value
> >>> is repeated- it is reasonable. If I'm wrong here, please let me know.
> >>
> >> Hi, there is a little difference betewwn BitmapNone and BitmapUnknow, if
> >> user doesn't pass in the "bitmap=xxx", then the BitmapUnkonw will be
> >> used to decide choosing BitmapNone or BimtapInternal based on the disk
> >> size. In Create:
> >>
> >> if (!s->bitmap_file &&
> >> ┊ !st->ss->external &&
> >> ┊ s->level >= 1 &&
> >> ┊ st->ss->add_internal_bitmap &&
> >> ┊ s->journaldisks == 0 &&
> >> ┊ (s->consistency_policy != CONSISTENCY_POLICY_RESYNC &&
> >> ┊ s->consistency_policy != CONSISTENCY_POLICY_PPL) &&
> >> ┊ (s->write_behind || s->size > 100*1024*1024ULL)) {
> >> if (c->verbose > 0)
> >> pr_err("automatically enabling write-intent
> >> bitmap on large array\n");
> >> s->bitmap_file = "internal";
> >> }
> >>
> >> And I realized that I should used BitmapUnknow here, not BimtapNone.
> >
> > Oh yes.. Looking on that from the interface perspective suggest me that we
> > should remove it and always let user to decide. If the are not satisfied
> > with resync times they can enable bitmap in any moment but it may cause
> > functional regression for users that are used to this auto turning on.
> >
> > Maybe, we can move it to main() and ask without checking raid size, assuming
> > that array size <100GB is used mainly for testing nowadays?
> >
> > Here, proposal based on current code, your change may require some
> > adjustments:
> >
> > diff --git a/mdadm.c b/mdadm.c
> > index 8cb4ba66ac20..2e803d441dd4 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -1535,6 +1535,13 @@ int main(int argc, char *argv[])
> > break;
> > }
> >
> > + if (!s->bitmap_file && !c.runstop != 1 && s->level >= 1) {
> > + int response = ask("To optimalize resync speed, it
> > is recommended to enable write-indent bitmap, do you want to enable it
> > now?"); +
> > + if (response)
> > + s->bitmap_file = "internal";
> > + }
> > +
> > rv = Create(ss, &ident, devs_found - 1, devlist->next, &s,
> > &c); break;
> > case MISC:
> >
> > This is more reasonable than auto-forcing bitmap without possibility
> > to skip it (even for testing). I added c->runstop verification because it is
> > often used in Create to skip some errors and questions.
> >
> > What do you think?
>
> I think it's good! I used to be curious why bitmap is not enabled by
> default for testing, and have to look into the source code.
>
One note here (this one is easy to be missed):
If user set --bitmap=None we should not prompt this question, assuming that user
already made his decision. You need to differentiate default BitmapNone
and user defined BitmapNone (boolean is_bitmap_set should be fine, because
adding another enum status is not valuable I think).
Mariusz
