super-ddf: optimize DDF header search for widely used RAID controllers #136

slayercat · 2024-12-09T08:18:50Z

Added logic to check the last 512 bytes of the device (dsize - 512) first
for the presence of the DDF header (DDF_HEADER_MAGIC).

If the header is found, the function immediately proceeds without performing a broader search.
Validates the magic value after reading the 512 bytes.

Fallback to searching the last 32MB of the device

using the search_for_ddf_headers function when the last 512 bytes do not contain the header.

Performs a block-by-block search to locate the DDF header within the specified region.

see also: #135

mtkaczyk · 2024-12-09T09:36:14Z

HI @slayercat
Contribution here requires understanding of kernel coding style.
There are a lot of comments made by our automation, please address them first (make review job passing).

mtkaczyk · 2024-12-09T09:38:08Z

super-ddf.c

+	const size_t READ_BLOCK_SIZE = 4096;
+	const unsigned long long SEARCH_REGION_SIZE = 32 * 1024 * 1024;
+
+	// Determine the search range


For comments, you should use one of:

/*Body*/ /* * Body */

ncroxon · 2024-12-09T18:43:01Z

@slayercat
Please check https://github.com/md-raid-utilities/mdadm/pull/136/checks for errors...

slayercat · 2024-12-10T15:55:56Z

Thank you for pointing out the issues with the code style. I have reviewed and modified the submitted code snippets.

mtkaczyk · 2024-12-11T14:34:50Z

super-ddf: optimize DDF header search for widely used RAID controllers
**Added logic to check the last 512 bytes of the device (`dsize -
  512`) first**
for the presence of the DDF header (`DDF_HEADER_MAGIC`).
  - If the header is found, the function immediately proceeds without
    performing a broader search.
  - Validates the magic value after reading the 512 bytes.

**Fallback to searching the last 32MB of the device**
using the  `search_for_ddf_headers` function when the last 512 bytes
do not contain the header.
  - Performs a block-by-block search to locate the DDF header
    within the specified region.

See also: https://github.com/md-raid-utilities/mdadm/issues/135

Signed-off-by: lilinzhe <[email protected]>

Kernel style is not only about the code style. You should also follow commit rules, described here:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes

I think that the description is good but you should divide it into 2 patches. Please do not use markup in commit body, you can keep it in PR description because I'm always cherry-picking commits and PR description is irrelevant.

Sorry, for that! I'm not trying to be nitpicker. As long as mailing list contribution is welcomed we trying to keep similar level of commits quality!

If you are interested, You can explore list online archive to find see comments from kernel folks on mdadm patches (you can search by "mdadm"):
https://lore.kernel.org/linux-raid/

I hope, this exercise will be useful for you to be better prepared for linux kernel contributions :)

mtkaczyk

Hello,
I did a quick code review. Please see my comments.

Thanks,
Mariusz

mtkaczyk · 2024-12-16T08:28:37Z

super-ddf.c

+	 * A dummy update_super function is needed
+	 * to enable the mounting feature.
+	 */
+	pr_err("%s: not implemented\n", __func__);


printing error each time is pointless. Please convert this to debug message dprintf or remove it.

Thank you for addressing that. I have converted this to use dprintf.

mtkaczyk · 2024-12-16T08:40:33Z

super-ddf.c

+			 int uuid_set, char *homehost)
+{
+	/*
+	 * A dummy update_super function is needed


What I see is mdadm assumes that this function exists for each metadata handler.
This is not about mounting at all but about making mdadm to work reliable with DDF.

It would be nice if you can put a crash (I assume it is crashing) in the commit message.

I would rephrase it:

A dummy function is defined because availability of ss->update_super is not always verified.

I don't see any clear reference to mounting in this context.
Mounting is handler by kernel when mdadm handles assemblation (preparing of the raid device to be mounted on demand later).

Thank you for addressing that. I have added the reason in the commit message and found a related discussion about this in the maillist.

commit e3d493d7046cc07b4eca95a2cc24cc59e0ee3b34 Author: lilinzhe <[email protected]> Date: Mon Dec 16 12:00:02 2024 +0800 super-ddf: Prevent crash when handling DDF metadata A dummy function is defined because availability of ss->update_super is not always verified. This fix addresses a crash reported when assembling a RAID array using mdadm with DDF metadata. For more details, see the discussion at: https://lore.kernel.org/all/ CALHdMH30LuxR4tz9jP2ykDaDJtZ3P7L3LrZ+9e4Fq=Q6NwSM=Q@mail.gmail.com/ The discussion centers on an issue with mdadm where attempting to assemble a RAID array caused a null pointer dereference. The problem was traced to a missing update_super() function in super-ddf.c, which led to a crash in Assemble.c. Signed-off-by: lilinzhe <[email protected]>

mtkaczyk · 2024-12-16T08:42:36Z

super-ddf.c

@@ -877,30 +877,170 @@ static void *load_section(int fd, struct ddf_super *super, void *buf,
 	return buf;
 }

-static int load_ddf_headers(int fd, struct ddf_super *super, char *devname)
+
+/* Search for DDF_HEADER_MAGIC in the last 32MB of the device */


Please copy part of the commit message here, I would like to have clarification why we are not following spec as a comment to. blame is volatile.

Thank you for addressing that. I have added the reason to the comments at the beginning of search_for_ddf_headers.

/* * Search for DDF_HEADER_MAGIC in the last 32MB of the device * * According to the specification, the Anchor Header should be placed at * the end of the disk. However,some widely used RAID hardware, such as * LSI and PERC, do not position it within the last 512 bytes of the disk. * */ static int search_for_ddf_headers(int fd, char *devname, unsigned long long *out)

super-ddf.c

A dummy function is defined because availability of ss->update_super is not always verified. This fix addresses a crash reported when assembling a RAID array using mdadm with DDF metadata. For more details, see the discussion at: https://lore.kernel.org/all/ CALHdMH30LuxR4tz9jP2ykDaDJtZ3P7L3LrZ+9e4Fq=Q6NwSM=Q@mail.gmail.com/ The discussion centers on an issue with mdadm where attempting to assemble a RAID array caused a null pointer dereference. The problem was traced to a missing update_super() function in super-ddf.c, which led to a crash in Assemble.c. Signed-off-by: lilinzhe <[email protected]>

slayercat · 2024-12-17T01:11:13Z

Thank you very much for your prompt response and detailed guidance. I have addressed your comments and corrected the submission accordingly.

If there is anything I may have misunderstood or any other areas that need further adjustments, please let me know.

Additionally, I performed a local CI check, focusing on the unit tests that previously failed in the CI server. Strangely, the tests that failed on the server did not fail locally. However, some tests failed locally, such as 07revert-inplace and 09imsm-assemble. I am not sure if these failures are related to the new submission.

I have attached the output of /var/tmp/07revert-inplace.log for your reference.

++ '[' 11 -le 10 ']'
++ echo 'Reshape has not started after 10 seconds'
Reshape has not started after 10 seconds
++ echo 'Waiting for grow-continue to finish'
Waiting for grow-continue to finish
++ wait_for_reshape_end
++ true
+++ grep -Ec '(resync|recovery|reshape|check|repair) *=' /proc/mdstat
++ sync_action=0
++ ((  0 != 0  ))
+++ pgrep -f 'mdadm --grow --continue'
++ [[ '' != '' ]]
++ break
++ sleep 5
++ wait_for_reshape_end
++ true
+++ grep -Ec '(resync|recovery|reshape|check|repair) *=' /proc/mdstat
++ sync_action=0
++ ((  0 != 0  ))
+++ pgrep -f 'mdadm --grow --continue'
++ [[ '' != '' ]]
++ break
++ echo 100
++ echo 500
++ check raid6
++ case $1 in
++ grep -sq 'active raid6 ' /proc/mdstat
++ die 'active raid6 not found'
++ echo -e '\n\tERROR: active raid6 not found \n'

        ERROR: active raid6 not found

++ save_log fail
++ status=fail
++ logfile=fail07revert-inplace.log
++ cat /var/tmp/stderr
++ cp /var/tmp/log /var/tmp/07revert-inplace.log
++ echo '## localhost: saving dmesg.'
++ dmesg -c
++ echo '## localhost: saving proc mdstat.'
++ cat /proc/mdstat
++ array=($(mdadm -Ds | cut -d' ' -f2))
+++ mdadm -Ds
+++ cut '-d ' -f2
+++ rm -f /var/tmp/stderr
+++ case $* in
+++ case $* in
+++ /sbin/mdadm -Ds
+++ rv=0
+++ case $* in
+++ cat /var/tmp/stderr
+++ return 0
++ '[' fail == fail ']'
++ fail FAILED
++ printf '\033[0;31mFAILED\033[0m'
^[[0;31mFAILED^[[0m++ echo ' - see /var/tmp/07revert-inplace.log and /var/tmp/fail07revert-inplace.log for details'
 - see /var/tmp/07revert-inplace.log and /var/tmp/fail07revert-inplace.log for details
++ '[' loop == lvm ']'
++ '[' loop == loop -o loop == disk ']'
++ '[' '!' -z '' -a 0 -ge 1 ']'
++ echo '## localhost: no array assembled!'
++ exit 2

Implemented fallback logic to search the last 32MB of the device for the DDF header (magic). If found, proceeds to load the DDF metadata from the located position. According to the specification, the Anchor Header should be placed at the end of the disk. However,some widely used RAID hardware, such as LSI and PERC, do not position it within the last 512 bytes of the disk. Signed-off-by: lilinzhe <[email protected]>

mtkaczyk · 2024-12-18T12:31:42Z

Thanks @slayercat,
I'm sick now and Christmas time is comming. I will proceed with it after holidays.

slayercat · 2024-12-19T01:22:31Z

Take care~

I hope you feel better soon!

Wishing you a joyful Christmas season filled with warmth and happiness. Rest up and take your time.

Looking forward to catching up after the holidays.

mtkaczyk reviewed Dec 9, 2024

View reviewed changes

slayercat force-pushed the main branch 2 times, most recently from 527767d to 441074a Compare December 10, 2024 15:54

slayercat force-pushed the main branch from 441074a to f478437 Compare December 10, 2024 16:00

slayercat force-pushed the main branch from f478437 to 44ee069 Compare December 16, 2024 06:44

mtkaczyk requested changes Dec 16, 2024

View reviewed changes

slayercat force-pushed the main branch from 44ee069 to 4fbf9ee Compare December 17, 2024 00:48

slayercat force-pushed the main branch from 4fbf9ee to e048f9b Compare December 17, 2024 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

super-ddf: optimize DDF header search for widely used RAID controllers #136

super-ddf: optimize DDF header search for widely used RAID controllers #136

slayercat commented Dec 9, 2024 •

edited

Loading

mtkaczyk commented Dec 9, 2024

mtkaczyk Dec 9, 2024

ncroxon commented Dec 9, 2024

slayercat commented Dec 10, 2024

mtkaczyk commented Dec 11, 2024

mtkaczyk left a comment

mtkaczyk Dec 16, 2024

slayercat Dec 17, 2024

mtkaczyk Dec 16, 2024

slayercat Dec 17, 2024

mtkaczyk Dec 16, 2024

slayercat Dec 17, 2024

slayercat commented Dec 17, 2024 •

edited

Loading

mtkaczyk commented Dec 18, 2024

slayercat commented Dec 19, 2024

super-ddf: optimize DDF header search for widely used RAID controllers #136

Are you sure you want to change the base?

super-ddf: optimize DDF header search for widely used RAID controllers #136

Conversation

slayercat commented Dec 9, 2024 • edited Loading

mtkaczyk commented Dec 9, 2024

mtkaczyk Dec 9, 2024

Choose a reason for hiding this comment

ncroxon commented Dec 9, 2024

slayercat commented Dec 10, 2024

mtkaczyk commented Dec 11, 2024

mtkaczyk left a comment

Choose a reason for hiding this comment

mtkaczyk Dec 16, 2024

Choose a reason for hiding this comment

slayercat Dec 17, 2024

Choose a reason for hiding this comment

mtkaczyk Dec 16, 2024

Choose a reason for hiding this comment

slayercat Dec 17, 2024

Choose a reason for hiding this comment

mtkaczyk Dec 16, 2024

Choose a reason for hiding this comment

slayercat Dec 17, 2024

Choose a reason for hiding this comment

slayercat commented Dec 17, 2024 • edited Loading

mtkaczyk commented Dec 18, 2024

slayercat commented Dec 19, 2024

slayercat commented Dec 9, 2024 •

edited

Loading

slayercat commented Dec 17, 2024 •

edited

Loading