On Monday November 17, maan@xxxxxxxxxxxxxxx wrote:
> Hi
>
> the current for-next kernel oopses on the 00multipath test of the
> mdadm test suite. I'm not sure if the patch below is the best way to
> fix this problem, but it avoids the oops and makes the 00multipath
> test succeed. In the long run it might be preferable to use a wrapper
> for sysfs_notify_dirent() that checks for NULL pointers, or even teach
> sysfs_notify_dirent() to return early if it gets passed a NULL pointer.
Hi Andre,
Thanks for this.
I thought I had fixed that already, but maybe I hadn't pushed the
tree out. I have now.
BTW, I have two problems with your patch as it stands.
> @@ -6233,8 +6233,8 @@ void md_check_recovery(mddev_t *mddev)
> unlock:
> if (!mddev->sync_thread) {
> clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> - if (test_and_clear_bit(MD_RECOVERY_RECOVER,
> - &mddev->recovery))
> + if (mddev->sysfs_action && test_and_clear_bit(
> + MD_RECOVERY_RECOVER, &mddev->recovery))
> sysfs_notify_dirent(mddev->sysfs_action);
> }
1/ The indenting is not what I like. Having the arguments to a
function further left than the function name makes is hard to read.
I would prefer
if (mddev->sysfs_action &&
test_and_clear_bit(MD_RECOVERY_RECOVER,
&mddev->recovery))
sysfs_notify_dirent(mddev->sysfs_action);
which is one more line, but worth it.
2/ You have introduced a subtle semantic change. If ->sys_action is
NULL, we no longer clear MD_RECOVERY_RECOVER. I suspect this isn't
actually an error in practice, but I think it is still the wrong
way around to do the tests.
I have
if (test_and_clear_bit(MD_RECOVERY_RECOVER,
&mddev->recovery))
if (mddev->sysfs_action)
sysfs_notify_dirent(mddev->sysfs_action);
Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
