devices: virtio: block: ignore Discard failures

Our branch of the 3.18 kernel has FALLOC_FL_PUNCH_HOLE disabled for the
ext4 filesystem, which means that systems running that kernel always
take the fallback path of writing buffers full of zeroes.  This is not
necessary for the Discard command, since it is just a hint and is not
required to actually zero the blocks.

Split the WriteZeroes trait up into a new PunchHole trait, which
corresponds to fallocate() with FALLOC_FL_PUNCH_HOLE, and use the new
trait to implement the virtio block Discard command.

BUG=chromium:896314
TEST=`mkfs.btrfs /dev/vdb` and verify the desired fallocate() is used
     and no write() calls are issued when inducing a failure

Change-Id: I67fd9a132758d8d766531ccca8358c7fe67b0460
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1286224
Reviewed-by: Dylan Reid <dgreid@chromium.org>

1 files changed, 22 insertions, 13 deletions

diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs
index a6594ec..51dff51 100644
--- a/devices/src/virtio/block.rs
+++ b/devices/src/virtio/block.rs
@@ -16,8 +16,8 @@ use std::u32;
 use sys_util::Error as SysError;
 use sys_util::Result as SysResult;
 use sys_util::{
-    EventFd, GuestAddress, GuestMemory, GuestMemoryError, PollContext, PollToken, TimerFd,
-    WriteZeroes,
+    EventFd, GuestAddress, GuestMemory, GuestMemoryError, PollContext, PollToken, PunchHole,
+    TimerFd, WriteZeroes,
 };
 
 use data_model::{DataInit, Le16, Le32, Le64};
@@ -108,8 +108,8 @@ const VIRTIO_BLK_DISCARD_WRITE_ZEROES_FLAG_UNMAP: u32 = 1 << 0;
 // Safe because it only has data and has no implicit padding.
 unsafe impl DataInit for virtio_blk_discard_write_zeroes {}
 
-pub trait DiskFile: Read + Seek + Write + WriteZeroes {}
-impl<D: Read + Seek + Write + WriteZeroes> DiskFile for D {}
+pub trait DiskFile: PunchHole + Read + Seek + Write + WriteZeroes {}
+impl<D: PunchHole + Read + Seek + Write + WriteZeroes> DiskFile for D {}
 
 #[derive(Copy, Clone, Debug, PartialEq)]
 enum RequestType {
@@ -431,15 +431,24 @@ impl Request {
                         });
                     }
 
-                    disk.seek(SeekFrom::Start(sector << SECTOR_SHIFT))
-                        .map_err(|e| ExecuteError::Seek { ioerr: e, sector })?;
-                    disk.write_zeroes((num_sectors as usize) << SECTOR_SHIFT)
-                        .map_err(|e| ExecuteError::DiscardWriteZeroes {
-                            ioerr: Some(e),
-                            sector,
-                            num_sectors,
-                            flags,
-                        })?;
+                    if self.request_type == RequestType::Discard {
+                        // Since Discard is just a hint and some filesystems may not implement
+                        // FALLOC_FL_PUNCH_HOLE, ignore punch_hole errors.
+                        let _ = disk.punch_hole(
+                            sector << SECTOR_SHIFT,
+                            (num_sectors as u64) << SECTOR_SHIFT,
+                        );
+                    } else {
+                        disk.seek(SeekFrom::Start(sector << SECTOR_SHIFT))
+                            .map_err(|e| ExecuteError::Seek { ioerr: e, sector })?;
+                        disk.write_zeroes((num_sectors as usize) << SECTOR_SHIFT)
+                            .map_err(|e| ExecuteError::DiscardWriteZeroes {
+                                ioerr: Some(e),
+                                sector,
+                                num_sectors,
+                                flags,
+                            })?;
+                    }
                 }
             }
             RequestType::Flush => {