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

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

On Wed, 20 Nov 2024 14:46:36 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> 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!

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.

+ 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.

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