Hi,
在 2024/11/20 18:27, Mariusz Tkaczyk 写道:
On Wed, 20 Nov 2024 14:46:36 +0800
Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>
Because it's marked deprecated for a long time now, and it's not worthy
to support it for new bitmap.
Now that we don't need to store filename for bitmap, also declare a new
enum type bitmap_type to simplify code.
Thanks for the enum! I really appreciate the additional effort you took to
make mdadm better.
I didn't not review it line by line because I see the problem that must
be resolved first.
I see that you added BitmapNone and BitmapUnknown and their usage is not clear,
let me help you!
Yeah, BitmapUnknow is used to match bitmap=NULL, and BitmapNonw is used
to match bitmap="none" before this patch.
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.
+ It would be nice to add tests to cover these config/cmdline bitmap
possibilities to define clear set of expected behavior. It is something
already missed so I do not require that strongly from you know.
That's fine, just we need 100GB+ disk for the above default choice.
Thanks,
Kuai
I propose you to create mapping_t for bitmap and to use map_name() to match the
bitmap strings, instead of hardcoding them but it is my recommendation not
something strongly required.
Then, you would be able to remove some checks for both (s->btype != BitmapNone
&& s->btype != BitmapUnknown).
The change proposed by my will provide clear differentiation between error
value and set of accepted values, messing that is always confusing for
maintainers end readers. I don't see that kind of mess necessary in this case.
Thanks,
Mariusz
.
