Bug 837324
| Summary: | [virtio-win][viostor]viostor reports support for FUA, but does not implement it | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Red Hat Enterprise Linux 7 | Reporter: | Paolo Bonzini <pbonzini> | ||||
| Component: | virtio-win | Assignee: | Vadim Rozenfeld <vrozenfe> | ||||
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Virtualization Bugs <virt-bugs> | ||||
| Severity: | unspecified | Docs Contact: | |||||
| Priority: | unspecified | ||||||
| Version: | 7.0 | CC: | acathrow, asad.saeed, bcao, bsarathy, hhuang, juzhang, kwolf, mdeng, michen, pbonzini, rhod, shuang, syeghiay, virt-maint, vrozenfe, xigao | ||||
| Target Milestone: | rc | ||||||
| Target Release: | 7.0 | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Whiteboard: | |||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||
| Doc Text: | Story Points: | --- | |||||
| Clone Of: | Environment: | ||||||
| Last Closed: | 2014-06-13 13:21:22 UTC | Type: | Bug | ||||
| Regression: | --- | Mount Type: | --- | ||||
| Documentation: | --- | CRM: | |||||
| Verified Versions: | Category: | --- | |||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||
| Embargoed: | |||||||
| Attachments: |
|
||||||
(In reply to comment #0) > Description of problem: > viostor processes flushes by first completing the SRB, and then sending a > VIRTIO_BLK_T_FLUSH request. This is wrong because and could cause data You must be using an old version of viostor driver. > loss, for example: > > write 1 > -> VIRTIO_BLK_T_OUT > <- processed > CompleteSRB > synchronize_cache > CompleteSRB > -> VIRTIO_BLK_T_FLUSH > write 2 > -> VIRTIO_BLK_T_OUT > <- processed > CompleteSRB > <<<<power loss or QEMU crash>>>> > > Now it could happen that write 2 will reach the disk but write 1 didn't, > contrarily to the expectations of the guest OS. > > This could be more visible with qcow2, because flushes also write the qcow2 > metadata. It can happen for example whenever write 2 doesn't need a > metadata write but write 1 needed one. > > Related to this, the viostor driver reports MODE_DSP_FUA_SUPPORTED but it > doesn't look at the DPO or FUA bits (respectively in reads and writes). The We were not able to pass WHQL without setting FUA flag on. So, we'd better to add FUA support to QEMU to make Windows guests happy. > bit should not be included in the MODE SENSE output, since there is no > equivalent in the virtio-blk protocol. > > viostor processes flushes by first completing the SRB, and then sending a > > VIRTIO_BLK_T_FLUSH request. This is wrong because and could cause data > > You must be using an old version of viostor driver. I'm looking at the github source: case SCSIOP_SYNCHRONIZE_CACHE: case SCSIOP_SYNCHRONIZE_CACHE16: { Srb->SrbStatus = SRB_STATUS_SUCCESS; Srb->ScsiStatus = SCSISTAT_GOOD; CompleteSRB(DeviceExtension, Srb); if (adaptExt->flush_state == FlushIdle) { adaptExt->flush_state = FlushRequested; } return TRUE; } > > Related to this, the viostor driver reports MODE_DSP_FUA_SUPPORTED but it > > doesn't look at the DPO or FUA bits (respectively in reads and writes). The > > We were not able to pass WHQL without setting FUA flag on. Interesting... it passes for Xen, but then that driver sets WriteCacheEnable == FALSE in the mode sense response. What was the failure? Hopefully running virtio-scsi WHQL tests (so that one layer is eliminated) will also shed more light on the issue. > So, we'd better to add FUA support to QEMU to make Windows guests > happy. You can emulate FUA in the driver. Send a flush as soon as the write completes, and only complete the SRB after it returns. You do not need support in QEMU. (In reply to comment #3) > > > viostor processes flushes by first completing the SRB, and then sending a > > > VIRTIO_BLK_T_FLUSH request. This is wrong because and could cause data > > > > You must be using an old version of viostor driver. > > I'm looking at the github source: > > case SCSIOP_SYNCHRONIZE_CACHE: > case SCSIOP_SYNCHRONIZE_CACHE16: { > Srb->SrbStatus = SRB_STATUS_SUCCESS; > Srb->ScsiStatus = SCSISTAT_GOOD; > CompleteSRB(DeviceExtension, Srb); > if (adaptExt->flush_state == FlushIdle) { > adaptExt->flush_state = FlushRequested; > } > return TRUE; > } > > I don't own any repository at github. Our code looks like: case SCSIOP_SYNCHRONIZE_CACHE: case SCSIOP_SYNCHRONIZE_CACHE16: { Srb->SrbStatus = SRB_STATUS_PENDING; Srb->ScsiStatus = SCSISTAT_GOOD; if (!RhelDoFlush(DeviceExtension, Srb)) { Srb->SrbStatus = SRB_STATUS_ERROR; CompleteSRB(DeviceExtension, Srb); } return TRUE; > > > Related to this, the viostor driver reports MODE_DSP_FUA_SUPPORTED but it > > > doesn't look at the DPO or FUA bits (respectively in reads and writes). The > > > > We were not able to pass WHQL without setting FUA flag on. > > Interesting... it passes for Xen, but then that driver sets WriteCacheEnable > == FALSE in the mode sense response. What was the failure? > One of the scsi compliance tests failed. > Hopefully running virtio-scsi WHQL tests (so that one layer is eliminated) > will also shed more light on the issue. > > > So, we'd better to add FUA support to QEMU to make Windows guests > > happy. > > You can emulate FUA in the driver. Send a flush as soon as the write > completes, and only complete the SRB after it returns. You do not need > support in QEMU. Should we craft workarounds in kernel space for something that should be done in QEMU? Maybe we should detect FUA on write request and pass it as additional parameter with VIRTIO_BLK_T_OUT type and perform flush in QEMU right after processing write request? > I don't own any repository at github. Where is your repository? > case SCSIOP_SYNCHRONIZE_CACHE: > case SCSIOP_SYNCHRONIZE_CACHE16: { > Srb->SrbStatus = SRB_STATUS_PENDING; > Srb->ScsiStatus = SCSISTAT_GOOD; > if (!RhelDoFlush(DeviceExtension, Srb)) { > Srb->SrbStatus = SRB_STATUS_ERROR; > CompleteSRB(DeviceExtension, Srb); > } > return TRUE; Even this doesn't wait for the flush to complete, no? RhelDoFlush returns as soon as it kicks the host. > > You can emulate FUA in the driver. Send a flush as soon as the write > > completes, and only complete the SRB after it returns. You do not need > > support in QEMU. > > Should we craft workarounds in kernel space for something that should be done > in QEMU? Maybe we should detect FUA on write request and pass it as > additional parameter with VIRTIO_BLK_T_OUT type and perform flush in QEMU > right after processing write request? We could, but its addition was rejected because it wouldn't provide any performance improvement. (In reply to comment #4) > (In reply to comment #3) > > You can emulate FUA in the driver. Send a flush as soon as the write > > completes, and only complete the SRB after it returns. You do not need > > support in QEMU. > > Should we craft workarounds in kernel space for something that should be > done in QEMU? Yes. You have a piece of hardware that doesn't support FUA, so if you write a driver for it you need to work around that limitation. That this piece of hardware is virtual doesn't really change the situation. Of course, you can ask the hardware vendor (i.e. qemu) to implement FUA in newer revisions of the hardware, but that doesn't make older versions of the hardware disappear, so you would have to support them anyway. (In reply to comment #5) > > I don't own any repository at github. > > Where is your repository? git://git.engineering.redhat.com/users/vrozenfe/internal-kvm-guest-drivers-windows/.git > > > case SCSIOP_SYNCHRONIZE_CACHE: > > case SCSIOP_SYNCHRONIZE_CACHE16: { > > Srb->SrbStatus = SRB_STATUS_PENDING; > > Srb->ScsiStatus = SCSISTAT_GOOD; > > if (!RhelDoFlush(DeviceExtension, Srb)) { > > Srb->SrbStatus = SRB_STATUS_ERROR; > > CompleteSRB(DeviceExtension, Srb); > > } > > return TRUE; > > Even this doesn't wait for the flush to complete, no? RhelDoFlush returns > as soon as it kicks the host. If we succeed to add and kick, then we complete SRB from inside of ISR handler. > > > > You can emulate FUA in the driver. Send a flush as soon as the write > > > completes, and only complete the SRB after it returns. You do not need > > > support in QEMU. > > > > Should we craft workarounds in kernel space for something that should be done > > in QEMU? Maybe we should detect FUA on write request and pass it as > > additional parameter with VIRTIO_BLK_T_OUT type and perform flush in QEMU > > right after processing write request? > > We could, but its addition was rejected because it wouldn't provide any > performance improvement. But IIUC, FUA is mostly for data consistency not for performance improvement. (In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > You can emulate FUA in the driver. Send a flush as soon as the write > > > completes, and only complete the SRB after it returns. You do not need > > > support in QEMU. > > > > Should we craft workarounds in kernel space for something that should be > > done in QEMU? > > Yes. You have a piece of hardware that doesn't support FUA, so if you write > a driver for it you need to work around that limitation. That this piece of > hardware is virtual doesn't really change the situation. > > Of course, you can ask the hardware vendor (i.e. qemu) to implement FUA in > newer revisions of the hardware, but that doesn't make older versions of the > hardware disappear, so you would have to support them anyway. It's what I usually do, when I write a device driver for a broken piece of HW. But in this particular case, why not to fix it in QEMU? It will be simpler from any points of view. (In reply to comment #8) > (In reply to comment #6) > > Yes. You have a piece of hardware that doesn't support FUA, so if you write > > a driver for it you need to work around that limitation. That this piece of > > hardware is virtual doesn't really change the situation. > > > > Of course, you can ask the hardware vendor (i.e. qemu) to implement FUA in > > newer revisions of the hardware, but that doesn't make older versions of the > > hardware disappear, so you would have to support them anyway. > > It's what I usually do, when I write a device driver for a broken piece of > HW. > But in this particular case, why not to fix it in QEMU? It will be simpler > from any points of view. It doesn't help you. Even if the virtio spec is updated to include FUA support and even if qemu supports it in 1.2 and newer, you will still have old versions of qemu (and other virtio-blk implementations outside qemu) that don't have the new support, both upstream and at least RHEL 5. You cannot simply stop supporting those. So your situation is really exactly the same as with real hardware. (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > Yes. You have a piece of hardware that doesn't support FUA, so if you write > > > a driver for it you need to work around that limitation. That this piece of > > > hardware is virtual doesn't really change the situation. > > > > > > Of course, you can ask the hardware vendor (i.e. qemu) to implement FUA in > > > newer revisions of the hardware, but that doesn't make older versions of the > > > hardware disappear, so you would have to support them anyway. > > > > It's what I usually do, when I write a device driver for a broken piece of > > HW. > > But in this particular case, why not to fix it in QEMU? It will be simpler > > from any points of view. > > It doesn't help you. Even if the virtio spec is updated to include FUA > support and even if qemu supports it in 1.2 and newer, you will still have > old versions of qemu (and other virtio-blk implementations outside qemu) > that don't have the new support, both upstream and at least RHEL 5. You > cannot simply stop supporting those. > > So your situation is really exactly the same as with real hardware. I see your point. But still, speaking practically, I don't see any reason in implementing pseudo-FUA feature inside of Windows miniport driver because it's going to be such performance killer that no one will like it. (In reply to comment #10) > I see your point. But still, speaking practically, I don't see any reason in > implementing pseudo-FUA feature inside of Windows miniport driver because > it's going to be such performance killer that no one will like it. If you advertise it, you must implement it. Silently ignoring a FUA is a bug that harms data integrity. And I think Paolo's point was that sending an explicit flush or real FUA handling inside qemu wouldn't make a big performance difference anyway. If you flush something to disk, your performance is killed anyway... (In reply to comment #11) > (In reply to comment #10) > > I see your point. But still, speaking practically, I don't see any reason in > > implementing pseudo-FUA feature inside of Windows miniport driver because > > it's going to be such performance killer that no one will like it. > > If you advertise it, you must implement it. Silently ignoring a FUA is a bug > that harms data integrity. > > And I think Paolo's point was that sending an explicit flush or real FUA > handling inside qemu wouldn't make a big performance difference anyway. If Of course it will. You need to send the same request twice to qemu. First time as a write request and as a flush request the second time. > you flush something to disk, your performance is killed anyway... > > Even this doesn't wait for the flush to complete, no? RhelDoFlush returns > > as soon as it kicks the host. > > If we succeed to add and kick, then we complete SRB from inside of ISR handler. Sorry, I couldn't read; changing the bug subject. That was fixed for bug 811161. Thanks for the pointer to the repository. > > We could, but its addition was rejected because it wouldn't provide any > > performance improvement. > > But IIUC, FUA is mostly for data consistency not for performance improvement. Yes, both flushing and FUA are for data consistency. However, using FUA-writes rather than just flushes also helps performance, because FUA-writes let the upper levels control which writes are flushed and which aren't. Filesystems can FUA-write metadata, and do regular writes for data, so that they only go to disk for a small part of the data. > I see your point. But still, speaking practically, I don't see any reason in > implementing pseudo-FUA feature inside of Windows miniport driver because it's > going to be such performance killer that no one will like it. Then you're trading performance for data consistency. Linux implements pseudo-FUA in the generic block layer (see block/blk-flush.c); virtio uses it and doesn't have such huge performance problems. Anyhow, the first thing to do is to understand the WHQL failure. If we can remove the FUA bit from the mode sense output, we shouldn't need any emulation. If Windows issued FUA writes, we should ask MSFT why, and whether ignoring the FUA bit is safe. Ignoring the FUA bit is not safe. I have patched both qemu-kvm and the virtio-stor driver for windows to support pseudo FUA. Prior to windows 8, microsoft still does not flush if FUA bit is not supported. https://github.com/asadpanda/kvm-guest-drivers-windows/tree/fua-support Could Anyone provide how to verify this bug ? sg3_utils is ported to Windows and should be able to send (emulated) SCSI commands to virtio-blk disks. The same procedure used for bug 814084 should work to verify this bug. Hi Mike, The issue problem should be fixed in build 35 available at http://download.devel.redhat.com/brewroot/packages/virtio-win-prewhql/0.1/35/win/virtio-win-prewhql-0.1.zip. Unfortunately, there is no WMI support in viostor driver at the moment, and it can take some time to add it in. Best regards, Vadim. dawu ,pls verify this bug on virtio-win-prewhql-41 I have reverted the relevant patch in build 43 http://download.devel.redhat.com/brewroot/packages/virtio-win-prewhql/0.1/43/win/virtio-win-prewhql-0.1.zip and asked to postpone the fix until rhel6.5 (In reply to comment #23) > I have reverted the relevant patch in build 43 > http://download.devel.redhat.com/brewroot/packages/virtio-win-prewhql/0.1/43/ > win/virtio-win-prewhql-0.1.zip > and asked to postpone the fix until rhel6.5 Hi, Vadim May I know any reason revert it ? WHQL ? Thanks, Mike (In reply to comment #24) > (In reply to comment #23) > > I have reverted the relevant patch in build 43 > > http://download.devel.redhat.com/brewroot/packages/virtio-win-prewhql/0.1/43/ > > win/virtio-win-prewhql-0.1.zip > > and asked to postpone the fix until rhel6.5 > > Hi, Vadim > > May I know any reason revert it ? WHQL ? Hi Mike, I really wasn't satisfied with the implementation. In addition, there were some problems, relieved during automation testing. Best regards, Vadim. > > Thanks, > Mike QE tested the bug with build 83 on HCK and submitted job named SCSI Compliance Test 2.0 mentioned in comment 32.The job could pass but there was warning messages in the logs so could you please have a look ? Pasted the warning messages and upload the HCK file to the bug.Any issues please let me know. Warning messages Index: 2477583668 Current: ASSERTION: INQUIRY Test for error when PAGE CODE field is nonzero and EVPD=0. Parent: WTTLOG Message 5/22/2014 10:13:46.099 AM Error sending SCSI command. GLE: 0x0000045d Message 5/22/2014 10:13:46.099 AM SCSI command sent to device was Message 5/22/2014 10:13:46.099 AM READ 6 Command: Message 5/22/2014 10:13:46.099 AM 08 00 00 01 02 00 Message 5/22/2014 10:13:46.099 AM READ (6) command not supported Message 5/22/2014 10:13:46.099 AM Fatal error, skipping all remaining READ6 test cases. Warning 5/22/2014 10:13:46.099 AM File: testsrc\driverstest\storage\wdk\scsicompliance\lib\core\policyscenario.cpp Line: 207 End Test 5/22/2014 10:13:46.099 AM ASSERTION: READ (6) Basic Verification Test Result: Warn Repro: scsicompliance.exe /device \\.\PhysicalDrive1 /verbosity 4 /scenario common /operation test Message 5/22/2014 10:13:46.099 AM Policy: IF_IMPLEMENTED Message 5/22/2014 10:13:46.099 AM Unit test Policy: Test Not Implemented: CDB was detected not to be implemented Start Test 5/22/2014 10:13:46.099 AM ASSERTION: WRITE (6) Basic Functionality Test Message 5/22/2014 10:13:46.099 AM Description: The command writes one block of data to device correctly.This test compares the data we want to write and the one returned by the READ after the write operation. If the data is the same, this implies that the WRITE command writes the data to disk correctly. Message 5/22/2014 10:13:46.099 AM Reference: SCSI Block Commands - 2 (SBC-2) Revision 16 (or published) specification Section 5.24 http://www.t10.org/ftp/t10/drafts/sbc2/sbc2r16.pdf Message 5/22/2014 10:13:46.099 AM Expectation: ScsiStatus == 0x0. Message 5/22/2014 10:13:46.099 AM Rationale: Some of the applications still use Write 6 and haven't transitioned into Write 10. Therefore we check if this command is implemented and proceed with testing. Message 5/22/2014 10:13:46.099 AM Error sending SCSI command. GLE: 0x0000045d Context _ _ Context Index: 312632662 Current: ASSERTION: INQUIRY Retrieving standard inquiry data. Parent: WTTLOG Message 5/22/2014 10:13:46.099 AM SCSI command sent to device was Message 5/22/2014 10:13:46.099 AM WRITE6 Command: Message 5/22/2014 10:13:46.099 AM 0a 00 00 00 01 00 Message 5/22/2014 10:13:46.099 AM WRITE (6) command not supported Message 5/22/2014 10:13:46.099 AM Fatal error, skipping all remaining WRITE6 test cases. Warning 5/22/2014 10:13:46.099 AM File: testsrc\driverstest\storage\wdk\scsicompliance\lib\core\policyscenario.cpp Line: 207 End Test 5/22/2014 10:13:46.099 AM ASSERTION: WRITE (6) Basic Functionality Test Result: Warn Repro: scsicompliance.exe /device \\.\PhysicalDrive1 /verbosity 4 /scenario common /operation test Message 5/22/2014 10:13:46.099 AM Policy: REQUIRED Created attachment 898181 [details]
Log
(In reply to dengmin from comment #34) > Created attachment 898181 [details] > Log Hi Min, FUA looks good: "End: Pass, ASSERTION: WRITE (10) FUA Test" Best regards, Vadim. Policy: REQUIRED Start: ASSERTION: WRITE (10) FUA Test, TUID= Description: The command writes data to disk correctly with cache and FUA (Force Unit Access) on. Reference: SCSI Block Commands - 2 (SBC-2) Revision 16 (or published) specification Section 5.25 http://www.t10.org/ftp/t10/drafts/sbc2/sbc2r16.pdf Expectation: Data is written correctly to disk with FUA on. Checksums of all WRITE (10) are correct. Sending SCSI command to device. Mode Sense 6 Command: 1a 08 08 00 ff 00 DeviceIoControl SUCCEEDED. SCSI status: 0x00 (GOOD) Sense bytes: 0x00 Data bytes: 0x10 00 04 08 0C --------------------------------------------------------------- 000 0F001000 080A0400 00000000 00000000 ................ == DONE ============================================================== Sending SCSI command to device. Mode Sense 6 Command: 1a 08 48 00 ff 00 DeviceIoControl SUCCEEDED. SCSI status: 0x00 (GOOD) Sense bytes: 0x00 Data bytes: 0x10 00 04 08 0C --------------------------------------------------------------- 000 0F001000 080A0400 00000000 00000000 ................ == DONE ============================================================== WRITE: Write Cache setting is changeable. Sending SCSI command to device. Mode Select 6 Command: 15 11 00 00 10 00 Error sending SCSI command. GLE: 0x0000045d SCSI command sent to device was Mode Select 6 Command: 15 11 00 00 10 00 WRITE: Error returned while issuing MODE SELECT command. The write cache is not changable. The test is skipped. End: Pass, ASSERTION: WRITE (10) FUA Test, TUID=, Repro=scsicompliance.exe /device \\.\PhysicalDrive1 /verbosity 4 /scenario common /operation test Move status to Verified according to comment #35 Vadim, can I review the code? The test probably would pass even with the old driver (it was a problem in the implementation of FUA, not the parsing). The code at git://git.engineering.redhat.com/users/vrozenfe/internal-kvm-guest-drivers-windows (as of commit 1b2a069) looks good. Thanks! (In reply to Paolo Bonzini from comment #38) > The code at > git://git.engineering.redhat.com/users/vrozenfe/internal-kvm-guest-drivers- > windows (as of commit 1b2a069) looks good. Thanks! Thank you for reviewing. Best regards, Vadim. This this issue has been fixed in build 82 while we are shipping build79 for virtio-block ,so no need for rhel7.0.z This request was resolved in Red Hat Enterprise Linux 7.0. Contact your manager or support representative in case you have further questions about the request. |
Description of problem: viostor processes flushes by first completing the SRB, and then sending a VIRTIO_BLK_T_FLUSH request. This is wrong because and could cause data loss, for example: write 1 -> VIRTIO_BLK_T_OUT <- processed CompleteSRB synchronize_cache CompleteSRB -> VIRTIO_BLK_T_FLUSH write 2 -> VIRTIO_BLK_T_OUT <- processed CompleteSRB <<<<power loss or QEMU crash>>>> Now it could happen that write 2 will reach the disk but write 1 didn't, contrarily to the expectations of the guest OS. This could be more visible with qcow2, because flushes also write the qcow2 metadata. It can happen for example whenever write 2 doesn't need a metadata write but write 1 needed one. Related to this, the viostor driver reports MODE_DSP_FUA_SUPPORTED but it doesn't look at the DPO or FUA bits (respectively in reads and writes). The bit should not be included in the MODE SENSE output, since there is no equivalent in the virtio-blk protocol.