| Message ID | 20180801145608.19880-1-gpiccoli@canonical.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce emergency raid0 stop for mounted arrays | expand |
On 01/08/2018 22:51, NeilBrown wrote: >> [...] > If you have hard drive and some sectors or track stop working, I think > you would still expect IO to the other sectors or tracks to keep > working. > > For this reason, the behaviour of md/raid0 is to continue to serve IO to > working devices, and only fail IO to failed/missing devices. > Hi Neil, thanks for your quick response. I agree with you about the potential sector failure, it shouldn't automatically fail the entire array for a single failed write. The check I'm using in the patch is against device request queue - if a raid0 member queue is dying/dead, then we consider the device as dead, and as a consequence, the array is marked dead. In my understanding of raid0/stripping, data is split among N devices, called raid members. If one member is failed, for sure the data written to the array will be corrupted, since that "portion" of data going to the failed device won't be stored. Regarding the current behavior, one test I made was to remove 1 device of a 2-disk raid0 array and after that, write a file. Write completed normally (no errors from the userspace perspective), and I hashed the file using md5. I then rebooted the machine, raid0 was back with the 2 devices, and guess what? The written file was there, but corrupted (with a different hash). I don't think this is something fine, user could have written important data and don't realize it was getting corrupted while writing. > It seems reasonable that you might want a different behaviour, but I > think that should be optional. i.e. you would need to explicitly set a > "one-out-all-out" flag on the array. I'm not sure if this should cause > reads to fail, but it seems quite reasonable that it would cause all > writes to fail. > > I would only change the kernel to recognise the flag and refuse any > writes after any error has been seen. > I would use udev/mdadm to detect a device removal and to mark the > relevant component device as missing. > Using the udev/mdadm to notice a member has failed and the array must be stopped might work, it was my first approach. The main issue here is timing: it takes "some time" until userspace is aware of the failure, so we have a window in which writes were sent between (A) the array member failed/got removed and (B) mdadm notices and instruct driver to refuse new writes; between (A) and (B), those writes are seen as completed, since they are indeed complete (at least, they are fine from the page cache point of view). Then, writeback will try to write those, which will cause problems or they will complete in a corrupted form (the file will be present in the array's filesystem after array is restored, but corrupted). So, the in-kernel mechanism avoided most part of window (A)-(B), although it seems we still have some problems when nesting arrays, due to this same window, even with the in-kernel mechanism (given the fact it takes some time to remove the top array when a pretty "far" bottom-member is failed). More suggestions on how to deal with this in a definitive manner are highly appreciated. Thanks, Guilherme -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Aug 02 2018, Guilherme G. Piccoli wrote: > On 01/08/2018 22:51, NeilBrown wrote: >>> [...] >> If you have hard drive and some sectors or track stop working, I think >> you would still expect IO to the other sectors or tracks to keep >> working. >> >> For this reason, the behaviour of md/raid0 is to continue to serve IO to >> working devices, and only fail IO to failed/missing devices. >> > > Hi Neil, thanks for your quick response. I agree with you about the > potential sector failure, it shouldn't automatically fail the entire > array for a single failed write. > > The check I'm using in the patch is against device request queue - if a > raid0 member queue is dying/dead, then we consider the device as dead, > and as a consequence, the array is marked dead. > > In my understanding of raid0/stripping, data is split among N devices, > called raid members. If one member is failed, for sure the data written > to the array will be corrupted, since that "portion" of data going to > the failed device won't be stored. > > Regarding the current behavior, one test I made was to remove 1 device > of a 2-disk raid0 array and after that, write a file. Write completed > normally (no errors from the userspace perspective), and I hashed the > file using md5. I then rebooted the machine, raid0 was back with the 2 > devices, and guess what? > The written file was there, but corrupted (with a different hash). I > don't think this is something fine, user could have written important > data and don't realize it was getting corrupted while writing. In your test, did you "fsync" the file after writing to it? That is essential for data security. If fsync succeeded even though the data wasn't written, that is certainly a bug. If it doesn't succeed, then you know there is a problem with your data. > > >> It seems reasonable that you might want a different behaviour, but I >> think that should be optional. i.e. you would need to explicitly set a >> "one-out-all-out" flag on the array. I'm not sure if this should cause >> reads to fail, but it seems quite reasonable that it would cause all >> writes to fail. >> >> I would only change the kernel to recognise the flag and refuse any >> writes after any error has been seen. >> I would use udev/mdadm to detect a device removal and to mark the >> relevant component device as missing. >> > > Using the udev/mdadm to notice a member has failed and the array must be > stopped might work, it was my first approach. The main issue here is > timing: it takes "some time" until userspace is aware of the failure, so > we have a window in which writes were sent between > > (A) the array member failed/got removed and > (B) mdadm notices and instruct driver to refuse new writes; I don't think the delay is relevant. If writes are happening, then the kernel will get write error from the failed devices and can flag the array as faulty. If writes aren't happening, then it no important cost in the "device is removed" message going up to user-space and back. NeilBrown > > between (A) and (B), those writes are seen as completed, since they are > indeed complete (at least, they are fine from the page cache point of > view). Then, writeback will try to write those, which will cause > problems or they will complete in a corrupted form (the file will > be present in the array's filesystem after array is restored, but > corrupted). > > So, the in-kernel mechanism avoided most part of window (A)-(B), > although it seems we still have some problems when nesting arrays, > due to this same window, even with the in-kernel mechanism (given the > fact it takes some time to remove the top array when a pretty "far" > bottom-member is failed). > > More suggestions on how to deal with this in a definitive manner are > highly appreciated. > Thanks, > > > Guilherme -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Neil, sorry for my delay. On 02/08/2018 18:37, NeilBrown wrote: > On Thu, Aug 02 2018, Guilherme G. Piccoli wrote: >> [...] >> Regarding the current behavior, one test I made was to remove 1 device >> of a 2-disk raid0 array and after that, write a file. Write completed >> normally (no errors from the userspace perspective), and I hashed the >> file using md5. I then rebooted the machine, raid0 was back with the 2 >> devices, and guess what? >> The written file was there, but corrupted (with a different hash). I >> don't think this is something fine, user could have written important >> data and don't realize it was getting corrupted while writing. > > > In your test, did you "fsync" the file after writing to it? That is > essential for data security. > If fsync succeeded even though the data wasn't written, that is > certainly a bug. If it doesn't succeed, then you know there is a > problem with your data. > Yes, I did. After writing, I ran both "sync" and "sync -f" after "dd" command complete (with no errors). The sync procedures also finished without errors, and the file was there. After a reboot, though, the file has a different md5, since it was corrupted. >> [...] >> Using the udev/mdadm to notice a member has failed and the array must be >> stopped might work, it was my first approach. The main issue here is >> timing: it takes "some time" until userspace is aware of the failure, so >> we have a window in which writes were sent between >> >> (A) the array member failed/got removed and >> (B) mdadm notices and instruct driver to refuse new writes; > > I don't think the delay is relevant. > If writes are happening, then the kernel will get write error from the > failed devices and can flag the array as faulty. > If writes aren't happening, then it no important cost in the "device is > removed" message going up to user-space and back. The problem with the time between userspace notice something is wrong and "warn" the kernel to stop writes is that many writes will be sent to the device in this mean time, and they can completed later - handling async completions of dead devices proved to be tricky, at least in my approach. Also, writeback threads will be filled with I/Os to be written to the dead devices too, this is other part of the problem. If you have suggestions to improve my approach, or perhaps a totally different idea than mine, I highly appreciate the feedback. Thank you very much for the attention. Cheers, Guilherme > > NeilBrown > >> >> between (A) and (B), those writes are seen as completed, since they are >> indeed complete (at least, they are fine from the page cache point of >> view). Then, writeback will try to write those, which will cause >> problems or they will complete in a corrupted form (the file will >> be present in the array's filesystem after array is restored, but >> corrupted). >> >> So, the in-kernel mechanism avoided most part of window (A)-(B), >> although it seems we still have some problems when nesting arrays, >> due to this same window, even with the in-kernel mechanism (given the >> fact it takes some time to remove the top array when a pretty "far" >> bottom-member is failed). >> >> More suggestions on how to deal with this in a definitive manner are >> highly appreciated. >> Thanks, >> >> >> Guilherme -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 09/08/2018 20:17, Guilherme G. Piccoli wrote: > [..] > If you have suggestions to improve my approach, or perhaps a totally > different idea than mine, I highly appreciate the feedback. > > Thank you very much for the attention. > Cheers, > > > Guilherme > Hi Neil and Shaohua Li, sorry for the ping - do you have any news on this? Any feedback is much appreciated. Cheers, Guilherme -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Currently the md driver completely relies in the userspace to stop an array in case of some failure. There's an interesting case for raid0: if we remove a raid0 member, like PCI hot(un)plugging an NVMe device, and the raid0 array is _mounted_, mdadm cannot stop the array, since the tool tries to open the block device (to perform the ioctl) with O_EXCL flag. So, in this case the array is still alive - users may write to this "broken-yet-alive" array and unless they check the kernel log or some other monitor tool, everything will seem fine and the writes are completed with no errors. Being more precise, direct writes will not work, but since usually writes are done in a regular form, i.e., backed by the page cache, the most common scenario is an user being able to regularly write to a broken raid0, and get all their data corrupted. PROPOSAL: The idea proposed here to fix this behavior is mimic other block devices: if one have a filesystem mounted in a block device on top of an NVMe or SCSI disk and the disk gets removed, writes are prevented, errors are observed and it's obvious something is wrong. Same goes for USB sticks, which are sometimes even removed physically from the machine without getting their filesystem unmounted before. We believe right now the md driver is not behaving properly for raid0 arrays (it is handling these errors for other levels though). The approach took for raid-0 is basically an emergency removal procedure, in which I/O is blocked from the device, the regular clean-up happens and the associate disk is deleted. It went to extensive testing, as detailed below. Not all are roses, we have some caveats that need to be resolved. Feedback is _much appreciated_. There is a caveats / questions / polemic choices section below the test section. Thanks in advance, Guilherme * Testing: The device topology for the tests is as follows: md6 | |******************************| | | md4 md5 | | |*************| |*************| | | | | md0 md2 md1 md3 | | | | |*******| |***| |***| |*******| | | | | | | | | nvme1n1 nvme0n1 sda sdd sde sdf nvme2n1 nvme3n1 We chose to test such complex topology to prevent corner cases and timing issues (which were found in the development phase). There are 3 test subsets basically: the whole set of devices, called here "md cascade", and 2 small branches, called here "md direct" testing. So, in summary: ### md direct (single arrays composed of SCSI/NVMe block devices) ### A1 = md1 A2 = md3 C1 = sde C1 = nvme2n1 C2 = sdf C2 = nvme3n1 ### md cascade (array composed of md devices) ### A3 = md6 underlying component UC1 = nvme1n1 underlying component UC2 = sdd underlying component UC3 = sdf underlying component UC4 = nvme2n1 ### Method of removal ### - For NVMe devices, "echo 1 > /sys/block/nvmeXnY/device/device/remove" - For SCSI devices, "echo 1 > /sys/block/sdX/device/delete" When tests are marked with *, it means PCI/disk hotplug was exercised too, using the qemu hotplug feature. ### Test ### Write a file using the command: "dd if=/dev/zero of=tmpfile bs=X" where X might be 1M or 4K - we've experimented with both. Each array also contains a valid file to be checked later, in order to validate that filesystem didn't get severely corrupted after the procedure. Tests marked with @ indicate direct writes were tested too. Tests with a [] indicator exhibited some oddity/issue, detailed in the caveats section. After each test, guest was rebooted. We also tested in some cases re-adding the previously removed SCSI/NVMe device (after unmounting the previous mount points) and restarting the arrays - worked fine. * Test results ("partition X" means we have a GPT table with 2 partitions in the device) ### md direct Remove members and start writing to array right after: A1 with: A2 with: -ext4 -ext4 --Removed C1: PASSED @ --Removed C2: PASSED @ -xfs -xfs --Removed C2: PASSED @ --Removed C1: PASSED @ -ext2 -btrfs --Removed C1: PASSED --Removed C2: PASSED -partition 1 + ext4 -partition 1 + xfs -partition 2 + xfs -partition 2 + ext4 --Removed C1: PASSED @ --Removed C2: PASSED @ -LVM + xfs -LVM + ext4 --Removed C2: PASSED --Removed C1: PASSED Start writing to array and remove member right after: A1 with: A2 with: -ext4 -ext4 --Removed C1: PASSED @ --Removed C1: PASSED @ --Removed C2: PASSED *@ -xfs -xfs --Removed C1: PARTIAL @ [(a)] --Removed C1: PASSED * --Removed C2: PASSED *@ -ext2 -btrfs --Removed C2: PASSED @ --Removed C1: PASSED @ -partition 1 + ext4 -partition 1 + xfs -partition 2 + xfs -partition 2 + ext4 --Removed C2: PARTIAL @ [(a)] --Removed C1: PASSED *@ -LVM + xfs -LVM + ext4 --Removed C1: PASSED --Removed C2: PASSED -fuse (NTFS) --Removed C2: PASSED ### md cascade Remove members and start writing to array right after: A3 with: -ext4: --Removed UC2: PASSED @ --Removed UC4: PASSED @ -xfs: --Removed UC2: PASSED @ --Removed UC4: PASSED @ -partition 1 + ext4 -partition 2 + xfs --Removed UC1: PASSED --Removed UC3: PASSED -ext2: --Removed UC3: PASSED @ -btrfs: --Removed UC1: PARTIAL [(d)] Start writing to array and remove member right after: A3 with: -ext4: --Removed UC2: PARTIAL @ [(a), (b)] --Removed UC4: PARTIAL @ [(b), (d)] -xfs: --Removed UC2: PARTIAL @ [(a), (c)] --Removed UC4: PARTIAL *@ [(c), (d)] -partition 1 + ext4 -partition 2 + xfs --Removed UC1: PASSED * --Removed UC3: PASSED -ext2: --Removed UC3: PASSED @ -btrfs: --Removed UC1: PARTIAL @ [(d)] * Caveats / points of uncertainty / questions: a) When using SCSI array members, depending "when" the raid member gets removed, raid0 driver might not quickly observe a failure in the queue, because the requests may be all "scheduled" in the SCSI cache to be flushed to the device. In other words, depending on the timing, the mechanism used in this patch to detect a failed array member will take some time to be triggered. It does not happen in direct I/O mode nor with NVMe - it's related with the SCSI cache sync. As soon a new I/O is queued in the md device, that triggers the array removal. #For md cascade only (nested arrays): b) In the case of direct I/Os, we might face an ext4 journal task blocked in io_schedule(). Stack trace: https://pastebin.ubuntu.com/p/RZ4qFvJjy2 c) Hung task in xfs after direct I/O only, when trying to unmount [xlog_wait() or xlog_dealloc_log()]. Stack trace: https://pastebin.ubuntu.com/p/fq3G45jHX2 d) Some arrays in the nested scenario still show in /sys/block after the removal. #Generic questions / polemic choices: i) With this patch, the STOP_ARRAY ioctl won't proceed in case a disk is removed and emergency stop routine already started to act, even in case of unmounted md arrays. This is a change in the old behavior, triggered by the way we check for failed members in raid0 driver. ii) Currently, the patch implements a kernel-only removal policy - shall it rely on userspace (mdadm) to do it? A first approach was based in userspace, but it proved to be more problematic in tests. iii) The patch implemented the md_delayed_stop() function to perform the emergency stop - should we refactor do_md_stop() instead, to encompass the emergency delayed stop minimal code? Guilherme G. Piccoli (1): md/raid0: Introduce emergency stop for raid0 arrays drivers/md/md.c | 126 +++++++++++++++++++++++++++++++++++++++++---- drivers/md/md.h | 6 +++ drivers/md/raid0.c | 14 +++++ 3 files changed, 136 insertions(+), 10 deletions(-)