virtio_blk: add cache flush command

RFC PATCH In review Christoph Hellwig 5 participants 21 messages May 11, 2009 – May 18, 2009

Subsystems (5)

Documentation KVM Block layer virtio core +1 more subsystems

lore

Share & tools

lore

Christoph Hellwig

author

2009-05-11 08:39 UTC #0 lore raw

RFC PATCH 1/1 virtio_blk: add cache flush command

Currently virtio-blk does support barriers for ordering requests which is enough to guarantee filesystem metadata integrity with write back caches, but it does not support any way to flush that writeback cache, to guarantee that data is stable on disk on a fsync.

This patch implements a new VIRTIO_BLK_T_FLUSH command to flush the cache and exposes the functionality to the block layer by implementing a prepare_flush method.

Do we need a new feature flag for this command or can we expect that all previous barrier support was buggy enough anyway?

Signed-off-by Christoph Hellwig <[email protected]>

drivers/block/virtio_blk.c +39 -8

Index: xfs/drivers/block/virtio_blk.c
===================================================================
@@ -65,13 +65,25 @@ static void blk_done(struct virtqueue *v
 			break;
 		}
 
-		if (blk_pc_request(vbr->req)) {
+		switch (vbr->req->cmd_type) {
+		case REQ_TYPE_FS:
+			nr_bytes = blk_rq_bytes(vbr->req);
+			break;
+		case REQ_TYPE_BLOCK_PC:
 			vbr->req->data_len = vbr->in_hdr.residual;
 			nr_bytes = vbr->in_hdr.data_len;
 			vbr->req->sense_len = vbr->in_hdr.sense_len;
 			vbr->req->errors = vbr->in_hdr.errors;
-		} else
-			nr_bytes = blk_rq_bytes(vbr->req);
+			break;
+		case REQ_TYPE_LINUX_BLOCK:
+			if (vbr->req->cmd[0] == REQ_LB_OP_FLUSH) {
+				nr_bytes = blk_rq_bytes(vbr->req);
+				break;
+			}
+			/*FALLTHRU*/
+		default:
+			BUG();
+		}
 
 		__blk_end_request(vbr->req, error, nr_bytes);
 		list_del(&vbr->list);

@@ -82,7 +94,7 @@ static void blk_done(struct virtqueue *v
 	spin_unlock_irqrestore(&vblk->lock, flags);
 }
 
-static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
+static noinline bool do_req(struct request_queue *q, struct virtio_blk *vblk,
 		   struct request *req)
 {
 	unsigned long num, out = 0, in = 0;

@@ -94,15 +106,27 @@ static bool do_req(struct request_queue 
 		return false;
 
 	vbr->req = req;
-	if (blk_fs_request(vbr->req)) {
+
+	switch (req->cmd_type) {
+	case REQ_TYPE_FS:
 		vbr->out_hdr.type = 0;
 		vbr->out_hdr.sector = vbr->req->sector;
 		vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
-	} else if (blk_pc_request(vbr->req)) {
+		break;
+	case REQ_TYPE_BLOCK_PC:
 		vbr->out_hdr.type = VIRTIO_BLK_T_SCSI_CMD;
 		vbr->out_hdr.sector = 0;
 		vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
-	} else {
+		break;
+	case REQ_TYPE_LINUX_BLOCK:
+		if (req->cmd[0] == REQ_LB_OP_FLUSH) {
+			vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
+			vbr->out_hdr.sector = 0;
+			vbr->out_hdr.ioprio = req_get_ioprio(vbr->req);
+			break;
+		}
+		/*FALLTHRU*/
+	default:
 		/* We don't put anything else in the queue. */
 		BUG();
 	}

@@ -174,6 +198,12 @@ static void do_virtblk_request(struct re
 		vblk->vq->vq_ops->kick(vblk->vq);
 }
 
+static void virtblk_prepare_flush(struct request_queue *q, struct request *req)
+{
+	req->cmd_type = REQ_TYPE_LINUX_BLOCK;
+	req->cmd[0] = REQ_LB_OP_FLUSH;
+}
+
 static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
 			 unsigned cmd, unsigned long data)
 {

@@ -310,7 +340,8 @@ static int virtblk_probe(struct virtio_d
 
 	/* If barriers are supported, tell block layer that queue is ordered */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_BARRIER))
-		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
+		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG_FLUSH,
+				  virtblk_prepare_flush);
 
 	/* If disk is read-only in the host, the guest should obey */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))

include/linux/virtio_blk.h +14 -0

Index: xfs/include/linux/virtio_blk.h
===================================================================
@@ -35,6 +35,17 @@ struct virtio_blk_config
 	__u32 blk_size;
 } __attribute__((packed));
 
+/*
+ * Command types
+ *
+ * Usage is a bit tricky as some bits are used as flags and some not.
+ *
+ * Rules:
+ *   VIRTIO_BLK_T_OUT may be combinaed with VIRTIO_BLK_T_SCSI_CMD or
+ *   VIRTIO_BLK_T_BARRIER.  VIRTIO_BLK_T_FLUSH is a command of it's own
+ *   and may no be comined with any of the other flags.
+ */
+
 /* These two define direction. */
 #define VIRTIO_BLK_T_IN		0
 #define VIRTIO_BLK_T_OUT	1

@@ -42,6 +53,9 @@ struct virtio_blk_config
 /* This bit says it's a scsi command, not an actual read or write. */
 #define VIRTIO_BLK_T_SCSI_CMD	2
 
+/* Cache flush command */
+#define VIRTIO_BLK_T_FLUSH	4
+
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER	0x80000000

Anthony Liguori

2009-05-11 14:51 UTC #1 lore raw

Quoted 9 lines · Christoph Hellwig

Currently virtio-blk does support barriers for ordering requests which is enough to guarantee filesystem metadata integrity with write back caches, but it does not support any way to flush that writeback cache, to guarantee that data is stable on disk on a fsync.
This patch implements a new VIRTIO_BLK_T_FLUSH command to flush the cache and exposes the functionality to the block layer by implementing a prepare_flush method.

What typically triggers a flush operation?

I would assume an fsync would, but would a flush happen after every O_DIRECT write?

If the backend implementation of T_FLUSH is fsync, I would think that this would result in rather poor performance for O_DIRECT operations in the guest.

Regards,

Anthony Liguori

Christoph Hellwig

author

2009-05-11 15:40 UTC #2 lore raw

Quoted 1 line · Anthony Liguori

What typically triggers a flush operation?

fsync.

Quoted 2 lines

I would assume an fsync would, but would a flush happen after every O_DIRECT write?

Right now it doesn't, but it probably should.

Quoted 3 lines

If the backend implementation of T_FLUSH is fsync, I would think that this would result in rather poor performance for O_DIRECT operations in the guest.

Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.

Avi Kivity

2009-05-11 15:45 UTC #3 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines
If the backend implementation of T_FLUSH is fsync, I would think that this would result in rather poor performance for O_DIRECT operations in the guest.
Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.

I think if we have cache=writeback we should ignore this. The user is saying, I don't care about data loss, make the thing go fast.

For cache=none and cache=writethrough we don't really need fsync, but we do need to flush the inflight commands.

Signature · 1 line

error compiling committee.c: too many arguments to function

Christoph Hellwig

author

2009-05-11 16:28 UTC #4 lore raw

Quoted 6 lines · Avi Kivity

Quoted 4 lines
Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.
I think if we have cache=writeback we should ignore this.

It's only needed for cache=writeback, because without that there is no reason to flush a write cache.

Quoted 2 lines

For cache=none and cache=writethrough we don't really need fsync, but we do need to flush the inflight commands.

What we do need for those modes is the basic barrier support because we can currently re-order requests. The next version of my patch will implement a barriers without cache flush mode, although I don't think a fdatasync without any outstanding dirty data should cause problems. (Or maybe ext3 actually is stupid enough to flush the whole fs even for that case)

Avi Kivity

2009-05-11 16:50 UTC #5 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Avi Kivity
Quoted 5 lines
Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.

Quoted 6 lines

Quoted 2 lines
I think if we have cache=writeback we should ignore this.
It's only needed for cache=writeback, because without that there is no reason to flush a write cache.

Maybe we should add a fourth cache= mode then. But cache=writeback+fsync doesn't correspond to any real world drive; in the real world you're limited to power failures and a few megabytes of cache (typically less), cache=writeback+fsync can lose hundreds of megabytes due to power loss or software failure.

Oh, and cache=writeback+fsync doesn't work on qcow2, unless we add fsync after metadata updates.

Quoted 8 lines

Quoted 2 lines
For cache=none and cache=writethrough we don't really need fsync, but we do need to flush the inflight commands.
What we do need for those modes is the basic barrier support because we can currently re-order requests. The next version of my patch will implement a barriers without cache flush mode, although I don't think a fdatasync without any outstanding dirty data should cause problems.

Yeah. And maybe one day push the barrier into the kernel.

Quoted 2 lines

(Or maybe ext3 actually is stupid enough to flush the whole fs even for that case

Sigh.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Anthony Liguori

2009-05-11 17:48 UTC #6 lore raw

Quoted 11 lines · Avi Kivity

Quoted 10 lines · Christoph Hellwig
Quoted 7 lines · Avi Kivity
Quoted 5 lines
Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.
I think if we have cache=writeback we should ignore this.

Quoted 7 lines

Quoted 3 lines
It's only needed for cache=writeback, because without that there is no reason to flush a write cache.
Maybe we should add a fourth cache= mode then. But cache=writeback+fsync doesn't correspond to any real world drive; in the real world you're limited to power failures and a few megabytes of cache (typically less), cache=writeback+fsync can lose hundreds of megabytes due to power loss or software failure.
Oh, and cache=writeback+fsync doesn't work on qcow2, unless we add fsync after metadata updates.

But how do we define the data integrity guarantees to the user of cache=writeback+fsync? It seems to require a rather detailed knowledge of Linux's use of T_FLUSH operations.

Right now, it's fairly easy to understand. cache=none and cache=writethrough guarantee that all write operations that the guest thinks have completed are completed. cache=writeback provides no such guarantee.

cache=writeback+fsync would guarantee that only operations that include a T_FLUSH are present on disk which currently includes fsyncs but does not include O_DIRECT writes. I guess whether O_SYNC does a T_FLUSH also has to be determined.

It seems too complicated to me. If we could provide a mode where cache=writeback provided as strong a guarantee as cache=writethrough, then that would be quite interesting.

Quoted 4 lines

Quoted 2 lines
(Or maybe ext3 actually is stupid enough to flush the whole fs even for that case
Sigh.

I'm also worried about ext3 here.

Regards,

Anthony Liguori

Avi Kivity

2009-05-11 18:01 UTC #7 lore raw

Quoted 16 lines · Anthony Liguori

Quoted 15 lines · Avi Kivity
Quoted 13 lines · Christoph Hellwig
Quoted 7 lines · Avi Kivity
Quoted 5 lines
Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.
I think if we have cache=writeback we should ignore this.
It's only needed for cache=writeback, because without that there is no reason to flush a write cache.

Quoted 5 lines

Quoted 3 lines
Maybe we should add a fourth cache= mode then. But cache=writeback+fsync doesn't correspond to any real world drive; in the real world you're limited to power failures and a few megabytes of cache (typically less), cache=writeback+fsync can lose hundreds of megabytes due to power loss or software failure.
Oh, and cache=writeback+fsync doesn't work on qcow2, unless we add fsync after metadata updates.
But how do we define the data integrity guarantees to the user of cache=writeback+fsync? It seems to require a rather detailed knowledge of Linux's use of T_FLUSH operations.

True. I don't think cache=writeback+fsync is useful. Like I mentioned, it doesn't act like a real drive, and it doesn't work well with qcow2.

Quoted 2 lines

Right now, it's fairly easy to understand. cache=none and cache=writethrough guarantee that all write operations that the guest thinks have completed are completed. cache=writeback provides no such guarantee.

cache=none is partially broken as well, since O_DIRECT writes might hit an un-battery-packed write cache. I think cache=writeback will send the necessary flushes, if the disk and the underlying filesystem support them.

Quoted 3 lines

cache=writeback+fsync would guarantee that only operations that include a T_FLUSH are present on disk which currently includes fsyncs but does not include O_DIRECT writes. I guess whether O_SYNC does a T_FLUSH also has to be determined.
It seems too complicated to me. If we could provide a mode where cache=writeback provided as strong a guarantee as cache=writethrough, then that would be quite interesting.

It don't think we realistically can.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
(Or maybe ext3 actually is stupid enough to flush the whole fs even for that case
Sigh.
I'm also worried about ext3 here.

I'm just waiting for btrfs.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Anthony Liguori

2009-05-11 18:29 UTC #8 lore raw

Quoted 6 lines · Avi Kivity

Quoted 2 lines · Anthony Liguori
Right now, it's fairly easy to understand. cache=none and cache=writethrough guarantee that all write operations that the guest thinks have completed are completed. cache=writeback provides no such guarantee.
cache=none is partially broken as well, since O_DIRECT writes might hit an un-battery-packed write cache. I think cache=writeback will send the necessary flushes, if the disk and the underlying filesystem support them.

Sure, but this likely doesn't upset people that much since O_DIRECT has always had this behavior. Using non-battery backed disks with writeback enabled introduces a larger set of possible data integrity issues. I think this case is acceptable to ignore because it's a straight forward policy.

Quoted 5 lines

Quoted 3 lines
cache=writeback+fsync would guarantee that only operations that include a T_FLUSH are present on disk which currently includes fsyncs but does not include O_DIRECT writes. I guess whether O_SYNC does a T_FLUSH also has to be determined.
It seems too complicated to me. If we could provide a mode where cache=writeback provided as strong a guarantee as cache=writethrough, then that would be quite interesting.
It don't think we realistically can.

Maybe two fds? One open in O_SYNC and one not. Is such a thing sane?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
(Or maybe ext3 actually is stupid enough to flush the whole fs even for that case
Sigh.
I'm also worried about ext3 here.
I'm just waiting for btrfs.

Even ext4 is saner but we'll get lots of bug reports while ext3 remains common.

Regards,

Anthony Liguori

Avi Kivity

2009-05-11 18:41 UTC #9 lore raw

Quoted 9 lines · Anthony Liguori

Quoted 6 lines · Avi Kivity
Quoted 2 lines · Anthony Liguori
Right now, it's fairly easy to understand. cache=none and cache=writethrough guarantee that all write operations that the guest thinks have completed are completed. cache=writeback provides no such guarantee.
cache=none is partially broken as well, since O_DIRECT writes might hit an un-battery-packed write cache. I think cache=writeback will send the necessary flushes, if the disk and the underlying filesystem support them.
Sure, but this likely doesn't upset people that much since O_DIRECT has always had this behavior.

But people are not using O_DIRECT. They're using their guests, which may or may not issue the appropriate barriers. They don't know that we're using O_DIRECT underneath with different guarantees.

Quoted 1 line

Using non-battery backed disks with writeback enabled introduces a larger set of possible data integrity issues. I think this case is acceptable to ignore because it's a straight forward policy.

It isn't straightforward to me. A guest should be able to get the same guarantees running on a hypervisor backed by such a disk as it would get if it was running on bare metal with the same disk. Right now, that's not the case, we're reducing the guarantees the guest gets.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
cache=writeback+fsync would guarantee that only operations that include a T_FLUSH are present on disk which currently includes fsyncs but does not include O_DIRECT writes. I guess whether O_SYNC does a T_FLUSH also has to be determined.
It seems too complicated to me. If we could provide a mode where cache=writeback provided as strong a guarantee as cache=writethrough, then that would be quite interesting.
It don't think we realistically can.
Maybe two fds? One open in O_SYNC and one not. Is such a thing sane?

For all I care, yes. Filesystem developers would probably have you locked up.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Christoph Hellwig

author

2009-05-18 12:04 UTC #10 lore raw

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

Quoted 1 line
It don't think we realistically can.
Maybe two fds? One open in O_SYNC and one not. Is such a thing sane?

O_SYNC and O_DIRECT currently behave exactly in the same way. In none of the filesystem I've looked at there is an explicit cache flush, but all send down a barrier which gives you an implicit cache flush with the way barriers are implemented.

Christoph Hellwig

author

2009-05-12 07:24 UTC #11 lore raw

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

But how do we define the data integrity guarantees to the user of cache=writeback+fsync? It seems to require a rather detailed knowledge of Linux's use of T_FLUSH operations.

It does work the same as for disks with writeback caches. If a barrier request completes the filesystem can assume all previous I/O on this disk has finished before the barrier request, and the barrier request has finished after these without any later request being finished before it. If a cache flush is issued the cache as of that point in time is flushed (used for fsync).

Quoted 4 lines

Right now, it's fairly easy to understand. cache=none and cache=writethrough guarantee that all write operations that the guest thinks have completed are completed. cache=writeback provides no such guarantee.

As said above with barriers it indeed does.

Quoted 4 lines

cache=writeback+fsync would guarantee that only operations that include a T_FLUSH are present on disk which currently includes fsyncs but does not include O_DIRECT writes. I guess whether O_SYNC does a T_FLUSH also has to be determined.

O_SYNC and O_DIRECT do it at least on XFS due to updating metadata after the write. I can't vouch for implementation details on other filesystems.

Christoph Hellwig

author

2009-05-12 07:20 UTC #12 lore raw

In reply to Avi Kivity

Quoted 5 lines · Avi Kivity

Maybe we should add a fourth cache= mode then. But cache=writeback+fsync doesn't correspond to any real world drive; in the real world you're limited to power failures and a few megabytes of cache (typically less), cache=writeback+fsync can lose hundreds of megabytes due to power loss or software failure.

cache=writeback+fsync is exactly the same model as a normal writeback cache disk drive. (Well, almost as we currently don't use tag ordering but drain flushes as a Linux implementation detail, but the disks also support TCQ-based ordering).

The cache size on disks is constantly growing, and if you lose cache it doesn't really matter how much you lose but what you lose.

Quoted 2 lines

Oh, and cache=writeback+fsync doesn't work on qcow2, unless we add fsync after metadata updates.

If you care about data integrity in case of crashes qcow2 doesn't work at all.

Avi Kivity

2009-05-12 08:35 UTC #13 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 2 lines · Avi Kivity
Maybe we should add a fourth cache= mode then. But cache=writeback+fsync doesn't correspond to any real world drive; in the real world you're limited to power failures and a few megabytes of cache (typically less), cache=writeback+fsync can lose hundreds of megabytes due to power loss or software failure.

Quoted 8 lines

cache=writeback+fsync is exactly the same model as a normal writeback cache disk drive. (Well, almost as we currently don't use tag ordering but drain flushes as a Linux implementation detail, but the disks also support TCQ-based ordering).
The cache size on disks is constantly growing, and if you lose cache it doesn't really matter how much you lose but what you lose.

Software errors won't cause data loss on a real disk (firmware bugs will, but the firmware is less likely to crash than the host OS).

Quoted 6 lines

Quoted 2 lines
Oh, and cache=writeback+fsync doesn't work on qcow2, unless we add fsync after metadata updates.
If you care about data integrity in case of crashes qcow2 doesn't work at all.

Do you known of any known corruptors in qcow2 with cache=writethrough?

Signature · 1 line

error compiling committee.c: too many arguments to function

Christoph Hellwig

author

2009-05-18 12:06 UTC #14 lore raw

Quoted 6 lines · Avi Kivity

Quoted 3 lines
The cache size on disks is constantly growing, and if you lose cache it doesn't really matter how much you lose but what you lose.
Software errors won't cause data loss on a real disk (firmware bugs will, but the firmware is less likely to crash than the host OS).

OS crash or hardware crash really makes no difference for writeback
caches. The case we are trying to protect against here is any crash,
and a hardware induced one or power failure is probably more likely in either case than a software failure in either the OS or firmware.

Quoted 5 lines

Quoted 3 lines
If you care about data integrity in case of crashes qcow2 doesn't work at all.
Do you known of any known corruptors in qcow2 with cache=writethrough?

It's not related to cache=writethrough. The issue is that there are no transactional guarantees in qcow2.

Anthony Liguori

2009-05-11 16:38 UTC #15 lore raw

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

Quoted 2 lines · Anthony Liguori
What typically triggers a flush operation?

Quoted 8 lines

fsync.
Quoted 2 lines
I would assume an fsync would, but would a flush happen after every O_DIRECT write?
Right now it doesn't, but it probably should.

So then with cache=writeback, fsync behaves itself but O_DIRECT writes do not.

This seems like a really undesirable combination of behavior from a guest integrity point of view. It makes me wonder if it's really useful. I think that any serious user would have to continue using cache=writethrough. Is there a path that would ever allow someone who cares about their data to use cache=writeback instead of cache=writethrough?

Quoted 7 lines

Quoted 2 lines
If the backend implementation of T_FLUSH is fsync, I would think that this would result in rather poor performance for O_DIRECT operations in the guest.
Right now it's fsync. By the time I'll submit the backend change it will still be fsync, but at least called from the posix-aio-compat thread pool.

fsync is pretty crappy on ext3 default configs. I'm concerned that this could be considered a DoS by a malicious guest. If it sat in a T_FLUSH loop, it would potentially bring your system to a crawl, no?

Regards,

Anthony Liguori

Christoph Hellwig

author

2009-05-12 07:26 UTC #16 lore raw

Quoted 5 lines · Anthony Liguori

Quoted 2 lines
Right now it doesn't, but it probably should.
So then with cache=writeback, fsync behaves itself but O_DIRECT writes do not.

Right now O_DIRECT does not do an explicit cache flush, but due to the way barriers are implemented in Linux we do get the cache flush as part of the metadata updates after I/O completion.

Quoted 3 lines

fsync is pretty crappy on ext3 default configs. I'm concerned that this could be considered a DoS by a malicious guest. If it sat in a T_FLUSH loop, it would potentially bring your system to a crawl, no?

It's exactly the same effect as a regular user doing fsync in a loop, whatever that causes on ext3.

Rusty Russell

2009-05-12 13:54 UTC #17 lore raw

In reply to Christoph Hellwig

Quoted 2 lines · Christoph Hellwig

Do we need a new feature flag for this command or can we expect that all previous barrier support was buggy enough anyway?

You mean reuse the VIRTIO_BLK_F_BARRIER for this as well? Seems fine.

AFAIK only lguest offered that, and lguest has no ABI. Best would be to implement VIRTIO_BLK_T_FLUSH as well; it's supposed to be demo code, and it should be easy).

Thanks,
Rusty.

Christian Borntraeger

2009-05-12 14:18 UTC #18 lore raw

Am Tuesday 12 May 2009 15:54:14 schrieb Rusty Russell:

Quoted 9 lines · Christoph Hellwig

Quoted 2 lines
Do we need a new feature flag for this command or can we expect that all previous barrier support was buggy enough anyway?
You mean reuse the VIRTIO_BLK_F_BARRIER for this as well? Seems fine.
AFAIK only lguest offered that, and lguest has no ABI. Best would be to implement VIRTIO_BLK_T_FLUSH as well; it's supposed to be demo code, and it should be easy).

It is also used by kuli (http://www.ibm.com/developerworks/linux/linux390/kuli.html)
and kuli used fdatasync. Since kuli is on offical webpages it takes a while to update that code. When you reuse the VIRTIO_BLK_F_BARRIER flag, that would trigger some VIRTIO_BLK_T_FLUSH commands sent to the kuli host, right? I think the if/else logic of kuli would interpret that as a read request....I am voting for a new feature flag :-)

Christian

Rusty Russell

2009-05-13 01:52 UTC #19 lore raw

Quoted 10 lines · Christian Borntraeger

Am Tuesday 12 May 2009 15:54:14 schrieb Rusty Russell:
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines
Do we need a new feature flag for this command or can we expect that all previous barrier support was buggy enough anyway?
You mean reuse the VIRTIO_BLK_F_BARRIER for this as well? Seems fine.
It is also used by kuli
(http://www.ibm.com/developerworks/linux/linux390/kuli.html) and kuli used fdatasync.

OK, that's sufficient for me. It's not like block is going to catch up with net for feature flags any time soon.

Christoph, please make a new feature flag.

Oh and FYI Christian, you might not have seen this patch:

lguest: barrier me harder

Impact: barrier correctness in example launcher

I doubt either lguest user will complain about performance.

Reported-by Christoph Hellwig <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Rusty Russell <[email protected]>

---
 Documentation/lguest/lguest.c |    7 +++++++
 1 file changed, 7 insertions(+)

Documentation/lguest/lguest.c +7 -0

@@ -1630,6 +1630,13 @@ static bool service_io(struct device *de
 		}
 	}
 
+	/* OK, so we noted that it was pretty poor to use an fdatasync as a
+	 * barrier.  But Christoph Hellwig points out that we need a sync
+	 * *afterwards* as well: "Barriers specify no reordering to the front
+	 * or the back."  And Jens Axboe confirmed it, so here we are: */
+	if (out->type & VIRTIO_BLK_T_BARRIER)
+		fdatasync(vblk->fd);
+
 	/* We can't trigger an IRQ, because we're not the Launcher.  It does
 	 * that when we tell it we're done. */
 	add_used(dev->vq, head, wlen);

Christoph Hellwig

author

2009-05-18 12:07 UTC #20 lore raw

In reply to Christian Borntraeger

Quoted 11 lines · Christian Borntraeger

Am Tuesday 12 May 2009 15:54:14 schrieb Rusty Russell:
Quoted 9 lines · Christoph Hellwig
Quoted 2 lines
Do we need a new feature flag for this command or can we expect that all previous barrier support was buggy enough anyway?
You mean reuse the VIRTIO_BLK_F_BARRIER for this as well? Seems fine.
AFAIK only lguest offered that, and lguest has no ABI. Best would be to implement VIRTIO_BLK_T_FLUSH as well; it's supposed to be demo code, and it should be easy).

Quoted 6 lines

It is also used by kuli (http://www.ibm.com/developerworks/linux/linux390/kuli.html)
and kuli used fdatasync. Since kuli is on offical webpages it takes a while to update that code. When you reuse the VIRTIO_BLK_F_BARRIER flag, that would trigger some VIRTIO_BLK_T_FLUSH commands sent to the kuli host, right? I think the if/else logic of kuli would interpret that as a read request....I am voting for a new feature flag :-)

Ok, next version will have a new feature flag. I actually have some other bits I want to redesign so it might take a bit longer to get it out.