On Fri, Jun 16 2017, Shaohua Li wrote: > On Fri, Jun 16, 2017 at 01:26:00PM +1000, Neil Brown wrote: >> On Fri, Jun 09 2017, Shaohua Li wrote: >> >> > From: Shaohua Li <shli@xxxxxx> >> > >> > suspend range is controlled by userspace. If userspace doesn't clear suspend >> > range, it's possible a thread will wait for the range forever, we can't even >> > kill it. This is bad behavior. Add a timeout in the wait. If timeout happens, >> > we return IO error. The app controlling suspend range looks like part of disk >> > firmware, if disk isn't responded for a long time, timed out IO error is >> > returned. >> > >> > A simple search in SCSI code shows maximum IO timeout is 120s, so I use this >> > value here too. >> >> I really don't like this. It is an API change with no really >> justification. Has the current behavior caused a problem? > > This centainly causes problem. Set the suspend range will make application > stall for ever, don't you think this is a problem? I agree that it could cause a problem. I'm asking it is actually, in practice, causes a problem. Do you have reports from people saying "the IO to my RAID array is hanging, what is wrong?" and you look into it and find out that suspend_hi is larger than suspend_lo? And if that does happen, is this really the best way to fix it? > >> Both md and dm export APIs which allow IO to be suspended while >> changes are made. Changing that to a timed-out period needs, I think, >> to be clearly justified. >> >> If it is changed to a timed-out period, then that should be explicit, >> rather than having each request independently time out. >> i.e. when the suspend is initiated, the end-time should be computed, and >> any IO would block until that time, not block for some number of >> seconds. > > Ok, this makes sense. We can add a timeout. If it's expired, we clear suspend > range. Userspace should know what they are doing. > >> >> If an md device is left suspended, then the current code will block IO >> indefinitely. This patch will at a 20minute times to every single >> request, which will mean IO proceeds, but extremely slowly. I don't see >> that as a useful improvement. > > It returns error, so application will not dispatch more IO. But I agree a > timeout to clear the suspend looks a better policy. Write errors only get back to the application if it calls fsync(), and many don't do that. Write errors can easily cause a filesystem to go read-only, and require an fsck. I think we should be very cautious about triggering write errors. NFS will hang indefinitely rather then return an error if the server is not available. That can certainly be annoying, but the alternative has been tried, and it leads to random data corruption. The two cases are only comparable at a very high level, but I think this result should encourage substantial caution. NeilBrown
Attachment:
signature.asc
Description: PGP signature
