Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mdraid: Add 'bitmap' option to MDRaidCreate method #1124

Closed
wants to merge 1 commit into from
Closed

mdraid: Add 'bitmap' option to MDRaidCreate method #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion data/org.freedesktop.UDisks2.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@
@level: The RAID level for the array.
@name: The name for the array.
@chunk: The chunk size (in bytes) or 0 if @level is <quote>raid1</quote>.
@options: Options (currently unused except for <link linkend="udisks-std-options">standard options</link>).
@options: Options - known options (in addition to <link linkend="udisks-std-options">standard options</link>) include <parameter>bitmap</parameter> (of type 'ay')
@resulting_array: An object path to the object implementing the #org.freedesktop.UDisks2.MDRaid interface.
@since: 2.0.0

Expand All @@ -165,6 +165,9 @@
Before the array is created, all devices in @blocks are
erased. Once created (but before the method returns), the RAID
array will be erased.

The @bitmap option can have the same values as are accepted by
SetBitmapLocation.
Comment on lines +169 to +170
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explicitly state the values accepted, for the sake of completeness? I.e. The value for the bitmap, currently only the values 'none' and 'internal' are supported.

Copy link
Member

Choose a reason for hiding this comment

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

Also, looking at the mdadm manpage, in case bd_md_create(bitmap=False), libblockdev doesn't add any --bitmap= option, leaving the logic on automatic mdadm rule:

When creating an array on devices which are 100G or larger, mdadm automatically adds an internal bitmap as it will usually be beneficial. This can be suppressed with --bitmap=none or by selecting a different consistency policy with --consistency-policy.

So either the values in UDisks should read 'auto' and 'internal', or we should change bd_md_create() to accept values of 'none', 'auto' and 'internal'. Intentionally omitting 'clustered' and filename as those we don't want to support.

Copy link
Contributor Author

@mvollmer mvollmer Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When creating an array on devices which are 100G or larger, mdadm automatically adds an internal bitmap as it will usually be beneficial. This can be suppressed with --bitmap=none or by selecting a different consistency policy with --consistency-policy.

Oh, interesting, let's bring in the experts.

@mauelsha, from our discussion in Brno I had the impression that mdadm will never use a bitmap but that it is always a good idea to use one. Now the man page seems to say that a bitmap is not always a good idea, and that mdadm does in fact decides to use a bitmap exactly when it is a good idea.

Can you clarify?

Right now I am thinking that we don't need to change anything, after all. New mdraid devices will have a bitmap or not as decided by the mdraid maintainers, and Cockpit allows it to be adjusted in case the maintainers are wrong in certain situations.

-->
<method name="MDRaidCreate">
<arg name="blocks" direction="in" type="ao"/>
Expand Down
12 changes: 11 additions & 1 deletion src/udiskslinuxmanager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,7 @@ handle_mdraid_create (UDisksManager *_object,
const gchar **disks = NULL;
guint disks_top = 0;
gboolean success = FALSE;
const gchar *option_bitmap = NULL;

if (!udisks_daemon_util_get_caller_uid_sync (manager->daemon,
invocation,
Expand Down Expand Up @@ -765,7 +766,16 @@ handle_mdraid_create (UDisksManager *_object,
}
disks[disks_top] = NULL;

if (!bd_md_create (array_name, arg_level, disks, 0, NULL, FALSE, arg_chunk, NULL, &error))
g_variant_lookup (arg_options, "bitmap", "^&ay", &option_bitmap);
if (!(g_strcmp0 (option_bitmap, "none") == 0 || g_strcmp0 (option_bitmap, "internal") == 0))
Copy link
Member

Choose a reason for hiding this comment

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

One more thing, revealed by the failed tests - in case of a NULL value this check should be skipped.

{
g_dbus_method_invocation_return_error (invocation, UDISKS_ERROR, UDISKS_ERROR_FAILED,
"Only values 'none' and 'internal' are currently supported for 'bitmap'.");
success = FALSE;
goto out;
}

if (!bd_md_create (array_name, arg_level, disks, 0, NULL, g_strcmp0 (option_bitmap, "internal") == 0, arg_chunk, NULL, &error))
{
g_prefix_error (&error, "Error creating RAID array: ");
udisks_simple_job_complete (UDISKS_SIMPLE_JOB (job), FALSE, error->message);
Expand Down