Re: [PATCH] vhost-blk: Add vhost-blk support v2
On 10/23/2012 08:07 AM, Rusty Russell wrote: > "Michael S. Tsirkin" writes: >> On Thu, Oct 18, 2012 at 02:50:56PM +1030, Rusty Russell wrote: >>> Asias He writes: >> +#define BLK_HDR 0 > > What's this for, exactly? Please add a comment. The block headr is in the first and separate buffer. >>> >>> Please don't assume this! We're trying to fix all the assumptions in >>> qemu at the moment. >>> >>> vhost_net handles this correctly, taking bytes off the descriptor chain >>> as required. >>> >>> Thanks, >>> Rusty. >> >> BTW are we agreed on the spec update that makes cmd 32 bytes? > > vhost-blk doesn't handle scsi requests, does it? No, it doesn't handle scsi requests currently. > > But since we're forced to use a feature bit, we could just put the cmd > size in explicitly. Though Paulo seems convinced that 32 is always > sufficient. Whoever implements it gets to decide... > > Here's my TODO list: > 1) Create qemu helpers to efficiently handle iovecs. > 2) Switch all the qemu devices to use them. > 3) ... except a special hack for virtio-blk in old-layout mode. > 4) Implement pci capability layout RFC for qemu. >- Detect whether guest uses capabilities. >- Device config in new mode is le. >- Add strict checking mode for extra compliance checks? > 5) Add explicit size-based accessors to virtio_config in kernel. > 6) Update pci capability RFC patches for linux to match. >- Use explicit accessors to allow for endian conversion. > 6) Push virtio torture patch to test variable boundaries. > 7) Update spec. > > That should keep me amused for a while... > > Cheers, > Rusty. > -- Asias ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-blk: Add vhost-blk support v2
"Michael S. Tsirkin" writes: > On Thu, Oct 18, 2012 at 02:50:56PM +1030, Rusty Russell wrote: >> Asias He writes: >> >>> +#define BLK_HDR 0 >> >> >> >> What's this for, exactly? Please add a comment. >> > >> > The block headr is in the first and separate buffer. >> >> Please don't assume this! We're trying to fix all the assumptions in >> qemu at the moment. >> >> vhost_net handles this correctly, taking bytes off the descriptor chain >> as required. >> >> Thanks, >> Rusty. > > BTW are we agreed on the spec update that makes cmd 32 bytes? vhost-blk doesn't handle scsi requests, does it? But since we're forced to use a feature bit, we could just put the cmd size in explicitly. Though Paulo seems convinced that 32 is always sufficient. Whoever implements it gets to decide... Here's my TODO list: 1) Create qemu helpers to efficiently handle iovecs. 2) Switch all the qemu devices to use them. 3) ... except a special hack for virtio-blk in old-layout mode. 4) Implement pci capability layout RFC for qemu. - Detect whether guest uses capabilities. - Device config in new mode is le. - Add strict checking mode for extra compliance checks? 5) Add explicit size-based accessors to virtio_config in kernel. 6) Update pci capability RFC patches for linux to match. - Use explicit accessors to allow for endian conversion. 6) Push virtio torture patch to test variable boundaries. 7) Update spec. That should keep me amused for a while... Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-blk: Add vhost-blk support v2
On 10/18/2012 12:20 PM, Rusty Russell wrote: > Asias He writes: +#define BLK_HDR 0 >>> >>> What's this for, exactly? Please add a comment. >> >> The block headr is in the first and separate buffer. > > Please don't assume this! We're trying to fix all the assumptions in > qemu at the moment. > > vhost_net handles this correctly, taking bytes off the descriptor chain > as required. Well, I will switch to "no assumption about the buffer layout" in next version. -- Asias ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-blk: Add vhost-blk support v2
On Thu, Oct 18, 2012 at 02:50:56PM +1030, Rusty Russell wrote: > Asias He writes: > >>> +#define BLK_HDR 0 > >> > >> What's this for, exactly? Please add a comment. > > > > The block headr is in the first and separate buffer. > > Please don't assume this! We're trying to fix all the assumptions in > qemu at the moment. > > vhost_net handles this correctly, taking bytes off the descriptor chain > as required. > > Thanks, > Rusty. BTW are we agreed on the spec update that makes cmd 32 bytes? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-blk: Add vhost-blk support v2
Asias He writes: >>> +#define BLK_HDR0 >> >> What's this for, exactly? Please add a comment. > > The block headr is in the first and separate buffer. Please don't assume this! We're trying to fix all the assumptions in qemu at the moment. vhost_net handles this correctly, taking bytes off the descriptor chain as required. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-blk: Add vhost-blk support v2
On Fri, Oct 12, 2012 at 09:18:54PM +0800, Asias He wrote:
> Hello Michael,
>
> Thanks for the review!
>
> On 10/11/2012 08:41 PM, Michael S. Tsirkin wrote:
> > On Tue, Oct 09, 2012 at 04:05:18PM +0800, Asias He wrote:
> >> vhost-blk is an in-kernel virito-blk device accelerator.
> >>
> >> Due to lack of proper in-kernel AIO interface, this version converts
> >> guest's I/O request to bio and use submit_bio() to submit I/O directly.
> >> So this version any supports raw block device as guest's disk image,
> >> e.g. /dev/sda, /dev/ram0. We can add file based image support to
> >> vhost-blk once we have in-kernel AIO interface. There are some work in
> >> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
> >>
> >>http://marc.info/?l=linux-fsdevel&m=133312234313122
> >>
> >> Performance evaluation:
> >> -
> >> 1) LKVM
> >> Fio with libaio ioengine on Fusion IO device using kvm tool
> >> IOPS Before After Improvement
> >> seq-read 107 121 +13.0%
> >> seq-write 130 179 +37.6%
> >> rnd-read 102 122 +19.6%
> >> rnd-write 125 159 +27.0%
> >>
> >> 2) QEMU
> >> Fio with libaio ioengine on Fusion IO device using QEMU
> >> IOPS Before After Improvement
> >> seq-read 76 123 +61.8%
> >> seq-write 139 173 +24.4%
> >> rnd-read 73 120 +64.3%
> >> rnd-write 75 156 +108.0%
> >>
> >> Userspace bits:
> >> -
> >> 1) LKVM
> >> The latest vhost-blk userspace bits for kvm tool can be found here:
> >> g...@github.com:asias/linux-kvm.git blk.vhost-blk
> >>
> >> 2) QEMU
> >> The latest vhost-blk userspace prototype for QEMU can be found here:
> >> g...@github.com:asias/qemu.git blk.vhost-blk
> >>
> >> Signed-off-by: Asias He
> >> ---
> >> drivers/vhost/Kconfig | 1 +
> >> drivers/vhost/Kconfig.blk | 10 +
> >> drivers/vhost/Makefile| 2 +
> >> drivers/vhost/blk.c | 641
> >> ++
> >> drivers/vhost/blk.h | 8 +
> >> 5 files changed, 662 insertions(+)
> >> create mode 100644 drivers/vhost/Kconfig.blk
> >> create mode 100644 drivers/vhost/blk.c
> >> create mode 100644 drivers/vhost/blk.h
> >>
> >> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> >> index 202bba6..acd8038 100644
> >> --- a/drivers/vhost/Kconfig
> >> +++ b/drivers/vhost/Kconfig
> >> @@ -11,4 +11,5 @@ config VHOST_NET
> >>
> >> if STAGING
> >> source "drivers/vhost/Kconfig.tcm"
> >> +source "drivers/vhost/Kconfig.blk"
> >> endif
> >> diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
> >> new file mode 100644
> >> index 000..ff8ab76
> >> --- /dev/null
> >> +++ b/drivers/vhost/Kconfig.blk
> >> @@ -0,0 +1,10 @@
> >> +config VHOST_BLK
> >> + tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
> >> + depends on BLOCK && EXPERIMENTAL && m
> >> + ---help---
> >> +This kernel module can be loaded in host kernel to accelerate
> >> +guest block with virtio_blk. Not to be confused with virtio_blk
> >> +module itself which needs to be loaded in guest kernel.
> >> +
> >> +To compile this driver as a module, choose M here: the module will
> >> +be called vhost_blk.
> >> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> >> index a27b053..1a8a4a5 100644
> >> --- a/drivers/vhost/Makefile
> >> +++ b/drivers/vhost/Makefile
> >> @@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
> >> vhost_net-y := vhost.o net.o
> >>
> >> obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
> >> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
> >> +vhost_blk-y := blk.o
> >> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> >> new file mode 100644
> >> index 000..6b2445a
> >> --- /dev/null
> >> +++ b/drivers/vhost/blk.c
> >> @@ -0,0 +1,641 @@
> >> +/*
> >> + * Copyright (C) 2011 Taobao, Inc.
> >> + * Author: Liu Yuan
> >> + *
> >> + * Copyright (C) 2012 Red Hat, Inc.
> >> + * Author: Asias He
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2.
> >> + *
> >> + * virtio-blk server in host kernel.
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "vhost.c"
> >> +#include "vhost.h"
> >> +#include "blk.h"
> >> +
> >> +#define BLK_HDR 0
> >
> > What's this for, exactly? Please add a comment.
>
>
> The block headr is in the first and separate buffer.
>
> >> +
> >> +static DEFINE_IDA(vhost_blk_index_ida);
> >> +
> >> +enum {
> >> + VHOST_BLK_VQ_REQ = 0,
> >> + VHOST_BLK_VQ_MAX = 1,
> >> +};
> >> +
> >> +struct req_page_list {
> >> + struct page **pages;
> >> + int pages_nr;
> >> +};
> >> +
> >> +struct vhost_blk_req {
> >> + struct llist_node llnode;
> >> + struct req_page_list *pl;
> >> + struct vhost_blk *blk;
> >> +
> >> + struct iovec *iov;
> >> + int iov_nr;
> >> +
>
Re: [PATCH] vhost-blk: Add vhost-blk support v2
Hello Michael,
Thanks for the review!
On 10/11/2012 08:41 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 09, 2012 at 04:05:18PM +0800, Asias He wrote:
>> vhost-blk is an in-kernel virito-blk device accelerator.
>>
>> Due to lack of proper in-kernel AIO interface, this version converts
>> guest's I/O request to bio and use submit_bio() to submit I/O directly.
>> So this version any supports raw block device as guest's disk image,
>> e.g. /dev/sda, /dev/ram0. We can add file based image support to
>> vhost-blk once we have in-kernel AIO interface. There are some work in
>> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
>>
>>http://marc.info/?l=linux-fsdevel&m=133312234313122
>>
>> Performance evaluation:
>> -
>> 1) LKVM
>> Fio with libaio ioengine on Fusion IO device using kvm tool
>> IOPS Before After Improvement
>> seq-read 107 121 +13.0%
>> seq-write 130 179 +37.6%
>> rnd-read 102 122 +19.6%
>> rnd-write 125 159 +27.0%
>>
>> 2) QEMU
>> Fio with libaio ioengine on Fusion IO device using QEMU
>> IOPS Before After Improvement
>> seq-read 76 123 +61.8%
>> seq-write 139 173 +24.4%
>> rnd-read 73 120 +64.3%
>> rnd-write 75 156 +108.0%
>>
>> Userspace bits:
>> -
>> 1) LKVM
>> The latest vhost-blk userspace bits for kvm tool can be found here:
>> g...@github.com:asias/linux-kvm.git blk.vhost-blk
>>
>> 2) QEMU
>> The latest vhost-blk userspace prototype for QEMU can be found here:
>> g...@github.com:asias/qemu.git blk.vhost-blk
>>
>> Signed-off-by: Asias He
>> ---
>> drivers/vhost/Kconfig | 1 +
>> drivers/vhost/Kconfig.blk | 10 +
>> drivers/vhost/Makefile| 2 +
>> drivers/vhost/blk.c | 641
>> ++
>> drivers/vhost/blk.h | 8 +
>> 5 files changed, 662 insertions(+)
>> create mode 100644 drivers/vhost/Kconfig.blk
>> create mode 100644 drivers/vhost/blk.c
>> create mode 100644 drivers/vhost/blk.h
>>
>> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
>> index 202bba6..acd8038 100644
>> --- a/drivers/vhost/Kconfig
>> +++ b/drivers/vhost/Kconfig
>> @@ -11,4 +11,5 @@ config VHOST_NET
>>
>> if STAGING
>> source "drivers/vhost/Kconfig.tcm"
>> +source "drivers/vhost/Kconfig.blk"
>> endif
>> diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
>> new file mode 100644
>> index 000..ff8ab76
>> --- /dev/null
>> +++ b/drivers/vhost/Kconfig.blk
>> @@ -0,0 +1,10 @@
>> +config VHOST_BLK
>> +tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
>> +depends on BLOCK && EXPERIMENTAL && m
>> +---help---
>> + This kernel module can be loaded in host kernel to accelerate
>> + guest block with virtio_blk. Not to be confused with virtio_blk
>> + module itself which needs to be loaded in guest kernel.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called vhost_blk.
>> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
>> index a27b053..1a8a4a5 100644
>> --- a/drivers/vhost/Makefile
>> +++ b/drivers/vhost/Makefile
>> @@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
>> vhost_net-y := vhost.o net.o
>>
>> obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
>> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
>> +vhost_blk-y := blk.o
>> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
>> new file mode 100644
>> index 000..6b2445a
>> --- /dev/null
>> +++ b/drivers/vhost/blk.c
>> @@ -0,0 +1,641 @@
>> +/*
>> + * Copyright (C) 2011 Taobao, Inc.
>> + * Author: Liu Yuan
>> + *
>> + * Copyright (C) 2012 Red Hat, Inc.
>> + * Author: Asias He
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.
>> + *
>> + * virtio-blk server in host kernel.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "vhost.c"
>> +#include "vhost.h"
>> +#include "blk.h"
>> +
>> +#define BLK_HDR 0
>
> What's this for, exactly? Please add a comment.
The block headr is in the first and separate buffer.
>> +
>> +static DEFINE_IDA(vhost_blk_index_ida);
>> +
>> +enum {
>> +VHOST_BLK_VQ_REQ = 0,
>> +VHOST_BLK_VQ_MAX = 1,
>> +};
>> +
>> +struct req_page_list {
>> +struct page **pages;
>> +int pages_nr;
>> +};
>> +
>> +struct vhost_blk_req {
>> +struct llist_node llnode;
>> +struct req_page_list *pl;
>> +struct vhost_blk *blk;
>> +
>> +struct iovec *iov;
>> +int iov_nr;
>> +
>> +struct bio **bio;
>> +atomic_t bio_nr;
>> +
>> +sector_t sector;
>> +int write;
>> +u16 head;
>> +long len;
>> +
>> +u8 *status;
>
> Is this a userspace pointer? If yes it must be tagged as such.
Will fix.
> Please run code checker - it will catch other bugs for you too.
Could you name one tha
Re: [PATCH] vhost-blk: Add vhost-blk support v2
On Tue, Oct 09, 2012 at 04:05:18PM +0800, Asias He wrote:
> vhost-blk is an in-kernel virito-blk device accelerator.
>
> Due to lack of proper in-kernel AIO interface, this version converts
> guest's I/O request to bio and use submit_bio() to submit I/O directly.
> So this version any supports raw block device as guest's disk image,
> e.g. /dev/sda, /dev/ram0. We can add file based image support to
> vhost-blk once we have in-kernel AIO interface. There are some work in
> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
>
>http://marc.info/?l=linux-fsdevel&m=133312234313122
>
> Performance evaluation:
> -
> 1) LKVM
> Fio with libaio ioengine on Fusion IO device using kvm tool
> IOPS Before After Improvement
> seq-read 107 121 +13.0%
> seq-write 130 179 +37.6%
> rnd-read 102 122 +19.6%
> rnd-write 125 159 +27.0%
>
> 2) QEMU
> Fio with libaio ioengine on Fusion IO device using QEMU
> IOPS Before After Improvement
> seq-read 76 123 +61.8%
> seq-write 139 173 +24.4%
> rnd-read 73 120 +64.3%
> rnd-write 75 156 +108.0%
>
> Userspace bits:
> -
> 1) LKVM
> The latest vhost-blk userspace bits for kvm tool can be found here:
> g...@github.com:asias/linux-kvm.git blk.vhost-blk
>
> 2) QEMU
> The latest vhost-blk userspace prototype for QEMU can be found here:
> g...@github.com:asias/qemu.git blk.vhost-blk
>
> Signed-off-by: Asias He
> ---
> drivers/vhost/Kconfig | 1 +
> drivers/vhost/Kconfig.blk | 10 +
> drivers/vhost/Makefile| 2 +
> drivers/vhost/blk.c | 641
> ++
> drivers/vhost/blk.h | 8 +
> 5 files changed, 662 insertions(+)
> create mode 100644 drivers/vhost/Kconfig.blk
> create mode 100644 drivers/vhost/blk.c
> create mode 100644 drivers/vhost/blk.h
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 202bba6..acd8038 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -11,4 +11,5 @@ config VHOST_NET
>
> if STAGING
> source "drivers/vhost/Kconfig.tcm"
> +source "drivers/vhost/Kconfig.blk"
> endif
> diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
> new file mode 100644
> index 000..ff8ab76
> --- /dev/null
> +++ b/drivers/vhost/Kconfig.blk
> @@ -0,0 +1,10 @@
> +config VHOST_BLK
> + tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
> + depends on BLOCK && EXPERIMENTAL && m
> + ---help---
> + This kernel module can be loaded in host kernel to accelerate
> + guest block with virtio_blk. Not to be confused with virtio_blk
> + module itself which needs to be loaded in guest kernel.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called vhost_blk.
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index a27b053..1a8a4a5 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
> vhost_net-y := vhost.o net.o
>
> obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
> +vhost_blk-y := blk.o
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> new file mode 100644
> index 000..6b2445a
> --- /dev/null
> +++ b/drivers/vhost/blk.c
> @@ -0,0 +1,641 @@
> +/*
> + * Copyright (C) 2011 Taobao, Inc.
> + * Author: Liu Yuan
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + * Author: Asias He
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + *
> + * virtio-blk server in host kernel.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "vhost.c"
> +#include "vhost.h"
> +#include "blk.h"
> +
> +#define BLK_HDR 0
What's this for, exactly? Please add a comment.
> +
> +static DEFINE_IDA(vhost_blk_index_ida);
> +
> +enum {
> + VHOST_BLK_VQ_REQ = 0,
> + VHOST_BLK_VQ_MAX = 1,
> +};
> +
> +struct req_page_list {
> + struct page **pages;
> + int pages_nr;
> +};
> +
> +struct vhost_blk_req {
> + struct llist_node llnode;
> + struct req_page_list *pl;
> + struct vhost_blk *blk;
> +
> + struct iovec *iov;
> + int iov_nr;
> +
> + struct bio **bio;
> + atomic_t bio_nr;
> +
> + sector_t sector;
> + int write;
> + u16 head;
> + long len;
> +
> + u8 *status;
Is this a userspace pointer? If yes it must be tagged as such.
Please run code checker - it will catch other bugs for you too.
> +};
> +
> +struct vhost_blk {
> + struct task_struct *host_kick;
> + struct vhost_blk_req *reqs;
> + struct vhost_virtqueue vq;
> + struct llist_head llhead;
> + struct vhost_dev dev;
> + u16 reqs_nr;
> + int index;
> +};
> +
> +static inline int iov_num_pages(struct iovec *io
Re: [PATCH] vhost-blk: Add vhost-blk support v2
Hello Christoph!
On 10/10/2012 01:39 AM, Christoph Hellwig wrote:
>> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file
>> *file)
>> +{
>> +
>> +struct inode *inode = file->f_mapping->host;
>> +struct block_device *bdev = inode->i_bdev;
>> +int ret;
>
> Please just pass the block_device directly instead of a file struct.
vhost_blk_req_submit() can be used to handle file based image later.
Using the file interface will work for both cases.
I do need a check in vhost_blk_set_backend() to tell if the user passed
file is a raw device file for now.
>> +
>> +ret = vhost_blk_bio_make(req, bdev);
>> +if (ret < 0)
>> +return ret;
>> +
>> +vhost_blk_bio_send(req);
>> +
>> +return ret;
>> +}
>
> Then again how simple the this function is it probably should just go
> away entirely.
No, vhost_blk_req_submit() is used for both read and write ops. It makes
no sense to write the code twice. Plus, this function might be complexer
when the file based image support is added.
>> +set_fs(USER_DS);
>
> What do we actually need the set_fs for here?
See this commit: d7ffde35e31a81100d2d0d2c4013cbf527bb32ea
>> +
>> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
>> +{
>> +
>> +*file = vhost_blk_stop_vq(blk, &blk->vq);
>> +}
>
> What is the point of this helper? Also I can't see anyone actually
> using the returned struct file.
It is used in both vhost_blk_reset_owner() and vhost_blk_release(). The
returned struct file is used for fput(). We have similar helper in
vhost_net: vhost_net_stop().
>> +case VIRTIO_BLK_T_FLUSH:
>> +ret = vfs_fsync(file, 1);
>> +status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
>> +if (!vhost_blk_set_status(req, status))
>> +vhost_add_used_and_signal(&blk->dev, vq, head, ret);
>> +break;
>
> Sending a fsync here is actually wrong in two different ways:
>
> a) it operates at the filesystem level instead of bio level
> b) it's a blocking operation
>
> It should instead send a REQ_FLUSH bio using the same submission scheme
> as the read/write requests.
Will fix this.
Thanks for the review!
--
Asias
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-blk: Add vhost-blk support v2
> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
> +{
> +
> + struct inode *inode = file->f_mapping->host;
> + struct block_device *bdev = inode->i_bdev;
> + int ret;
Please just pass the block_device directly instead of a file struct.
> +
> + ret = vhost_blk_bio_make(req, bdev);
> + if (ret < 0)
> + return ret;
> +
> + vhost_blk_bio_send(req);
> +
> + return ret;
> +}
Then again how simple the this function is it probably should just go
away entirely.
> + set_fs(USER_DS);
What do we actually need the set_fs for here?
> +
> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
> +{
> +
> + *file = vhost_blk_stop_vq(blk, &blk->vq);
> +}
What is the point of this helper? Also I can't see anyone actually
using the returned struct file.
> + case VIRTIO_BLK_T_FLUSH:
> + ret = vfs_fsync(file, 1);
> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> + if (!vhost_blk_set_status(req, status))
> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> + break;
Sending a fsync here is actually wrong in two different ways:
a) it operates at the filesystem level instead of bio level
b) it's a blocking operation
It should instead send a REQ_FLUSH bio using the same submission scheme
as the read/write requests.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
