| Message ID | 1401799978-9558-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
I guess this is more of an RFC, but still a useful starting point for discussion. Il 03/06/2014 14:52, Fam Zheng ha scritto: > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index e406efa..74f0f32 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq { > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > struct VirtIOBlockDataPlane *s; This is just dev->dataplane, so it's trivial to remove. > QEMUIOVector *inhdr; /* iovecs for virtio_blk_inhdr */ This can be unified with the "in" field; the status is only one byte, so using a full-blown QEMUIOVector is overkill. Stefan, what do you think? For the sake of restarting requests, we also need dataplane to populate the "out" field for dataplane. We can also take the occasion to change it from "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane and use iov_discard_front on the elem (see dataplane's process_request function). Can you do it in v2 of this patch series? With this in place we can look at the missing pieces: - rerror/werror - accounting (trivial) - multiwrite (if desired). Thanks, Paolo > - bool read; /* read or write? */ > #endif > } VirtIOBlockReq; > >
On Tue, 06/03 15:37, Paolo Bonzini wrote: > I guess this is more of an RFC, but still a useful starting point for > discussion. > > Il 03/06/2014 14:52, Fam Zheng ha scritto: > >diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > >index e406efa..74f0f32 100644 > >--- a/include/hw/virtio/virtio-blk.h > >+++ b/include/hw/virtio/virtio-blk.h > >@@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq { > > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > > struct VirtIOBlockDataPlane *s; > > This is just dev->dataplane, so it's trivial to remove. > > > QEMUIOVector *inhdr; /* iovecs for virtio_blk_inhdr */ > > This can be unified with the "in" field; the status is only one byte, so > using a full-blown QEMUIOVector is overkill. Stefan, what do you think? > > For the sake of restarting requests, we also need dataplane to populate the > "out" field for dataplane. We can also take the occasion to change it from > "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane > and use iov_discard_front on the elem (see dataplane's process_request > function). > > Can you do it in v2 of this patch series? With this in place we can look at > the missing pieces: > > - rerror/werror > > - accounting (trivial) > > - multiwrite (if desired). > Yes, good idea. I will do it! Thanks, Fam
On Tue, 06/03 15:37, Paolo Bonzini wrote: > I guess this is more of an RFC, but still a useful starting point for > discussion. > > Il 03/06/2014 14:52, Fam Zheng ha scritto: > >diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > >index e406efa..74f0f32 100644 > >--- a/include/hw/virtio/virtio-blk.h > >+++ b/include/hw/virtio/virtio-blk.h > >@@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq { > > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > > struct VirtIOBlockDataPlane *s; > > This is just dev->dataplane, so it's trivial to remove. > > > QEMUIOVector *inhdr; /* iovecs for virtio_blk_inhdr */ > > This can be unified with the "in" field; the status is only one byte, so > using a full-blown QEMUIOVector is overkill. Stefan, what do you think? > > For the sake of restarting requests, we also need dataplane to populate the > "out" field for dataplane. We can also take the occasion to change it from > "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane > and use iov_discard_front on the elem (see dataplane's process_request > function). Using pointer avoids copying, what's the advantage of converting to "sturct virtio_blk_outhdr" for non-dataplane code? For thread-safety? Fam > > Can you do it in v2 of this patch series? With this in place we can look at > the missing pieces: > > - rerror/werror > > - accounting (trivial) > > - multiwrite (if desired). > > Thanks, > > Paolo > > >- bool read; /* read or write? */ > > #endif > > } VirtIOBlockReq; > > > > >
Il 04/06/2014 09:53, Fam Zheng ha scritto: >> > For the sake of restarting requests, we also need dataplane to populate the >> > "out" field for dataplane. We can also take the occasion to change it from >> > "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane >> > and use iov_discard_front on the elem (see dataplane's process_request >> > function). > Using pointer avoids copying, what's the advantage of converting to "sturct > virtio_blk_outhdr" for non-dataplane code? For thread-safety? The virtio code currently assumes that the outhdr is in its own iovec. This is not guaranteed by the spec, it's just that the drivers do it because QEMU required it. Paolo
On Tue, Jun 03, 2014 at 03:37:29PM +0200, Paolo Bonzini wrote: > > QEMUIOVector *inhdr; /* iovecs for virtio_blk_inhdr */ > > This can be unified with the "in" field; the status is only one byte, so > using a full-blown QEMUIOVector is overkill. Stefan, what do you think? I thought about this when initially writing the code, but it seemed like a hack to assume virtio_blk_inhdr will always be 1 byte. virtio-blk is unlikely to change much since the focus is on virtio-scsi rather than piling on more virtio-blk feature. I still prefer we treat it like a struct without making size assumptions, but if the code turns out to be nicer then I don't mind. Stefan
On Wed, 06/04 10:42, Paolo Bonzini wrote: > Il 04/06/2014 09:53, Fam Zheng ha scritto: > >>> For the sake of restarting requests, we also need dataplane to populate the > >>> "out" field for dataplane. We can also take the occasion to change it from > >>> "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane > >>> and use iov_discard_front on the elem (see dataplane's process_request > >>> function). > >Using pointer avoids copying, what's the advantage of converting to "sturct > >virtio_blk_outhdr" for non-dataplane code? For thread-safety? > > The virtio code currently assumes that the outhdr is in its own iovec. This > is not guaranteed by the spec, it's just that the drivers do it because QEMU > required it. > I see, thanks. Will convert it. IIUC, this also applies to inhdr? Do we need to fix them as well? Fam
Il 04/06/2014 13:03, Fam Zheng ha scritto: > On Wed, 06/04 10:42, Paolo Bonzini wrote: >> Il 04/06/2014 09:53, Fam Zheng ha scritto: >>>>> For the sake of restarting requests, we also need dataplane to populate the >>>>> "out" field for dataplane. We can also take the occasion to change it from >>>>> "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane >>>>> and use iov_discard_front on the elem (see dataplane's process_request >>>>> function). >>> Using pointer avoids copying, what's the advantage of converting to "sturct >>> virtio_blk_outhdr" for non-dataplane code? For thread-safety? >> >> The virtio code currently assumes that the outhdr is in its own iovec. This >> is not guaranteed by the spec, it's just that the drivers do it because QEMU >> required it. > > I see, thanks. Will convert it. IIUC, this also applies to inhdr? Do we need to > fix them as well? No, but only because inhdr is a single byte. Paolo
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index b88eee9..c75141b 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -129,7 +129,6 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read, req->s = s; req->elem = elem; req->inhdr = inhdr; - req->read = read; qemu_iovec_init_external(&req->qiov, iov, iov_cnt); qiov = &req->qiov; diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index e406efa..74f0f32 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq { #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE struct VirtIOBlockDataPlane *s; QEMUIOVector *inhdr; /* iovecs for virtio_blk_inhdr */ - bool read; /* read or write? */ #endif } VirtIOBlockReq;
Since it's set but not used. Signed-off-by: Fam Zheng <famz@redhat.com> --- hw/block/dataplane/virtio-blk.c | 1 - include/hw/virtio/virtio-blk.h | 1 - 2 files changed, 2 deletions(-)