Skip to content

mdadm: fix bitmap check#255

Open
bkucman wants to merge 1 commit into
md-raid-utilities:mainfrom
bkucman:bitmap_check_fix
Open

mdadm: fix bitmap check#255
bkucman wants to merge 1 commit into
md-raid-utilities:mainfrom
bkucman:bitmap_check_fix

Conversation

@bkucman
Copy link
Copy Markdown
Collaborator

@bkucman bkucman commented Mar 11, 2026

Commit e97c4e1 ("mdadm: ask user if bitmap is not set") adds a prompt to the user regarding bitmap enabling during creation. However, the case of creating a container, where the bitmap is not meaningful, was omitted.

This may mislead the user, as attempting to enable bitmap ends with a container creation error: "mdadm: bitmaps not meaningful with level container"

At the stage where this check is performed, the level holds the UnSet value, LEVEL_CONTAINER is set at a later stage in Create.

The fix adds a check if the level is UnSet and set bitmap to BitmapNone, because this is the only acceptable value for the container. This check also covers the case when the level is not given during volume creation, as it makes no sense to ask for a bitmap without knowledge about level.

Commit e97c4e1 ("mdadm: ask user if bitmap is not set") adds a prompt to the user regarding
bitmap enabling during creation. However, the case of creating a container, where the bitmap is not
meaningful, was omitted.

This may mislead the user, as attempting to enable bitmap ends with a container creation error:
"mdadm: bitmaps not meaningful with level container"

At the stage where this check is performed, the level holds the UnSet value, LEVEL_CONTAINER
is set at a later stage in Create.

The fix adds a check if the level is UnSet and set bitmap to BitmapNone, because this is the only
acceptable value for the container. This check also covers the case when the level is not given
during volume creation, as it makes no sense to ask for a bitmap without knowledge about level.

Signed-off-by: Blazej Kucman <blazej.kucman@graidtech.com>
Copy link
Copy Markdown
Collaborator

@mwilck mwilck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, one nitpick below.

Comment thread mdadm.c
if (s.level == UnSet)
s.btype = BitmapNone;
else if (c.runstop != 1 && s.level >= 1 &&
ask("To optimize recovery speed, it is recommended to enable write-intent bitmap, do you want to enable it now?"))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation broken?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is part of the "if" condition, I aligned it with the first argument in parentheses.
but I also see that GH cannot display indents correctly.

Image

If there is another better approach to this let me know.

@mtkaczyk
Copy link
Copy Markdown
Member

mtkaczyk commented Mar 13, 2026

		case O(BUILD,'l'): /* set raid level*/
			if (s.level != UnSet) {
				pr_err("raid level may only be set once.  Second value is %s.\n", optarg);
				exit(2);
			}
			s.level = map_name(pers, optarg);
			if (s.level == UnSet) {
				pr_err("invalid raid level: %s\n",
					optarg);
				exit(2);

Could you please share the scenario it was discovered? is it regular:
# mdadm -CR imsm -e imsm ...

If you will add --level=container to the command what will happen?

Did you considered something like:

		case O(CREATE,'e'):
		if (optarg == "imsm"
                   s.level = LEVEL_CONTAINER

Just asking, I don't remember exactly the flows.
At least comment it missing because the fix is an workaround for a mdadm.c rutine, maybe we should just discover container earlier as proposed but it was probably never well tested so there could be new issues.

Anyway, I think that discovering container is right way to fix this rather than workarounding the cmdline.

@mwilck mwilck mentioned this pull request Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants