Re: [PATCH V6 2/2] virtio-blk: Add REQ_FLUSH and REQ_FUA support to bio path
From: Christoph Hellwig <hch@lst.de>
Date: 2012-08-07 09:15:13
Also in:
lkml, virtualization
At least after review is done I really think this patch sopuld be folded into the previous one. Some more comments below:
quoted hunk ↗ jump to hunk
@@ -58,6 +58,12 @@ struct virtblk_req struct bio *bio; struct virtio_blk_outhdr out_hdr; struct virtio_scsi_inhdr in_hdr; + struct work_struct work; + struct virtio_blk *vblk;
I think using bio->bi_private for the virtio_blk pointer would be cleaner.
quoted hunk ↗ jump to hunk
+ bool is_flush; + bool req_flush; + bool req_data; + bool req_fua;
This could be a bitmap, or better even a single state field.
+static int virtblk_bio_send_flush(struct virtio_blk *vblk, + struct virtblk_req *vbr)
+static int virtblk_bio_send_data(struct virtio_blk *vblk, + struct virtblk_req *vbr)
These should only get the struct virtblk_req * argument as the virtio_blk structure is easily derivable from it.
quoted hunk ↗ jump to hunk
+static inline void virtblk_bio_done_flush(struct virtio_blk *vblk, + struct virtblk_req *vbr) { + if (vbr->req_data) { + /* Send out the actual write data */ + struct virtblk_req *_vbr; + _vbr = virtblk_alloc_req(vblk, GFP_NOIO); + if (!_vbr) { + bio_endio(vbr->bio, -ENOMEM); + goto out; + } + _vbr->req_fua = vbr->req_fua; + _vbr->bio = vbr->bio; + _vbr->vblk = vblk; + INIT_WORK(&_vbr->work, virtblk_bio_send_data_work); + queue_work(virtblk_wq, &_vbr->work);
The _vbr naming isn't too nice. Also can you explain why the original request can't be reused in a comment here? Also if using a state variable I think the whole code would be a bit cleaner if the bio_done helpers are combined.
quoted hunk ↗ jump to hunk
- if (writeback && !use_bio) + if (writeback) blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
Shouldn't this be REQ_FLUSH | REQ_FUA for the bio case? Btw, did you verify that flushes really work correctly for all cases using tracing in qemu?