On 2021/03/01 21:30, Damien Le Moal wrote: > On 2021/03/01 20:33, Guido Trentalancia wrote: >> I have just checked the drive and I can now confirm that you are right >> about the capabilities reported: it is indeed reporting Write Caching. >> >> However the point is that the current kernel behaviour is wrong and >> leading to data corruption on such drives, as the sync function fails >> due to missing Synchronize Cache command. > > Without sync working, how could you ever guarantee that even a clean shutdown of > the host does not result in data loss ? I am not even talking about power > failures and crashes here. A simple, clean "shutdown -h now". Without issuing a > synchronize cache command after flushing the page cache and unmounting the FS, > you will loose data and corrupt things. 100% guaranteed. > > As James explained in different terms, it is the other way around: the kernel is > correct and behaves according to the fact that the drive is saying "I am caching > written sectors". That implies that synchronize cache *is* supported, per the > standards. The corruption errors you are seeing are due to the drive being silly > and failing synchronize cache commands, resulting in the cached data *not* going > to persistent media, and loss of data on power down or crash. This is not a bad > behavior. On the contrary, not seeing any data corruption would only mean that > you are being lucky. > >> Once again, this is because of very old HD specifications that were >> implementing Write Caching without that command. > > If the drive is scanned and initialized by sd/libata, it means that it is > reporting following a supported standard version (SPC, SBC, ACS). Probably an > old version, but still a standard. Synchronize cache command (for scsi) and > flush cache/flush cache ext (for ATA) are not recent additions to SCSI & ATA. > These have been around for a long time. It does not matter that the drive is old > and following an old version. It should support cache flushing. Refer to the ACS > specs. It is clearly noted that: "If the volatile write cache is > disabled or no volatile write cache is present, the device shall indicate > command completion without error.". Get the point ? That drive firmware is > simply broken and missing a critical command. > >> The way forward is to treat the command failure as non-critical (see >> attached patch) because clearly it's not implemented in all drives, but >> only more recent ones. > > Nope. Simply disable the write cache. Try "hdparm -W 0 /dev/sdX" or "sdparm > --clear=WCE /dev/sdX" to disable it. And a udev rule can do that for you on boot > too. Failures and data corruption will go away. But the performance will likely > go to the trash bin too... > > A little bit of history: it used to be a thing, many many years ago, to see > drives that had synchronize cache commands implemented as "nop" or not > implemented at all. Unscrupulous vendors would do that to get better performance > results for their drives with benchmarks. Because synchronize cache can be very > costly and can take several seconds to complete. Using such drives in big RAID > arrays with power failure protections would be fine, but any other use case by > any regular user would create data corruption situations very quickly. As noted > above, even a simple clean shutdown would lose data ! Such bad practice ended > fairly quickly. For some reasons these bad drives failed to sell very well :) I checked the standards again. It turns out that SYNCHRONIZE CACHE command is optional in SBC... Aouch. Got so used to have that one on any drive that I thought it was mandatory. Well, it certainly is mandatory if the drive has a write cache, which seems to be the case for you. The problem with your patch though is that you disable write caching when you see an ILLEGAL REQUEST/INVALID OPCODE termination of synchronize cache. Which means that the drive was already used, written too and the cache has likely dirty data and I do not see any method to guarantee that that data makes it to persistent media before shutdown. Imagine if that was the synchronize cache sent before shutdown. So the only reasonable solution for such drive is to use it with write cache disabled from the start. > >> >> Guido >> >> On Mon, 01/03/2021 at 07.38 +0000, Damien Le Moal wrote: >>> On 2021/03/01 16:12, Guido Trentalancia wrote: >>>> Hi James, >>>> >>>> thanks for getting back on this issue. >>>> >>>> I have tested this patch for over a year and it works flawlessly >>>> without any data corruption ! >>>> >>>> On such kind of drives the actual situation is just the opposite as >>>> you >>>> describe: data corruption occurs when not using this patch ! >>>> >>>> I do not agree with you: if a drive does not support Synchronize >>>> Cache >>>> command, there is no point in treating the failure as critical and >>>> by >>>> all means the failure must be ignored, as there is nothing which >>>> can be >>>> done about it and it should not be treated as a failure ! >>> >>> If the drive does not support synchronize cache, then the drive >>> should *not* >>> report write caching either. If it does, then the kernel will issue >>> synchronize >>> cache commands and that command failing indicates the drive is >>> broken/lying == >>> junk and should not be trusted. >>> >>> The user can trivially remedy to this situation by force disabling >>> the write >>> cache: no more synchronize cache commands will be issued and no more >>> failures. >>> No need to patch the kernel for that. And if the drive does not allow >>> disabling >>> write caching, then I seriously recommend replacing it :) >>> >>>> >>>> However, if you are willing to propose an alternative patch, I'd be >>>> happy to test it and report back, as long as this bug is fixed in >>>> the >>>> shortest time possible. >>>> >>>> Guido >>>> >>>> On Sun, 28/02/2021 at 08.37 -0800, James Bottomley wrote: >>>>> On Sun, 2021-02-28 at 10:01 +0100, Guido Trentalancia wrote: >>>>>> Many obsolete hard drives do not support the Synchronize Cache >>>>>> SCSI >>>>>> command. Such command is generally issued during fsync() calls >>>>>> which >>>>>> at the moment therefore fail with the ILLEGAL_REQUEST sense >>>>>> key. >>>>> >>>>> It should be that all drives that don't support sync cache also >>>>> don't >>>>> have write back caches, which means we don't try to do a cache >>>>> sync >>>>> on >>>>> them. The only time you we ever try to sync the cache is if the >>>>> device >>>>> advertises a write back cache, in which case the sync cache >>>>> command >>>>> is >>>>> mandatory. >>>>> >>>>> I'm sure some SATA manufacturers somewhere cut enough corners to >>>>> produce an illegally spec'd drive like this, but your proposed >>>>> remedy >>>>> is unviable: you can't ignore a cache failure on flush barriers >>>>> which >>>>> will cause data corruption. You have to disable barriers on the >>>>> filesystem to get correct operation and be very careful about >>>>> power >>>>> down. >>>>> >>>>> James >>>>> >>>>> >>> >>> >> > > -- Damien Le Moal Western Digital Research
