Re: [PATCH v3 3/4] mdadm: remove bitmap file support

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Fri, 22 Nov 2024 09:13:18 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> 在 2024/11/21 16:15, Mariusz Tkaczyk 写道:
> > On Thu, 21 Nov 2024 09:25:50 +0800
> > Yu Kuai <yukuai1@huaweicloud.com> 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