fail_last_dev and FailFast/LastDev flag incompatibility

Hi All,
During my work under failed arrays handling[1] improvements, I discovered potential issue with "failfast" and metadata writes. In commit message[2] Neil mentioned that:
"If we get a failure writing metadata but the device doesn't fail, it must be the last device so we re-write without FAILFAST".

Obviously, this is not true for RAID456 (again)[1] but it is also not true for RAID1 and RAID10 with "fail_las_dev"[3] functionality enabled.

I did a quick check and can see that setter for "LastDev" flag is called if "Faulty" on device is not set. I proposed some changes in the area in my patchset[4] but after discussion we decided to drop changes here. Current approach is not correct for all branches, so my proposal is to change:

drivers/md/md.c +1 -1

index 7b024912f1eb..3daec14ef6b2 100644
@@ -931,7 +931,7 @@ static void super_written(struct bio *bio)
                pr_err("md: %s gets error=%d\n", __func__,
                       blk_status_to_errno(bio->bi_status));
                md_error(mddev, rdev);
-               if (!test_bit(Faulty, &rdev->flags)
+               if (test_bit(MD_BROKEN, mddev->flag)
                    && (bio->bi_opf & MD_FAILFAST)) {
                        set_bit(MD_SB_NEED_REWRITE, &mddev->sb_flags);
                        set_bit(LastDev, &rdev->flags);

It will force "LastDev" to be set on every metadata rewrite if mddevice is known to be failed.
Do you have any other suggestions?

+ Guoqing - author of fail_last_dev.
+ Xiao - you are familiarized with FailFast so please take a look.

[1]https://lore.kernel.org/linux-raid/CAPhsuW54_9CTR6sh7mnQ6O77F2HNArLHGWHYsUdbNGy7pXgipQ@mail.gmail.com/T/#m8cf7c57429b6fd332220157186151900ce23865d [2]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=46533ff7fefb7e9e3539494f5873b00091caa8eb [3]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=9a567843f7ce [4]https://lore.kernel.org/linux-raid/CAPhsuW5bV+Bz=Od9jomNHoedaEMFAXymN11J80G62GVPwSp41g@mail.gmail.com/

Thanks,
Mariusz

Hi Marisuz

We don't need to consider MD_FAILFAST for raid456. Because only raid1 and raid10 support it.
MD_FAILFAST_SUPPORTED is only set in raid1_run/raid10_run. So LastDev only be useful for
raid1/raid10. It should be good to only check Faulty here.

Best Regards
Xiao

Quoted 48 lines · Mariusz Tkaczyk

Hi All,
During my work under failed arrays handling[1] improvements, I discovered potential issue with "failfast" and metadata writes. In commit message[2] Neil mentioned that:
"If we get a failure writing metadata but the device doesn't fail, it must be the last device so we re-write without FAILFAST".

Obviously, this is not true for RAID456 (again)[1] but it is also not true for RAID1 and RAID10 with "fail_las_dev"[3] functionality enabled.

I did a quick check and can see that setter for "LastDev" flag is called if "Faulty" on device is not set. I proposed some changes in the area in my patchset[4] but after discussion we decided to drop changes here. Current approach is not correct for all branches, so my proposal is to change:

drivers/md/md.c +1 -1

index 7b024912f1eb..3daec14ef6b2 100644
@@ -931,7 +931,7 @@ static void super_written(struct bio *bio)
                pr_err("md: %s gets error=%d\n", __func__,
                       blk_status_to_errno(bio->bi_status));
                md_error(mddev, rdev);
-               if (!test_bit(Faulty, &rdev->flags)
+               if (test_bit(MD_BROKEN, mddev->flag)
                    && (bio->bi_opf & MD_FAILFAST)) {
                        set_bit(MD_SB_NEED_REWRITE, &mddev->sb_flags);
                        set_bit(LastDev, &rdev->flags);

It will force "LastDev" to be set on every metadata rewrite if mddevice is known to be failed.
Do you have any other suggestions?

+ Guoqing - author of fail_last_dev.
+ Xiao - you are familiarized with FailFast so please take a look.

[1]https://lore.kernel.org/linux-raid/CAPhsuW54_9CTR6sh7mnQ6O77F2HNArLHGWHYsUdbNGy7pXgipQ@mail.gmail.com/T/#m8cf7c57429b6fd332220157186151900ce23865d [2]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=46533ff7fefb7e9e3539494f5873b00091caa8eb [3]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=9a567843f7ce [4]https://lore.kernel.org/linux-raid/CAPhsuW5bV+Bz=Od9jomNHoedaEMFAXymN11J80G62GVPwSp41g@mail.gmail.com/

Thanks,
Mariusz

And for raid1/raid10, it looks like fail_last_dev and FailFast want to do opposite things.
It can fail the last and it doesn't send a rewrite bio when fail_last_dev is true. Because the
last dev has been set faulty. There is no meaning to send the rewrite bio. So FailFast only
works when fail_last_dev is false.

Quoted 62 lines · Xiao Ni

Hi Marisuz

We don't need to consider MD_FAILFAST for raid456. Because only raid1 and raid10 support it.
MD_FAILFAST_SUPPORTED is only set in raid1_run/raid10_run. So LastDev only be useful for
raid1/raid10. It should be good to only check Faulty here.

Best Regards
Xiao

Quoted 48 lines · Mariusz Tkaczyk

Hi All,
During my work under failed arrays handling[1] improvements, I discovered potential issue with "failfast" and metadata writes. In commit message[2] Neil mentioned that:
"If we get a failure writing metadata but the device doesn't fail, it must be the last device so we re-write without FAILFAST".

Obviously, this is not true for RAID456 (again)[1] but it is also not true for RAID1 and RAID10 with "fail_las_dev"[3] functionality enabled.

I did a quick check and can see that setter for "LastDev" flag is called if "Faulty" on device is not set. I proposed some changes in the area in my patchset[4] but after discussion we decided to drop changes here. Current approach is not correct for all branches, so my proposal is to change:

drivers/md/md.c +1 -1

index 7b024912f1eb..3daec14ef6b2 100644
@@ -931,7 +931,7 @@ static void super_written(struct bio *bio)
                pr_err("md: %s gets error=%d\n", __func__,
                       blk_status_to_errno(bio->bi_status));
                md_error(mddev, rdev);
-               if (!test_bit(Faulty, &rdev->flags)
+               if (test_bit(MD_BROKEN, mddev->flag)
                    && (bio->bi_opf & MD_FAILFAST)) {
                        set_bit(MD_SB_NEED_REWRITE, &mddev->sb_flags);
                        set_bit(LastDev, &rdev->flags);

It will force "LastDev" to be set on every metadata rewrite if mddevice is known to be failed.
Do you have any other suggestions?

+ Guoqing - author of fail_last_dev.
+ Xiao - you are familiarized with FailFast so please take a look.

[1]https://lore.kernel.org/linux-raid/CAPhsuW54_9CTR6sh7mnQ6O77F2HNArLHGWHYsUdbNGy7pXgipQ@mail.gmail.com/T/#m8cf7c57429b6fd332220157186151900ce23865d [2]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=46533ff7fefb7e9e3539494f5873b00091caa8eb [3]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=9a567843f7ce [4]https://lore.kernel.org/linux-raid/CAPhsuW5bV+Bz=Od9jomNHoedaEMFAXymN11J80G62GVPwSp41g@mail.gmail.com/

Thanks,
Mariusz

After thinking for a while, my words from my last email don't describe properly. For raid1/raid10, if fail_last_dev is true. The bios which are sent to member disks all have MD_FAILFAST. If there are no errors, failfast works well until the last device failure. It will not re-send the bio without MD_FAILFAST when fail_last_dev is true, because the last device has been set faulty. There is no meaning to send the bio again in this situation. So it should be right to only check faulty flag here.

Quoted 74 lines

And for raid1/raid10, it looks like fail_last_dev and FailFast want to do opposite things.
It can fail the last and it doesn't send a rewrite bio when fail_last_dev is true. Because the
last dev has been set faulty. There is no meaning to send the rewrite bio. So FailFast only
works when fail_last_dev is false.

Quoted 64 lines · Xiao Ni

Hi Marisuz

We don't need to consider MD_FAILFAST for raid456. Because only raid1 and raid10 support it.
MD_FAILFAST_SUPPORTED is only set in raid1_run/raid10_run. So LastDev only be useful for
raid1/raid10. It should be good to only check Faulty here.

Best Regards
Xiao

Quoted 50 lines · Mariusz Tkaczyk

Hi All,
During my work under failed arrays handling[1] improvements, I discovered potential issue with "failfast" and metadata writes. In commit message[2] Neil mentioned that:
"If we get a failure writing metadata but the device doesn't fail, it must be the last device so we re-write without FAILFAST".

Obviously, this is not true for RAID456 (again)[1] but it is also not true for RAID1 and RAID10 with "fail_las_dev"[3] functionality enabled.

I did a quick check and can see that setter for "LastDev" flag is called if "Faulty" on device is not set. I proposed some changes in the area in my patchset[4] but after discussion we decided to drop changes here. Current approach is not correct for all branches, so my proposal is to change:

drivers/md/md.c +1 -1

index 7b024912f1eb..3daec14ef6b2 100644
@@ -931,7 +931,7 @@ static void super_written(struct bio *bio)
                pr_err("md: %s gets error=%d\n", __func__,
                       blk_status_to_errno(bio->bi_status));
                md_error(mddev, rdev);
-               if (!test_bit(Faulty, &rdev->flags)
+               if (test_bit(MD_BROKEN, mddev->flag)
                    && (bio->bi_opf & MD_FAILFAST)) {
                        set_bit(MD_SB_NEED_REWRITE,
 &mddev->sb_flags); set_bit(LastDev, &rdev->flags);

It will force "LastDev" to be set on every metadata rewrite if mddevice is known to be failed.
Do you have any other suggestions?

+ Guoqing - author of fail_last_dev.
+ Xiao - you are familiarized with FailFast so please take a look.

[1]https://lore.kernel.org/linux-raid/CAPhsuW54_9CTR6sh7mnQ6O77F2HNArLHGWHYsUdbNGy7pXgipQ@mail.gmail.com/T/#m8cf7c57429b6fd332220157186151900ce23865d [2]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=46533ff7fefb7e9e3539494f5873b00091caa8eb [3]https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=9a567843f7ce [4]https://lore.kernel.org/linux-raid/CAPhsuW5bV+Bz=Od9jomNHoedaEMFAXymN11J80G62GVPwSp41g@mail.gmail.com/

Thanks,
Mariusz

Hi All,
During my work under failed arrays handling[1]*improvements*,

I discovered potential issue with "failfast" and metadata writes. In commit message[2] Neil mentioned that:
"If we get a failure writing metadata but the device doesn't fail, it must be the last device so we re-write without FAILFAST".

Obviously, this is not true for RAID456 (again)[1] but it is also not true for RAID1 and RAID10 with "fail_las_dev"[3] functionality enabled.

I did a quick check and can see that setter for "LastDev" flag is called if "Faulty" on device is not set. I proposed some changes in the area in my patchset[4] but after discussion we decided to drop changes here. Current approach is not correct for all branches, so my proposal is to change:

drivers/md/md.c +0 -0

index 7b024912f1eb..3daec14ef6b2 100644