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

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

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?

Thanks,
Mariusz