* Re: [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION
From: Michael S. Tsirkin @ 2012-07-29 12:27 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, linux-scsi, kvm-devel, Greg Kroah-Hartman, qemu-devel,
lf-virt, Anthony Liguori, target-devel, Avi Kivity, Paolo Bonzini,
Zhi Yong Wu, Christoph Hellwig, Stefan Hajnoczi
In-Reply-To: <1343250648.1813.169.camel@haakon2.linux-iscsi.org>
On Wed, Jul 25, 2012 at 02:10:48PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2012-07-25 at 16:10 +0300, Avi Kivity wrote:
> > On 07/24/2012 11:45 PM, Nicholas A. Bellinger wrote:
> >
> > >> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > >> > index e942df9..3d5378f 100644
> > >> > --- a/drivers/vhost/tcm_vhost.h
> > >> > +++ b/drivers/vhost/tcm_vhost.h
> > >> > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> > >> >
> > >> > #include <linux/vhost.h>
> > >> >
> > >> > +/*
> > >> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > >> > + *
> > >> > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > >> > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > >> > + */
> >
> >
> > If it's out of tree, why consider it at all? Put a stable ABI in tree
> > and extend it in compatible ways.
> >
> >
>
> This comment was supposed to convey that ABI=0 vhost-scsi userspace code
> is not supported with tcm_vhost mainline code.
>
> But obviously that was not clear enough here. Updating the comment to
> reflect to make this clear.
>
> So the main question here was if it's fine to start with ABI=1, and
> require >= ABI=1 for all vhost-scsi userspace code to function with
> tcm_vhost.
>
> The idea was to avoid confusion for the ABI=0 vhost-scsi code that's
> been floating around for the last 2 years.
>
> --nab
>
>
>
>
>
>
Yes but I am guessing that code does not invoke the ABI ioctl?
--
MST
^ permalink raw reply
* Re: [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION
From: Michael S. Tsirkin @ 2012-07-29 12:35 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, Stefan Hajnoczi, linux-scsi, Greg Kroah-Hartman,
qemu-devel, lf-virt, Anthony Liguori, target-devel, Avi Kivity,
kvm-devel, Paolo Bonzini, Zhi Yong Wu, Christoph Hellwig
In-Reply-To: <1343333727.22538.3.camel@haakon2.linux-iscsi.org>
On Thu, Jul 26, 2012 at 01:15:26PM -0700, Nicholas A. Bellinger wrote:
> If there is a better way to handle ioctl compat I'd
> certainly like to hear about it.
You basically don't touch exiting commands, always add new ones.
For example if you want to simply stop some ioctl from working
for old userspace, the simplest way is to change ioctl number.
--
MST
^ permalink raw reply
* Re: [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION
From: Avi Kivity @ 2012-07-29 12:50 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
Greg Kroah-Hartman, qemu-devel, lf-virt, Anthony Liguori,
target-devel, linux-scsi, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <1343333727.22538.3.camel@haakon2.linux-iscsi.org>
On 07/26/2012 11:15 PM, Nicholas A. Bellinger wrote:
>>
>
> Example..? If there is a better way to handle ioctl compat I'd
> certainly like to hear about it.
>
r = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_ASSIGN_DEV_IRQ);
if (r == -1)
...
if (r)
// ioctl(fd, KVM_ASSIGN_DEV_IRQ, ...) is available
else
not available
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Michael S. Tsirkin @ 2012-07-29 12:59 UTC (permalink / raw)
To: Asias He
Cc: kvm, linux-kernel, virtualization, Paolo Bonzini,
Christoph Hellwig
In-Reply-To: <501350B1.7080908@redhat.com>
On Sat, Jul 28, 2012 at 10:38:41AM +0800, Asias He wrote:
> On 07/27/2012 08:33 AM, Rusty Russell wrote:
> >On Fri, 13 Jul 2012 16:38:51 +0800, Asias He <asias@redhat.com> wrote:
> >>Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
> >>use_bio=1' to enable ->make_request_fn() based I/O path.
> >
> >This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
> >host cache between writeback and writethrough' which is also queued (see
> >linux-next).
>
> Rebased against Paolo's patch in V4.
>
> >I'm not sure what the correct behavior for bio & cacheflush is, if any.
>
> REQ_FLUSH is not supported in the bio path.
>
> >But as to the patch itself: it's a hack.
> >
> >1) Leaving the guest's admin to turn on the switch is a terrible choice.
> >2) The block layer should stop merging and sorting when a device is
> > fast, not the driver.
> >3) I pointed out that slow disks have low IOPS, so why is this
> > conditional? Sure, more guest exits, but it's still a small number
> > for a slow device.
> >4) The only case where we want merging is on a slow device when the host
> > isn't doing it.
> >
> >Now, despite this, I'm prepared to commit it. But in my mind it's a
> >hack: we should aim for use_bio to be based on a feature bit fed from
> >the host, and use the module parameter only if we want to override it.
>
> OK. A feature bit from host sound like a choice but a switch is also
> needed on host side.
qemu automatically gives you the ability to control
any feature bit.
> And for other OS, e.g. Windows, the bio thing
> does not apply at all.
Let's try to define when it's a good idea. Is it a hint to guest that
backend handles small accesses efficiently so ok to disable batching?
> Anyway, I have to admit that adding a module parameter here is not
> the best choice. Let's think more.
>
> --
> Asias
^ permalink raw reply
* Re: [RFC-v5] tcm_vhost: Initial merge for vhost level target fabric driver
From: Michael S. Tsirkin @ 2012-07-29 13:40 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, Anthony Liguori, Stefan Hajnoczi, kvm-devel,
Greg Kroah-Hartman, qemu-devel, lf-virt, Anthony Liguori,
target-devel, linux-scsi, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <1343346200-19850-1-git-send-email-nab@linux-iscsi.org>
On Thu, Jul 26, 2012 at 11:43:20PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Hello KVM + QEMU folks,
>
> This is -v5 of the in-flight tcm_vhost fabric driver for KVM host virtualized
> target support using the in-kernel storage stack with for-3.6 code.
>
> The changes since -v4 have been pretty minimal. A new GET_ABI_VERSION ioctl
> has been added as requested by Anthony & Co, and after some back <-> forth on
> the lists we've decided upon a starting point of VHOST_SCSI_ABI_VERSION=0,
> and vhost-scsi RFC userspace has been updated accordingly.
>
> Along with that, a few handfuls of vq related error messages where converted
> to use vq_err as requested by MST, along with some minor checkpatch fixes.
>
> So this point everything required to run tcm_vhost from drivers/target +
> drivers/vhost, and virtio-scsi LLD bugfix has been merged into v3.6-rc0 code.
>
> Please have a look and let us know if there are any last-minute pre-merge
> comments ahead of MST returning from holiday this weekend.
>
> Thank you!
>
> --nab
Acked-by: Michael S. Tsirkin <mst@redhat.com>
some comments below but can be fixed in-tree I think.
> Changelog v4 -> v5:
>
> Expose ABI version via VHOST_SCSI_GET_ABI_VERSION + use Rev 0 as
> starting point for v3.6-rc code (Stefan + ALiguori + nab)
> Convert vhost_scsi_handle_vq() to vq_err() (nab + MST)
> Minor style fixes from checkpatch (nab)
>
> Changelog v3 -> v4:
>
> Rename vhost_vring_target -> vhost_scsi_target (mst + nab)
> Use TRANSPORT_IQN_LEN in vhost_scsi_target->vhost_wwpn[] def (nab)
> Move back to drivers/vhost/, and just use drivers/vhost/Kconfig.tcm (mst)
> Move TCM_VHOST related ioctl defines from include/linux/vhost.h ->
> drivers/vhost/tcm_vhost.h as requested by MST (nab)
> Move Kbuild.tcm include from drivers/staging -> drivers/vhost/, and
> just use 'if STAGING' around 'source drivers/vhost/Kbuild.tcm'
>
> Changelog v2 -> v3:
>
> Unlock on error in tcm_vhost_drop_nexus() (DanC)
> Fix strlen() doesn't count the terminator (DanC)
> Call kfree() on an error path (DanC)
> Convert tcm_vhost_write_pending to use target_execute_cmd (hch + nab)
> Fix another strlen() off by one in tcm_vhost_make_tport (DanC)
> Add option under drivers/staging/Kconfig, and move to drivers/vhost/tcm/
> as requested by MST (nab)
>
> Changelog v1 -> v2:
>
> Fix tv_cmd completion -> release SGL memory leak (nab)
> Fix sparse warnings for static variable usage ((Fengguang Wu)
> Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
> Convert to cmwq submission for I/O dispatch (nab + hch)
>
> Changelog v0 -> v1:
>
> Merge into single source + header file, and move to drivers/vhost/
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Cc: Zhi Yong Wu <wuzhy@cn.ibm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/vhost/Kconfig | 3 +
> drivers/vhost/Kconfig.tcm | 6 +
> drivers/vhost/Makefile | 2 +
> drivers/vhost/tcm_vhost.c | 1628 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/tcm_vhost.h | 101 +++
> 5 files changed, 1740 insertions(+), 0 deletions(-)
> create mode 100644 drivers/vhost/Kconfig.tcm
> create mode 100644 drivers/vhost/tcm_vhost.c
> create mode 100644 drivers/vhost/tcm_vhost.h
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index e4e2fd1..202bba6 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -9,3 +9,6 @@ config VHOST_NET
> To compile this driver as a module, choose M here: the module will
> be called vhost_net.
>
> +if STAGING
> +source "drivers/vhost/Kconfig.tcm"
> +endif
> diff --git a/drivers/vhost/Kconfig.tcm b/drivers/vhost/Kconfig.tcm
> new file mode 100644
> index 0000000..a9c6f76
> --- /dev/null
> +++ b/drivers/vhost/Kconfig.tcm
> @@ -0,0 +1,6 @@
> +config TCM_VHOST
> + tristate "TCM_VHOST fabric module (EXPERIMENTAL)"
> + depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && m
> + default n
> + ---help---
> + Say M here to enable the TCM_VHOST fabric module for use with virtio-scsi guests
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index 72dd020..a27b053 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -1,2 +1,4 @@
> obj-$(CONFIG_VHOST_NET) += vhost_net.o
> vhost_net-y := vhost.o net.o
> +
> +obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> new file mode 100644
> index 0000000..fb36654
> --- /dev/null
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -0,0 +1,1628 @@
> +/*******************************************************************************
> + * Vhost kernel TCM fabric driver for virtio SCSI initiators
> + *
> + * (C) Copyright 2010-2012 RisingTide Systems LLC.
> + * (C) Copyright 2010-2012 IBM Corp.
> + *
> + * Licensed to the Linux Foundation under the General Public License (GPL) version 2.
> + *
> + * Authors: Nicholas A. Bellinger <nab@risingtidesystems.com>
> + * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + ****************************************************************************/
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <generated/utsrelease.h>
> +#include <linux/utsname.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/kthread.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +#include <linux/configfs.h>
> +#include <linux/ctype.h>
> +#include <linux/compat.h>
> +#include <linux/eventfd.h>
> +#include <linux/vhost.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <asm/unaligned.h>
> +#include <scsi/scsi.h>
> +#include <scsi/scsi_tcq.h>
> +#include <target/target_core_base.h>
> +#include <target/target_core_fabric.h>
> +#include <target/target_core_fabric_configfs.h>
> +#include <target/target_core_configfs.h>
> +#include <target/configfs_macros.h>
> +#include <linux/vhost.h>
> +#include <linux/virtio_net.h> /* TODO vhost.h currently depends on this */
> +#include <linux/virtio_scsi.h>
> +
> +#include "vhost.c"
> +#include "vhost.h"
> +#include "tcm_vhost.h"
> +
> +struct vhost_scsi {
> + atomic_t vhost_ref_cnt;
> + struct tcm_vhost_tpg *vs_tpg;
Would b enice to add a note on locking about this field.
> + struct vhost_dev dev;
> + struct vhost_virtqueue vqs[3];
> +
> + struct vhost_work vs_completion_work; /* cmd completion work item */
> + struct list_head vs_completion_list; /* cmd completion queue */
> + spinlock_t vs_completion_lock; /* protects s_completion_list */
> +};
> +
> +/* Local pointer to allocated TCM configfs fabric module */
> +static struct target_fabric_configfs *tcm_vhost_fabric_configfs;
> +
> +static struct workqueue_struct *tcm_vhost_workqueue;
> +
> +/* Global spinlock to protect tcm_vhost TPG list for vhost IOCTL access */
> +static DEFINE_MUTEX(tcm_vhost_mutex);
> +static LIST_HEAD(tcm_vhost_list);
> +
> +static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
> +{
> + return 1;
> +}
> +
> +static int tcm_vhost_check_false(struct se_portal_group *se_tpg)
> +{
> + return 0;
> +}
> +
> +static char *tcm_vhost_get_fabric_name(void)
> +{
> + return "vhost";
> +}
> +
> +static u8 tcm_vhost_get_fabric_proto_ident(struct se_portal_group *se_tpg)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_tport *tport = tpg->tport;
> +
> + switch (tport->tport_proto_id) {
> + case SCSI_PROTOCOL_SAS:
> + return sas_get_fabric_proto_ident(se_tpg);
> + case SCSI_PROTOCOL_FCP:
> + return fc_get_fabric_proto_ident(se_tpg);
> + case SCSI_PROTOCOL_ISCSI:
> + return iscsi_get_fabric_proto_ident(se_tpg);
> + default:
> + pr_err("Unknown tport_proto_id: 0x%02x, using"
> + " SAS emulation\n", tport->tport_proto_id);
> + break;
> + }
> +
> + return sas_get_fabric_proto_ident(se_tpg);
> +}
> +
> +static char *tcm_vhost_get_fabric_wwn(struct se_portal_group *se_tpg)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_tport *tport = tpg->tport;
> +
> + return &tport->tport_name[0];
> +}
> +
> +static u16 tcm_vhost_get_tag(struct se_portal_group *se_tpg)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + return tpg->tport_tpgt;
> +}
> +
> +static u32 tcm_vhost_get_default_depth(struct se_portal_group *se_tpg)
> +{
> + return 1;
> +}
> +
> +static u32 tcm_vhost_get_pr_transport_id(
> + struct se_portal_group *se_tpg,
> + struct se_node_acl *se_nacl,
> + struct t10_pr_registration *pr_reg,
> + int *format_code,
> + unsigned char *buf)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_tport *tport = tpg->tport;
> +
> + switch (tport->tport_proto_id) {
> + case SCSI_PROTOCOL_SAS:
> + return sas_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> + format_code, buf);
> + case SCSI_PROTOCOL_FCP:
> + return fc_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> + format_code, buf);
> + case SCSI_PROTOCOL_ISCSI:
> + return iscsi_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> + format_code, buf);
> + default:
> + pr_err("Unknown tport_proto_id: 0x%02x, using"
> + " SAS emulation\n", tport->tport_proto_id);
> + break;
> + }
> +
> + return sas_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> + format_code, buf);
> +}
> +
> +static u32 tcm_vhost_get_pr_transport_id_len(
> + struct se_portal_group *se_tpg,
> + struct se_node_acl *se_nacl,
> + struct t10_pr_registration *pr_reg,
> + int *format_code)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_tport *tport = tpg->tport;
> +
> + switch (tport->tport_proto_id) {
> + case SCSI_PROTOCOL_SAS:
> + return sas_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> + format_code);
> + case SCSI_PROTOCOL_FCP:
> + return fc_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> + format_code);
> + case SCSI_PROTOCOL_ISCSI:
> + return iscsi_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> + format_code);
> + default:
> + pr_err("Unknown tport_proto_id: 0x%02x, using"
> + " SAS emulation\n", tport->tport_proto_id);
> + break;
> + }
> +
> + return sas_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> + format_code);
> +}
> +
> +static char *tcm_vhost_parse_pr_out_transport_id(
> + struct se_portal_group *se_tpg,
> + const char *buf,
> + u32 *out_tid_len,
> + char **port_nexus_ptr)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_tport *tport = tpg->tport;
> +
> + switch (tport->tport_proto_id) {
> + case SCSI_PROTOCOL_SAS:
> + return sas_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> + port_nexus_ptr);
> + case SCSI_PROTOCOL_FCP:
> + return fc_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> + port_nexus_ptr);
> + case SCSI_PROTOCOL_ISCSI:
> + return iscsi_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> + port_nexus_ptr);
> + default:
> + pr_err("Unknown tport_proto_id: 0x%02x, using"
> + " SAS emulation\n", tport->tport_proto_id);
> + break;
> + }
> +
> + return sas_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> + port_nexus_ptr);
> +}
> +
> +static struct se_node_acl *tcm_vhost_alloc_fabric_acl(
> + struct se_portal_group *se_tpg)
> +{
> + struct tcm_vhost_nacl *nacl;
> +
> + nacl = kzalloc(sizeof(struct tcm_vhost_nacl), GFP_KERNEL);
> + if (!nacl) {
> + pr_err("Unable to alocate struct tcm_vhost_nacl\n");
> + return NULL;
> + }
> +
> + return &nacl->se_node_acl;
> +}
> +
> +static void tcm_vhost_release_fabric_acl(
> + struct se_portal_group *se_tpg,
> + struct se_node_acl *se_nacl)
> +{
> + struct tcm_vhost_nacl *nacl = container_of(se_nacl,
> + struct tcm_vhost_nacl, se_node_acl);
> + kfree(nacl);
> +}
> +
> +static u32 tcm_vhost_tpg_get_inst_index(struct se_portal_group *se_tpg)
> +{
> + return 1;
> +}
> +
> +static void tcm_vhost_release_cmd(struct se_cmd *se_cmd)
> +{
> + return;
> +}
> +
> +static int tcm_vhost_shutdown_session(struct se_session *se_sess)
> +{
> + return 0;
> +}
> +
> +static void tcm_vhost_close_session(struct se_session *se_sess)
> +{
> + return;
> +}
> +
> +static u32 tcm_vhost_sess_get_index(struct se_session *se_sess)
> +{
> + return 0;
> +}
> +
> +static int tcm_vhost_write_pending(struct se_cmd *se_cmd)
> +{
> + /* Go ahead and process the write immediately */
> + target_execute_cmd(se_cmd);
> + return 0;
> +}
> +
> +static int tcm_vhost_write_pending_status(struct se_cmd *se_cmd)
> +{
> + return 0;
> +}
> +
> +static void tcm_vhost_set_default_node_attrs(struct se_node_acl *nacl)
> +{
> + return;
> +}
> +
> +static u32 tcm_vhost_get_task_tag(struct se_cmd *se_cmd)
> +{
> + return 0;
> +}
> +
> +static int tcm_vhost_get_cmd_state(struct se_cmd *se_cmd)
> +{
> + return 0;
> +}
> +
> +static void vhost_scsi_complete_cmd(struct tcm_vhost_cmd *);
can we reorder code to avoid forward declarations?
> +
> +static int tcm_vhost_queue_data_in(struct se_cmd *se_cmd)
> +{
> + struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
> + struct tcm_vhost_cmd, tvc_se_cmd);
> + vhost_scsi_complete_cmd(tv_cmd);
> + return 0;
> +}
> +
> +static int tcm_vhost_queue_status(struct se_cmd *se_cmd)
> +{
> + struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
> + struct tcm_vhost_cmd, tvc_se_cmd);
> + vhost_scsi_complete_cmd(tv_cmd);
> + return 0;
> +}
> +
> +static int tcm_vhost_queue_tm_rsp(struct se_cmd *se_cmd)
> +{
> + return 0;
> +}
> +
> +static u16 tcm_vhost_set_fabric_sense_len(struct se_cmd *se_cmd,
> + u32 sense_length)
> +{
> + return 0;
> +}
> +
> +static u16 tcm_vhost_get_fabric_sense_len(void)
> +{
> + return 0;
> +}
> +
> +static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> +{
> + struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
> +
> + /* TODO locking against target/backend threads? */
> + transport_generic_free_cmd(se_cmd, 1);
> +
> + if (tv_cmd->tvc_sgl_count) {
> + u32 i;
> + for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
> + put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> +
> + kfree(tv_cmd->tvc_sgl);
> + }
> +
> + kfree(tv_cmd);
> +}
> +
> +/* Dequeue a command from the completion list */
> +static struct tcm_vhost_cmd *vhost_scsi_get_cmd_from_completion(
> + struct vhost_scsi *vs)
> +{
> + struct tcm_vhost_cmd *tv_cmd = NULL;
> +
> + spin_lock_bh(&vs->vs_completion_lock);
> + if (list_empty(&vs->vs_completion_list)) {
> + spin_unlock_bh(&vs->vs_completion_lock);
> + return NULL;
> + }
> +
> + list_for_each_entry(tv_cmd, &vs->vs_completion_list,
> + tvc_completion_list) {
> + list_del(&tv_cmd->tvc_completion_list);
> + break;
> + }
> + spin_unlock_bh(&vs->vs_completion_lock);
> + return tv_cmd;
> +}
> +
> +/* Fill in status and signal that we are done processing this command
> + *
> + * This is scheduled in the vhost work queue so we are called with the owner
> + * process mm and can access the vring.
> + */
> +static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
> +{
> + struct vhost_scsi *vs = container_of(work, struct vhost_scsi,
> + vs_completion_work);
> + struct tcm_vhost_cmd *tv_cmd;
> +
> + while ((tv_cmd = vhost_scsi_get_cmd_from_completion(vs)) != NULL) {
I normally prefer while ((tv_cmd = vhost_scsi_get_cmd_from_completion(vs)))
without != NULL
> + struct virtio_scsi_cmd_resp v_rsp;
> + struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
> + int ret;
> +
> + pr_debug("%s tv_cmd %p resid %u status %#02x\n", __func__,
> + tv_cmd, se_cmd->residual_count, se_cmd->scsi_status);
> +
> + memset(&v_rsp, 0, sizeof(v_rsp));
> + v_rsp.resid = se_cmd->residual_count;
> + /* TODO is status_qualifier field needed? */
> + v_rsp.status = se_cmd->scsi_status;
> + v_rsp.sense_len = se_cmd->scsi_sense_length;
> + memcpy(v_rsp.sense, tv_cmd->tvc_sense_buf,
> + v_rsp.sense_len);
> + ret = copy_to_user(tv_cmd->tvc_resp, &v_rsp, sizeof(v_rsp));
> + if (likely(ret == 0))
> + vhost_add_used(&vs->vqs[2], tv_cmd->tvc_vq_desc, 0);
> + else
> + pr_err("Faulted on virtio_scsi_cmd_resp\n");
> +
> + vhost_scsi_free_cmd(tv_cmd);
> + }
> +
> + vhost_signal(&vs->dev, &vs->vqs[2]);
> +}
> +
> +static void vhost_scsi_complete_cmd(struct tcm_vhost_cmd *tv_cmd)
> +{
> + struct vhost_scsi *vs = tv_cmd->tvc_vhost;
> +
> + pr_debug("%s tv_cmd %p\n", __func__, tv_cmd);
Is this doing something you can not already do with
tracing? If all these pr_debug messages are useful,
I think it's best to add a kconfig entry for them
so that they are off for people even if they build
debug kernels.
> +
> + spin_lock_bh(&vs->vs_completion_lock);
> + list_add_tail(&tv_cmd->tvc_completion_list, &vs->vs_completion_list);
> + spin_unlock_bh(&vs->vs_completion_lock);
> +
> + vhost_work_queue(&vs->dev, &vs->vs_completion_work);
> +}
> +
> +static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
> + struct tcm_vhost_tpg *tv_tpg,
> + struct virtio_scsi_cmd_req *v_req,
> + u32 exp_data_len,
> + int data_direction)
> +{
> + struct tcm_vhost_cmd *tv_cmd;
> + struct tcm_vhost_nexus *tv_nexus;
> + struct se_portal_group *se_tpg = &tv_tpg->se_tpg;
> + struct se_session *se_sess;
> + struct se_cmd *se_cmd;
> + int sam_task_attr;
> +
> + tv_nexus = tv_tpg->tpg_nexus;
> + if (!tv_nexus) {
> + pr_err("Unable to locate active struct tcm_vhost_nexus\n");
> + return ERR_PTR(-EIO);
> + }
> + se_sess = tv_nexus->tvn_se_sess;
> +
> + tv_cmd = kzalloc(sizeof(struct tcm_vhost_cmd), GFP_ATOMIC);
> + if (!tv_cmd) {
> + pr_err("Unable to allocate struct tcm_vhost_cmd\n");
> + return ERR_PTR(-ENOMEM);
> + }
> + INIT_LIST_HEAD(&tv_cmd->tvc_completion_list);
> + tv_cmd->tvc_tag = v_req->tag;
> +
> + se_cmd = &tv_cmd->tvc_se_cmd;
> + /*
> + * Locate the SAM Task Attr from virtio_scsi_cmd_req
> + */
> + sam_task_attr = v_req->task_attr;
> + /*
> + * Initialize struct se_cmd descriptor from TCM infrastructure
> + */
> + transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, exp_data_len,
> + data_direction, sam_task_attr,
> + &tv_cmd->tvc_sense_buf[0]);
> +
> +#if 0 /* FIXME: vhost_scsi_allocate_cmd() BIDI operation */
> + if (bidi)
> + se_cmd->se_cmd_flags |= SCF_BIDI;
> +#endif
> + return tv_cmd;
> +}
> +
> +/*
> + * Map a user memory range into a scatterlist
> + *
> + * Returns the number of scatterlist entries used or -errno on error.
> + */
> +static int vhost_scsi_map_to_sgl(struct scatterlist *sgl,
> + unsigned int sgl_count, void __user *ptr, size_t len, int write)
> +{
> + struct scatterlist *sg = sgl;
> + unsigned int npages = 0;
> + int ret;
> +
> + while (len > 0) {
> + struct page *page;
> + unsigned int offset = (uintptr_t)ptr & ~PAGE_MASK;
> + unsigned int nbytes = min_t(unsigned int,
> + PAGE_SIZE - offset, len);
> +
> + if (npages == sgl_count) {
> + ret = -ENOBUFS;
> + goto err;
> + }
> +
> + ret = get_user_pages_fast((unsigned long)ptr, 1, write, &page);
> + BUG_ON(ret == 0); /* we should either get our page or fail */
> + if (ret < 0)
> + goto err;
> +
> + sg_set_page(sg, page, nbytes, offset);
> + ptr += nbytes;
> + len -= nbytes;
> + sg++;
> + npages++;
> + }
> + return npages;
> +
> +err:
> + /* Put pages that we hold */
> + for (sg = sgl; sg != &sgl[npages]; sg++)
> + put_page(sg_page(sg));
> + return ret;
> +}
> +
> +static int vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *tv_cmd,
> + struct iovec *iov, unsigned int niov, int write)
> +{
> + int ret;
> + unsigned int i;
> + u32 sgl_count;
> + struct scatterlist *sg;
> +
> + /*
> + * Find out how long sglist needs to be
> + */
> + sgl_count = 0;
> + for (i = 0; i < niov; i++) {
> + sgl_count += (((uintptr_t)iov[i].iov_base + iov[i].iov_len +
> + PAGE_SIZE - 1) >> PAGE_SHIFT) -
> + ((uintptr_t)iov[i].iov_base >> PAGE_SHIFT);
> + }
> + /* TODO overflow checking */
> +
> + sg = kmalloc(sizeof(tv_cmd->tvc_sgl[0]) * sgl_count, GFP_ATOMIC);
> + if (!sg)
> + return -ENOMEM;
> + pr_debug("%s sg %p sgl_count %u is_err %ld\n", __func__,
> + sg, sgl_count, IS_ERR(sg));
> + sg_init_table(sg, sgl_count);
> +
> + tv_cmd->tvc_sgl = sg;
> + tv_cmd->tvc_sgl_count = sgl_count;
> +
> + pr_debug("Mapping %u iovecs for %u pages\n", niov, sgl_count);
> + for (i = 0; i < niov; i++) {
> + ret = vhost_scsi_map_to_sgl(sg, sgl_count, iov[i].iov_base,
> + iov[i].iov_len, write);
> + if (ret < 0) {
> + for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
> + put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> + kfree(tv_cmd->tvc_sgl);
> + tv_cmd->tvc_sgl = NULL;
> + tv_cmd->tvc_sgl_count = 0;
> + return ret;
> + }
> +
> + sg += ret;
> + sgl_count -= ret;
> + }
> + return 0;
> +}
> +
> +static void tcm_vhost_submission_work(struct work_struct *work)
> +{
> + struct tcm_vhost_cmd *tv_cmd =
> + container_of(work, struct tcm_vhost_cmd, work);
> + struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
> + struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
> + int rc, sg_no_bidi = 0;
> + /*
> + * Locate the struct se_lun pointer based on v_req->lun, and
> + * attach it to struct se_cmd
> + */
> + rc = transport_lookup_cmd_lun(&tv_cmd->tvc_se_cmd, tv_cmd->tvc_lun);
> + if (rc < 0) {
> + pr_err("Failed to look up lun: %d\n", tv_cmd->tvc_lun);
> + transport_send_check_condition_and_sense(&tv_cmd->tvc_se_cmd,
> + tv_cmd->tvc_se_cmd.scsi_sense_reason, 0);
> + transport_generic_free_cmd(se_cmd, 0);
> + return;
> + }
> +
> + rc = target_setup_cmd_from_cdb(se_cmd, tv_cmd->tvc_cdb);
> + if (rc == -ENOMEM) {
> + transport_send_check_condition_and_sense(se_cmd,
> + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
> + transport_generic_free_cmd(se_cmd, 0);
> + return;
> + } else if (rc < 0) {
> + if (se_cmd->se_cmd_flags & SCF_SCSI_RESERVATION_CONFLICT)
> + tcm_vhost_queue_status(se_cmd);
> + else
> + transport_send_check_condition_and_sense(se_cmd,
> + se_cmd->scsi_sense_reason, 0);
> + transport_generic_free_cmd(se_cmd, 0);
> + return;
> + }
> +
> + if (tv_cmd->tvc_sgl_count) {
> + sg_ptr = tv_cmd->tvc_sgl;
> + /*
> + * For BIDI commands, pass in the extra READ buffer
> + * to transport_generic_map_mem_to_cmd() below..
> + */
> +/* FIXME: Fix BIDI operation in tcm_vhost_submission_work() */
> +#if 0
> + if (se_cmd->se_cmd_flags & SCF_BIDI) {
> + sg_bidi_ptr = NULL;
> + sg_no_bidi = 0;
> + }
> +#endif
> + } else {
> + sg_ptr = NULL;
> + }
> +
> + rc = transport_generic_map_mem_to_cmd(se_cmd, sg_ptr,
> + tv_cmd->tvc_sgl_count, sg_bidi_ptr,
> + sg_no_bidi);
> + if (rc < 0) {
> + transport_send_check_condition_and_sense(se_cmd,
> + se_cmd->scsi_sense_reason, 0);
> + transport_generic_free_cmd(se_cmd, 0);
> + return;
> + }
> + transport_handle_cdb_direct(se_cmd);
> +}
> +
> +static void vhost_scsi_handle_vq(struct vhost_scsi *vs)
> +{
> + struct vhost_virtqueue *vq = &vs->vqs[2];
> + struct virtio_scsi_cmd_req v_req;
> + struct tcm_vhost_tpg *tv_tpg;
> + struct tcm_vhost_cmd *tv_cmd;
> + u32 exp_data_len, data_first, data_num, data_direction;
> + unsigned out, in, i;
> + int head, ret;
> +
> + /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
> + tv_tpg = vs->vs_tpg;
> + if (unlikely(!tv_tpg)) {
> + pr_err("%s endpoint not set\n", __func__);
> + return;
> + }
> +
> + mutex_lock(&vq->mutex);
> + vhost_disable_notify(&vs->dev, vq);
> +
> + for (;;) {
> + head = vhost_get_vq_desc(&vs->dev, vq, vq->iov,
> + ARRAY_SIZE(vq->iov), &out, &in,
> + NULL, NULL);
> + pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n",
> + head, out, in);
> + /* On error, stop handling until the next kick. */
> + if (unlikely(head < 0))
> + break;
> + /* Nothing new? Wait for eventfd to tell us they refilled. */
> + if (head == vq->num) {
> + if (unlikely(vhost_enable_notify(&vs->dev, vq))) {
> + vhost_disable_notify(&vs->dev, vq);
> + continue;
> + }
> + break;
> + }
> +
> +/* FIXME: BIDI operation */
> + if (out == 1 && in == 1) {
> + data_direction = DMA_NONE;
> + data_first = 0;
> + data_num = 0;
> + } else if (out == 1 && in > 1) {
> + data_direction = DMA_FROM_DEVICE;
> + data_first = out + 1;
> + data_num = in - 1;
> + } else if (out > 1 && in == 1) {
> + data_direction = DMA_TO_DEVICE;
> + data_first = 1;
> + data_num = out - 1;
> + } else {
> + vq_err(vq, "Invalid buffer layout out: %u in: %u\n",
> + out, in);
> + break;
> + }
> +
> + /*
> + * Check for a sane resp buffer so we can report errors to
> + * the guest.
> + */
> + if (unlikely(vq->iov[out].iov_len !=
> + sizeof(struct virtio_scsi_cmd_resp))) {
> + vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
> + " bytes\n", vq->iov[out].iov_len);
> + break;
> + }
> +
> + if (unlikely(vq->iov[0].iov_len != sizeof(v_req))) {
> + vq_err(vq, "Expecting virtio_scsi_cmd_req, got %zu"
> + " bytes\n", vq->iov[0].iov_len);
> + break;
> + }
> + pr_debug("Calling __copy_from_user: vq->iov[0].iov_base: %p,"
> + " len: %zu\n", vq->iov[0].iov_base, sizeof(v_req));
> + ret = __copy_from_user(&v_req, vq->iov[0].iov_base,
> + sizeof(v_req));
> + if (unlikely(ret)) {
> + vq_err(vq, "Faulted on virtio_scsi_cmd_req\n");
> + break;
> + }
> +
> + exp_data_len = 0;
> + for (i = 0; i < data_num; i++)
> + exp_data_len += vq->iov[data_first + i].iov_len;
> +
> + tv_cmd = vhost_scsi_allocate_cmd(tv_tpg, &v_req,
> + exp_data_len, data_direction);
> + if (IS_ERR(tv_cmd)) {
> + vq_err(vq, "vhost_scsi_allocate_cmd failed %ld\n",
> + PTR_ERR(tv_cmd));
> + break;
> + }
> + pr_debug("Allocated tv_cmd: %p exp_data_len: %d, data_direction"
> + ": %d\n", tv_cmd, exp_data_len, data_direction);
> +
> + tv_cmd->tvc_vhost = vs;
> +
> + if (unlikely(vq->iov[out].iov_len !=
> + sizeof(struct virtio_scsi_cmd_resp))) {
> + vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
> + " bytes, out: %d, in: %d\n",
> + vq->iov[out].iov_len, out, in);
> + break;
> + }
> +
> + tv_cmd->tvc_resp = vq->iov[out].iov_base;
> +
> + /*
> + * Copy in the recieved CDB descriptor into tv_cmd->tvc_cdb
> + * that will be used by tcm_vhost_new_cmd_map() and down into
> + * target_setup_cmd_from_cdb()
> + */
> + memcpy(tv_cmd->tvc_cdb, v_req.cdb, TCM_VHOST_MAX_CDB_SIZE);
> + /*
> + * Check that the recieved CDB size does not exceeded our
> + * hardcoded max for tcm_vhost
> + */
> + /* TODO what if cdb was too small for varlen cdb header? */
> + if (unlikely(scsi_command_size(tv_cmd->tvc_cdb) >
> + TCM_VHOST_MAX_CDB_SIZE)) {
> + vq_err(vq, "Received SCSI CDB with command_size: %d that"
> + " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
> + scsi_command_size(tv_cmd->tvc_cdb),
> + TCM_VHOST_MAX_CDB_SIZE);
> + break; /* TODO */
> + }
> + tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
> +
> + pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
> + tv_cmd->tvc_cdb[0], tv_cmd->tvc_lun);
> +
> + if (data_direction != DMA_NONE) {
> + ret = vhost_scsi_map_iov_to_sgl(tv_cmd,
> + &vq->iov[data_first], data_num,
> + data_direction == DMA_TO_DEVICE);
> + if (unlikely(ret)) {
> + vq_err(vq, "Failed to map iov to sgl\n");
> + break; /* TODO */
> + }
> + }
> +
> + /*
> + * Save the descriptor from vhost_get_vq_desc() to be used to
> + * complete the virtio-scsi request in TCM callback context via
> + * tcm_vhost_queue_data_in() and tcm_vhost_queue_status()
> + */
> + tv_cmd->tvc_vq_desc = head;
> + /*
> + * Dispatch tv_cmd descriptor for cmwq execution in process
> + * context provided by tcm_vhost_workqueue. This also ensures
> + * tv_cmd is executed on the same kworker CPU as this vhost
> + * thread to gain positive L2 cache locality effects..
> + */
> + INIT_WORK(&tv_cmd->work, tcm_vhost_submission_work);
> + queue_work(tcm_vhost_workqueue, &tv_cmd->work);
> + }
> +
> + mutex_unlock(&vq->mutex);
> +}
> +
> +static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
> +{
> + pr_err("%s: The handling func for control queue.\n", __func__);
> +}
> +
> +static void vhost_scsi_evt_handle_kick(struct vhost_work *work)
> +{
> + pr_err("%s: The handling func for event queue.\n", __func__);
> +}
> +
can userspace still trigger these? If not pls add a comment explaining
why are above never triggering.
> +static void vhost_scsi_handle_kick(struct vhost_work *work)
> +{
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_scsi *vs = container_of(vq->dev, struct vhost_scsi, dev);
> +
> + vhost_scsi_handle_vq(vs);
> +}
> +
> +/*
> + * Called from vhost_scsi_ioctl() context to walk the list of available
> + * tcm_vhost_tpg with an active struct tcm_vhost_nexus
> + */
> +static int vhost_scsi_set_endpoint(
> + struct vhost_scsi *vs,
> + struct vhost_scsi_target *t)
> +{
> + struct tcm_vhost_tport *tv_tport;
> + struct tcm_vhost_tpg *tv_tpg;
> + int index;
> +
> + mutex_lock(&vs->dev.mutex);
> + /* Verify that ring has been setup correctly. */
> + for (index = 0; index < vs->dev.nvqs; ++index) {
> + /* Verify that ring has been setup correctly. */
> + if (!vhost_vq_access_ok(&vs->vqs[index])) {
> + mutex_unlock(&vs->dev.mutex);
> + return -EFAULT;
> + }
> + }
> +
> + if (vs->vs_tpg) {
> + mutex_unlock(&vs->dev.mutex);
> + return -EEXIST;
> + }
> + mutex_unlock(&vs->dev.mutex);
vs->vs_tpg can change at this point and be != NULL in loop below. Not an issue?
> + mutex_lock(&tcm_vhost_mutex);
> + list_for_each_entry(tv_tpg, &tcm_vhost_list, tv_tpg_list) {
If you move this loop to a function, then it can use goto to cleanup on error.
> + mutex_lock(&tv_tpg->tv_tpg_mutex);
> + if (!tv_tpg->tpg_nexus) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + continue;
> + }
> + if (atomic_read(&tv_tpg->tv_tpg_vhost_count)) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + continue;
> + }
> + tv_tport = tv_tpg->tport;
> +
> + if (!strcmp(tv_tport->tport_name, t->vhost_wwpn) &&
> + (tv_tpg->tport_tpgt == t->vhost_tpgt)) {
> + atomic_inc(&tv_tpg->tv_tpg_vhost_count);
> + smp_mb__after_atomic_inc();
this an other uses of memory barriers need a comment: what are we
flushing out and why. Also - could something like kref be used for
reference counting? If this is not datapath this might be better.
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + mutex_unlock(&tcm_vhost_mutex);
> +
> + mutex_lock(&vs->dev.mutex);
> + vs->vs_tpg = tv_tpg;
> + atomic_inc(&vs->vhost_ref_cnt);
> + smp_mb__after_atomic_inc();
> + mutex_unlock(&vs->dev.mutex);
> + return 0;
> + }
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + }
> + mutex_unlock(&tcm_vhost_mutex);
> + return -EINVAL;
> +}
> +
> +static int vhost_scsi_clear_endpoint(
> + struct vhost_scsi *vs,
> + struct vhost_scsi_target *t)
> +{
> + struct tcm_vhost_tport *tv_tport;
> + struct tcm_vhost_tpg *tv_tpg;
> + int index;
> +
> + mutex_lock(&vs->dev.mutex);
> + /* Verify that ring has been setup correctly. */
> + for (index = 0; index < vs->dev.nvqs; ++index) {
> + if (!vhost_vq_access_ok(&vs->vqs[index])) {
> + mutex_unlock(&vs->dev.mutex);
> + return -EFAULT;
> + }
> + }
> +
> + if (!vs->vs_tpg) {
> + mutex_unlock(&vs->dev.mutex);
> + return -ENODEV;
> + }
> + tv_tpg = vs->vs_tpg;
> + tv_tport = tv_tpg->tport;
> +
> + if (strcmp(tv_tport->tport_name, t->vhost_wwpn) ||
> + (tv_tpg->tport_tpgt != t->vhost_tpgt)) {
> + mutex_unlock(&vs->dev.mutex);
> + pr_warn("tv_tport->tport_name: %s, tv_tpg->tport_tpgt: %hu"
> + " does not match t->vhost_wwpn: %s, t->vhost_tpgt: %hu\n",
> + tv_tport->tport_name, tv_tpg->tport_tpgt,
> + t->vhost_wwpn, t->vhost_tpgt);
> + return -EINVAL;
> + }
> + atomic_dec(&tv_tpg->tv_tpg_vhost_count);
> + vs->vs_tpg = NULL;
> + mutex_unlock(&vs->dev.mutex);
> +
> + return 0;
We could do it like this:
err:
mutex_unlock(&vs->dev.mutex);
return ret;
to make it clear we do not exit this with lock taken
> +}
> +
> +static int vhost_scsi_open(struct inode *inode, struct file *f)
> +{
> + struct vhost_scsi *s;
> + int r;
> +
> + s = kzalloc(sizeof(*s), GFP_KERNEL);
> + if (!s)
> + return -ENOMEM;
> +
> + vhost_work_init(&s->vs_completion_work, vhost_scsi_complete_cmd_work);
> + INIT_LIST_HEAD(&s->vs_completion_list);
> + spin_lock_init(&s->vs_completion_lock);
> +
> + s->vqs[0].handle_kick = vhost_scsi_ctl_handle_kick;
> + s->vqs[1].handle_kick = vhost_scsi_evt_handle_kick;
> + s->vqs[2].handle_kick = vhost_scsi_handle_kick;
better to add an enum with VQ numbers.
> + r = vhost_dev_init(&s->dev, s->vqs, 3);
> + if (r < 0) {
> + kfree(s);
> + return r;
> + }
> +
> + f->private_data = s;
> + return 0;
> +}
> +
> +static int vhost_scsi_release(struct inode *inode, struct file *f)
> +{
> + struct vhost_scsi *s = f->private_data;
> +
> + if (s->vs_tpg && s->vs_tpg->tport) {
> + struct vhost_scsi_target backend;
> +
> + memcpy(backend.vhost_wwpn, s->vs_tpg->tport->tport_name,
> + sizeof(backend.vhost_wwpn));
> + backend.vhost_tpgt = s->vs_tpg->tport_tpgt;
> + vhost_scsi_clear_endpoint(s, &backend);
> + }
> +
> + vhost_dev_cleanup(&s->dev, false);
> + kfree(s);
> + return 0;
> +}
> +
> +static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> +{
> + if (features & ~VHOST_FEATURES)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&vs->dev.mutex);
> + if ((features & (1 << VHOST_F_LOG_ALL)) &&
> + !vhost_log_access_ok(&vs->dev)) {
> + mutex_unlock(&vs->dev.mutex);
> + return -EFAULT;
> + }
> + vs->dev.acked_features = features;
> + /* TODO possibly smp_wmb() and flush vqs */
Either that or fail if already running.
Current userspace does not change features once set,
but it's cleaner not to make assumptions.
> + mutex_unlock(&vs->dev.mutex);
> + return 0;
> +}
> +
> +static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
> + unsigned long arg)
> +{
> + struct vhost_scsi *vs = f->private_data;
> + struct vhost_scsi_target backend;
> + void __user *argp = (void __user *)arg;
> + u64 __user *featurep = argp;
> + u64 features;
> + int r;
> +
> + switch (ioctl) {
> + case VHOST_SCSI_SET_ENDPOINT:
> + if (copy_from_user(&backend, argp, sizeof backend))
> + return -EFAULT;
> +
> + return vhost_scsi_set_endpoint(vs, &backend);
> + case VHOST_SCSI_CLEAR_ENDPOINT:
> + if (copy_from_user(&backend, argp, sizeof backend))
> + return -EFAULT;
> +
> + return vhost_scsi_clear_endpoint(vs, &backend);
> + case VHOST_SCSI_GET_ABI_VERSION:
> + if (copy_from_user(&backend, argp, sizeof backend))
> + return -EFAULT;
Why are we reading this in BTW?
> +
> + backend.abi_version = VHOST_SCSI_ABI_VERSION;
> +
> + if (copy_to_user(argp, &backend, sizeof backend))
> + return -EFAULT;
> + return 0;
> + case VHOST_GET_FEATURES:
> + features = VHOST_FEATURES;
> + if (copy_to_user(featurep, &features, sizeof features))
> + return -EFAULT;
> + return 0;
> + case VHOST_SET_FEATURES:
> + if (copy_from_user(&features, featurep, sizeof features))
> + return -EFAULT;
> + return vhost_scsi_set_features(vs, features);
> + default:
> + mutex_lock(&vs->dev.mutex);
> + r = vhost_dev_ioctl(&vs->dev, ioctl, arg);
> + mutex_unlock(&vs->dev.mutex);
> + return r;
> + }
> +}
> +
> +static const struct file_operations vhost_scsi_fops = {
> + .owner = THIS_MODULE,
> + .release = vhost_scsi_release,
> + .unlocked_ioctl = vhost_scsi_ioctl,
> + /* TODO compat ioctl? */
well?
> + .open = vhost_scsi_open,
> + .llseek = noop_llseek,
> +};
> +
> +static struct miscdevice vhost_scsi_misc = {
> + MISC_DYNAMIC_MINOR,
> + "vhost-scsi",
> + &vhost_scsi_fops,
> +};
> +
> +static int __init vhost_scsi_register(void)
> +{
> + return misc_register(&vhost_scsi_misc);
> +}
> +
> +static int vhost_scsi_deregister(void)
> +{
> + return misc_deregister(&vhost_scsi_misc);
> +}
> +
> +static char *tcm_vhost_dump_proto_id(struct tcm_vhost_tport *tport)
> +{
> + switch (tport->tport_proto_id) {
> + case SCSI_PROTOCOL_SAS:
> + return "SAS";
> + case SCSI_PROTOCOL_FCP:
> + return "FCP";
> + case SCSI_PROTOCOL_ISCSI:
> + return "iSCSI";
> + default:
> + break;
> + }
> +
> + return "Unknown";
> +}
> +
> +static int tcm_vhost_port_link(
> + struct se_portal_group *se_tpg,
> + struct se_lun *lun)
I'd like this and other places to be like this:
static int tcm_vhost_port_link(struct se_portal_group *se_tpg,
struct se_lun *lun)
or
static int
tcm_vhost_port_link(struct se_portal_group *se_tpg,
struct se_lun *lun)
so that it's more like the rest of vhost code.
> +{
> + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> +
> + atomic_inc(&tv_tpg->tv_tpg_port_count);
> + smp_mb__after_atomic_inc();
> +
> + return 0;
> +}
> +
> +static void tcm_vhost_port_unlink(
> + struct se_portal_group *se_tpg,
> + struct se_lun *se_lun)
> +{
> + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> +
> + atomic_dec(&tv_tpg->tv_tpg_port_count);
> + smp_mb__after_atomic_dec();
> +}
> +
> +static struct se_node_acl *tcm_vhost_make_nodeacl(
> + struct se_portal_group *se_tpg,
> + struct config_group *group,
> + const char *name)
> +{
> + struct se_node_acl *se_nacl, *se_nacl_new;
> + struct tcm_vhost_nacl *nacl;
> + u64 wwpn = 0;
> + u32 nexus_depth;
> +
> + /* tcm_vhost_parse_wwn(name, &wwpn, 1) < 0)
> + return ERR_PTR(-EINVAL); */
> + se_nacl_new = tcm_vhost_alloc_fabric_acl(se_tpg);
> + if (!se_nacl_new)
> + return ERR_PTR(-ENOMEM);
> +
> + nexus_depth = 1;
> + /*
> + * se_nacl_new may be released by core_tpg_add_initiator_node_acl()
> + * when converting a NodeACL from demo mode -> explict
> + */
> + se_nacl = core_tpg_add_initiator_node_acl(se_tpg, se_nacl_new,
> + name, nexus_depth);
> + if (IS_ERR(se_nacl)) {
> + tcm_vhost_release_fabric_acl(se_tpg, se_nacl_new);
> + return se_nacl;
> + }
> + /*
> + * Locate our struct tcm_vhost_nacl and set the FC Nport WWPN
> + */
> + nacl = container_of(se_nacl, struct tcm_vhost_nacl, se_node_acl);
> + nacl->iport_wwpn = wwpn;
> +
> + return se_nacl;
> +}
> +
> +static void tcm_vhost_drop_nodeacl(struct se_node_acl *se_acl)
> +{
> + struct tcm_vhost_nacl *nacl = container_of(se_acl,
> + struct tcm_vhost_nacl, se_node_acl);
> + core_tpg_del_initiator_node_acl(se_acl->se_tpg, se_acl, 1);
> + kfree(nacl);
> +}
> +
> +static int tcm_vhost_make_nexus(
> + struct tcm_vhost_tpg *tv_tpg,
> + const char *name)
> +{
> + struct se_portal_group *se_tpg;
> + struct tcm_vhost_nexus *tv_nexus;
> +
> + mutex_lock(&tv_tpg->tv_tpg_mutex);
> + if (tv_tpg->tpg_nexus) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + pr_debug("tv_tpg->tpg_nexus already exists\n");
> + return -EEXIST;
> + }
> + se_tpg = &tv_tpg->se_tpg;
> +
> + tv_nexus = kzalloc(sizeof(struct tcm_vhost_nexus), GFP_KERNEL);
> + if (!tv_nexus) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + pr_err("Unable to allocate struct tcm_vhost_nexus\n");
> + return -ENOMEM;
> + }
> + /*
> + * Initialize the struct se_session pointer
> + */
> + tv_nexus->tvn_se_sess = transport_init_session();
> + if (IS_ERR(tv_nexus->tvn_se_sess)) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + kfree(tv_nexus);
> + return -ENOMEM;
> + }
> + /*
> + * Since we are running in 'demo mode' this call with generate a
> + * struct se_node_acl for the tcm_vhost struct se_portal_group with
> + * the SCSI Initiator port name of the passed configfs group 'name'.
> + */
> + tv_nexus->tvn_se_sess->se_node_acl = core_tpg_check_initiator_node_acl(
> + se_tpg, (unsigned char *)name);
> + if (!tv_nexus->tvn_se_sess->se_node_acl) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + pr_debug("core_tpg_check_initiator_node_acl() failed"
> + " for %s\n", name);
> + transport_free_session(tv_nexus->tvn_se_sess);
> + kfree(tv_nexus);
> + return -ENOMEM;
> + }
> + /*
> + * Now register the TCM vHost virtual I_T Nexus as active with the
I think s/vHost/vhost/ would be nice :)
> + * call to __transport_register_session()
> + */
> + __transport_register_session(se_tpg, tv_nexus->tvn_se_sess->se_node_acl,
> + tv_nexus->tvn_se_sess, tv_nexus);
> + tv_tpg->tpg_nexus = tv_nexus;
> +
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + return 0;
> +}
> +
> +static int tcm_vhost_drop_nexus(
> + struct tcm_vhost_tpg *tpg)
> +{
> + struct se_session *se_sess;
> + struct tcm_vhost_nexus *tv_nexus;
> +
> + mutex_lock(&tpg->tv_tpg_mutex);
> + tv_nexus = tpg->tpg_nexus;
> + if (!tv_nexus) {
> + mutex_unlock(&tpg->tv_tpg_mutex);
> + return -ENODEV;
> + }
> +
> + se_sess = tv_nexus->tvn_se_sess;
> + if (!se_sess) {
> + mutex_unlock(&tpg->tv_tpg_mutex);
> + return -ENODEV;
> + }
> +
> + if (atomic_read(&tpg->tv_tpg_port_count)) {
> + mutex_unlock(&tpg->tv_tpg_mutex);
> + pr_err("Unable to remove TCM_vHost I_T Nexus with"
> + " active TPG port count: %d\n",
> + atomic_read(&tpg->tv_tpg_port_count));
> + return -EPERM;
EBUSY?
> + }
> +
> + if (atomic_read(&tpg->tv_tpg_vhost_count)) {
> + mutex_unlock(&tpg->tv_tpg_mutex);
> + pr_err("Unable to remove TCM_vHost I_T Nexus with"
> + " active TPG vhost count: %d\n",
> + atomic_read(&tpg->tv_tpg_vhost_count));
> + return -EPERM;
Same?
> + }
> +
> + pr_debug("TCM_vHost_ConfigFS: Removing I_T Nexus to emulated"
> + " %s Initiator Port: %s\n", tcm_vhost_dump_proto_id(tpg->tport),
> + tv_nexus->tvn_se_sess->se_node_acl->initiatorname);
> + /*
> + * Release the SCSI I_T Nexus to the emulated vHost Target Port
> + */
> + transport_deregister_session(tv_nexus->tvn_se_sess);
> + tpg->tpg_nexus = NULL;
> + mutex_unlock(&tpg->tv_tpg_mutex);
> +
> + kfree(tv_nexus);
> + return 0;
> +}
> +
> +static ssize_t tcm_vhost_tpg_show_nexus(
> + struct se_portal_group *se_tpg,
> + char *page)
> +{
> + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_nexus *tv_nexus;
> + ssize_t ret;
> +
> + mutex_lock(&tv_tpg->tv_tpg_mutex);
> + tv_nexus = tv_tpg->tpg_nexus;
> + if (!tv_nexus) {
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> + return -ENODEV;
> + }
> + ret = snprintf(page, PAGE_SIZE, "%s\n",
> + tv_nexus->tvn_se_sess->se_node_acl->initiatorname);
> + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> +
> + return ret;
> +}
> +
> +static ssize_t tcm_vhost_tpg_store_nexus(
> + struct se_portal_group *se_tpg,
> + const char *page,
> + size_t count)
> +{
> + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> + struct tcm_vhost_tport *tport_wwn = tv_tpg->tport;
> + unsigned char i_port[TCM_VHOST_NAMELEN], *ptr, *port_ptr;
> + int ret;
> + /*
> + * Shutdown the active I_T nexus if 'NULL' is passed..
> + */
> + if (!strncmp(page, "NULL", 4)) {
> + ret = tcm_vhost_drop_nexus(tv_tpg);
> + return (!ret) ? count : ret;
> + }
> + /*
> + * Otherwise make sure the passed virtual Initiator port WWN matches
> + * the fabric protocol_id set in tcm_vhost_make_tport(), and call
> + * tcm_vhost_make_nexus().
> + */
> + if (strlen(page) >= TCM_VHOST_NAMELEN) {
> + pr_err("Emulated NAA Sas Address: %s, exceeds"
> + " max: %d\n", page, TCM_VHOST_NAMELEN);
> + return -EINVAL;
> + }
> + snprintf(&i_port[0], TCM_VHOST_NAMELEN, "%s", page);
> +
> + ptr = strstr(i_port, "naa.");
> + if (ptr) {
> + if (tport_wwn->tport_proto_id != SCSI_PROTOCOL_SAS) {
> + pr_err("Passed SAS Initiator Port %s does not"
> + " match target port protoid: %s\n", i_port,
> + tcm_vhost_dump_proto_id(tport_wwn));
> + return -EINVAL;
> + }
> + port_ptr = &i_port[0];
> + goto check_newline;
> + }
> + ptr = strstr(i_port, "fc.");
> + if (ptr) {
> + if (tport_wwn->tport_proto_id != SCSI_PROTOCOL_FCP) {
> + pr_err("Passed FCP Initiator Port %s does not"
> + " match target port protoid: %s\n", i_port,
> + tcm_vhost_dump_proto_id(tport_wwn));
> + return -EINVAL;
> + }
> + port_ptr = &i_port[3]; /* Skip over "fc." */
> + goto check_newline;
> + }
> + ptr = strstr(i_port, "iqn.");
> + if (ptr) {
> + if (tport_wwn->tport_proto_id != SCSI_PROTOCOL_ISCSI) {
> + pr_err("Passed iSCSI Initiator Port %s does not"
> + " match target port protoid: %s\n", i_port,
> + tcm_vhost_dump_proto_id(tport_wwn));
> + return -EINVAL;
> + }
> + port_ptr = &i_port[0];
> + goto check_newline;
> + }
> + pr_err("Unable to locate prefix for emulated Initiator Port:"
> + " %s\n", i_port);
> + return -EINVAL;
> + /*
> + * Clear any trailing newline for the NAA WWN
> + */
> +check_newline:
> + if (i_port[strlen(i_port)-1] == '\n')
> + i_port[strlen(i_port)-1] = '\0';
> +
> + ret = tcm_vhost_make_nexus(tv_tpg, port_ptr);
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +
> +TF_TPG_BASE_ATTR(tcm_vhost, nexus, S_IRUGO | S_IWUSR);
> +
> +static struct configfs_attribute *tcm_vhost_tpg_attrs[] = {
> + &tcm_vhost_tpg_nexus.attr,
> + NULL,
> +};
> +
> +static struct se_portal_group *tcm_vhost_make_tpg(
> + struct se_wwn *wwn,
> + struct config_group *group,
> + const char *name)
> +{
> + struct tcm_vhost_tport *tport = container_of(wwn,
> + struct tcm_vhost_tport, tport_wwn);
> +
> + struct tcm_vhost_tpg *tpg;
> + unsigned long tpgt;
> + int ret;
> +
> + if (strstr(name, "tpgt_") != name)
> + return ERR_PTR(-EINVAL);
> + if (kstrtoul(name + 5, 10, &tpgt) || tpgt > UINT_MAX)
> + return ERR_PTR(-EINVAL);
> +
> + tpg = kzalloc(sizeof(struct tcm_vhost_tpg), GFP_KERNEL);
> + if (!tpg) {
> + pr_err("Unable to allocate struct tcm_vhost_tpg");
> + return ERR_PTR(-ENOMEM);
> + }
> + mutex_init(&tpg->tv_tpg_mutex);
> + INIT_LIST_HEAD(&tpg->tv_tpg_list);
> + tpg->tport = tport;
> + tpg->tport_tpgt = tpgt;
> +
> + ret = core_tpg_register(&tcm_vhost_fabric_configfs->tf_ops, wwn,
> + &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
> + if (ret < 0) {
> + kfree(tpg);
> + return NULL;
> + }
> + mutex_lock(&tcm_vhost_mutex);
> + list_add_tail(&tpg->tv_tpg_list, &tcm_vhost_list);
> + mutex_unlock(&tcm_vhost_mutex);
> +
> + return &tpg->se_tpg;
> +}
> +
> +static void tcm_vhost_drop_tpg(struct se_portal_group *se_tpg)
> +{
> + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> + struct tcm_vhost_tpg, se_tpg);
> +
> + mutex_lock(&tcm_vhost_mutex);
> + list_del(&tpg->tv_tpg_list);
> + mutex_unlock(&tcm_vhost_mutex);
> + /*
> + * Release the virtual I_T Nexus for this vHost TPG
> + */
> + tcm_vhost_drop_nexus(tpg);
> + /*
> + * Deregister the se_tpg from TCM..
> + */
> + core_tpg_deregister(se_tpg);
> + kfree(tpg);
> +}
> +
> +static struct se_wwn *tcm_vhost_make_tport(
> + struct target_fabric_configfs *tf,
> + struct config_group *group,
> + const char *name)
> +{
> + struct tcm_vhost_tport *tport;
> + char *ptr;
> + u64 wwpn = 0;
> + int off = 0;
> +
> + /* if (tcm_vhost_parse_wwn(name, &wwpn, 1) < 0)
> + return ERR_PTR(-EINVAL); */
> +
> + tport = kzalloc(sizeof(struct tcm_vhost_tport), GFP_KERNEL);
> + if (!tport) {
> + pr_err("Unable to allocate struct tcm_vhost_tport");
> + return ERR_PTR(-ENOMEM);
> + }
> + tport->tport_wwpn = wwpn;
> + /*
> + * Determine the emulated Protocol Identifier and Target Port Name
> + * based on the incoming configfs directory name.
> + */
> + ptr = strstr(name, "naa.");
> + if (ptr) {
> + tport->tport_proto_id = SCSI_PROTOCOL_SAS;
> + goto check_len;
> + }
> + ptr = strstr(name, "fc.");
> + if (ptr) {
> + tport->tport_proto_id = SCSI_PROTOCOL_FCP;
> + off = 3; /* Skip over "fc." */
> + goto check_len;
> + }
> + ptr = strstr(name, "iqn.");
> + if (ptr) {
> + tport->tport_proto_id = SCSI_PROTOCOL_ISCSI;
> + goto check_len;
> + }
> +
> + pr_err("Unable to locate prefix for emulated Target Port:"
> + " %s\n", name);
> + kfree(tport);
> + return ERR_PTR(-EINVAL);
> +
> +check_len:
> + if (strlen(name) >= TCM_VHOST_NAMELEN) {
> + pr_err("Emulated %s Address: %s, exceeds"
> + " max: %d\n", name, tcm_vhost_dump_proto_id(tport),
> + TCM_VHOST_NAMELEN);
> + kfree(tport);
> + return ERR_PTR(-EINVAL);
> + }
> + snprintf(&tport->tport_name[0], TCM_VHOST_NAMELEN, "%s", &name[off]);
> +
> + pr_debug("TCM_VHost_ConfigFS: Allocated emulated Target"
> + " %s Address: %s\n", tcm_vhost_dump_proto_id(tport), name);
> +
> + return &tport->tport_wwn;
> +}
> +
> +static void tcm_vhost_drop_tport(struct se_wwn *wwn)
> +{
> + struct tcm_vhost_tport *tport = container_of(wwn,
> + struct tcm_vhost_tport, tport_wwn);
> +
> + pr_debug("TCM_VHost_ConfigFS: Deallocating emulated Target"
> + " %s Address: %s\n", tcm_vhost_dump_proto_id(tport),
> + tport->tport_name);
> +
> + kfree(tport);
> +}
> +
> +static ssize_t tcm_vhost_wwn_show_attr_version(
> + struct target_fabric_configfs *tf,
> + char *page)
> +{
> + return sprintf(page, "TCM_VHOST fabric module %s on %s/%s"
> + "on "UTS_RELEASE"\n", TCM_VHOST_VERSION, utsname()->sysname,
> + utsname()->machine);
> +}
> +
> +TF_WWN_ATTR_RO(tcm_vhost, version);
> +
> +static struct configfs_attribute *tcm_vhost_wwn_attrs[] = {
> + &tcm_vhost_wwn_version.attr,
> + NULL,
> +};
> +
> +static struct target_core_fabric_ops tcm_vhost_ops = {
> + .get_fabric_name = tcm_vhost_get_fabric_name,
> + .get_fabric_proto_ident = tcm_vhost_get_fabric_proto_ident,
> + .tpg_get_wwn = tcm_vhost_get_fabric_wwn,
> + .tpg_get_tag = tcm_vhost_get_tag,
> + .tpg_get_default_depth = tcm_vhost_get_default_depth,
> + .tpg_get_pr_transport_id = tcm_vhost_get_pr_transport_id,
> + .tpg_get_pr_transport_id_len = tcm_vhost_get_pr_transport_id_len,
> + .tpg_parse_pr_out_transport_id = tcm_vhost_parse_pr_out_transport_id,
> + .tpg_check_demo_mode = tcm_vhost_check_true,
> + .tpg_check_demo_mode_cache = tcm_vhost_check_true,
> + .tpg_check_demo_mode_write_protect = tcm_vhost_check_false,
> + .tpg_check_prod_mode_write_protect = tcm_vhost_check_false,
> + .tpg_alloc_fabric_acl = tcm_vhost_alloc_fabric_acl,
> + .tpg_release_fabric_acl = tcm_vhost_release_fabric_acl,
> + .tpg_get_inst_index = tcm_vhost_tpg_get_inst_index,
> + .release_cmd = tcm_vhost_release_cmd,
> + .shutdown_session = tcm_vhost_shutdown_session,
> + .close_session = tcm_vhost_close_session,
> + .sess_get_index = tcm_vhost_sess_get_index,
> + .sess_get_initiator_sid = NULL,
> + .write_pending = tcm_vhost_write_pending,
> + .write_pending_status = tcm_vhost_write_pending_status,
> + .set_default_node_attributes = tcm_vhost_set_default_node_attrs,
> + .get_task_tag = tcm_vhost_get_task_tag,
> + .get_cmd_state = tcm_vhost_get_cmd_state,
> + .queue_data_in = tcm_vhost_queue_data_in,
> + .queue_status = tcm_vhost_queue_status,
> + .queue_tm_rsp = tcm_vhost_queue_tm_rsp,
> + .get_fabric_sense_len = tcm_vhost_get_fabric_sense_len,
> + .set_fabric_sense_len = tcm_vhost_set_fabric_sense_len,
> + /*
> + * Setup callers for generic logic in target_core_fabric_configfs.c
> + */
> + .fabric_make_wwn = tcm_vhost_make_tport,
> + .fabric_drop_wwn = tcm_vhost_drop_tport,
> + .fabric_make_tpg = tcm_vhost_make_tpg,
> + .fabric_drop_tpg = tcm_vhost_drop_tpg,
> + .fabric_post_link = tcm_vhost_port_link,
> + .fabric_pre_unlink = tcm_vhost_port_unlink,
> + .fabric_make_np = NULL,
> + .fabric_drop_np = NULL,
> + .fabric_make_nodeacl = tcm_vhost_make_nodeacl,
> + .fabric_drop_nodeacl = tcm_vhost_drop_nodeacl,
> +};
> +
> +static int tcm_vhost_register_configfs(void)
> +{
> + struct target_fabric_configfs *fabric;
> + int ret;
> +
> + pr_debug("TCM_VHOST fabric module %s on %s/%s"
> + " on "UTS_RELEASE"\n", TCM_VHOST_VERSION, utsname()->sysname,
> + utsname()->machine);
> + /*
> + * Register the top level struct config_item_type with TCM core
> + */
> + fabric = target_fabric_configfs_init(THIS_MODULE, "vhost");
> + if (IS_ERR(fabric)) {
> + pr_err("target_fabric_configfs_init() failed\n");
> + return PTR_ERR(fabric);
> + }
> + /*
> + * Setup fabric->tf_ops from our local tcm_vhost_ops
> + */
> + fabric->tf_ops = tcm_vhost_ops;
> + /*
> + * Setup default attribute lists for various fabric->tf_cit_tmpl
> + */
> + TF_CIT_TMPL(fabric)->tfc_wwn_cit.ct_attrs = tcm_vhost_wwn_attrs;
> + TF_CIT_TMPL(fabric)->tfc_tpg_base_cit.ct_attrs = tcm_vhost_tpg_attrs;
> + TF_CIT_TMPL(fabric)->tfc_tpg_attrib_cit.ct_attrs = NULL;
> + TF_CIT_TMPL(fabric)->tfc_tpg_param_cit.ct_attrs = NULL;
> + TF_CIT_TMPL(fabric)->tfc_tpg_np_base_cit.ct_attrs = NULL;
> + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_base_cit.ct_attrs = NULL;
> + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_attrib_cit.ct_attrs = NULL;
> + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_auth_cit.ct_attrs = NULL;
> + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_param_cit.ct_attrs = NULL;
> + /*
> + * Register the fabric for use within TCM
> + */
> + ret = target_fabric_configfs_register(fabric);
> + if (ret < 0) {
> + pr_err("target_fabric_configfs_register() failed"
> + " for TCM_VHOST\n");
> + return ret;
> + }
> + /*
> + * Setup our local pointer to *fabric
> + */
> + tcm_vhost_fabric_configfs = fabric;
> + pr_debug("TCM_VHOST[0] - Set fabric -> tcm_vhost_fabric_configfs\n");
> + return 0;
> +};
> +
> +static void tcm_vhost_deregister_configfs(void)
> +{
> + if (!tcm_vhost_fabric_configfs)
> + return;
> +
> + target_fabric_configfs_deregister(tcm_vhost_fabric_configfs);
> + tcm_vhost_fabric_configfs = NULL;
> + pr_debug("TCM_VHOST[0] - Cleared tcm_vhost_fabric_configfs\n");
> +};
> +
> +static int __init tcm_vhost_init(void)
> +{
> + int ret = -ENOMEM;
> +
> + tcm_vhost_workqueue = alloc_workqueue("tcm_vhost", 0, 0);
> + if (!tcm_vhost_workqueue)
> + goto out;
Pls add a comment explaining why we need our own workqueue
and aren't using the system one instead.
> +
> + ret = vhost_scsi_register();
> + if (ret < 0)
> + goto out_destroy_workqueue;
> +
> + ret = tcm_vhost_register_configfs();
> + if (ret < 0)
> + goto out_vhost_scsi_deregister;
> +
> + return 0;
> +
> +out_vhost_scsi_deregister:
> + vhost_scsi_deregister();
> +out_destroy_workqueue:
> + destroy_workqueue(tcm_vhost_workqueue);
> +out:
> + return ret;
> +};
> +
> +static void tcm_vhost_exit(void)
> +{
> + tcm_vhost_deregister_configfs();
> + vhost_scsi_deregister();
> + destroy_workqueue(tcm_vhost_workqueue);
> +};
> +
> +MODULE_DESCRIPTION("TCM_VHOST series fabric driver");
> +MODULE_LICENSE("GPL");
> +module_init(tcm_vhost_init);
> +module_exit(tcm_vhost_exit);
> diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> new file mode 100644
> index 0000000..c983ed2
> --- /dev/null
> +++ b/drivers/vhost/tcm_vhost.h
> @@ -0,0 +1,101 @@
> +#define TCM_VHOST_VERSION "v0.1"
> +#define TCM_VHOST_NAMELEN 256
> +#define TCM_VHOST_MAX_CDB_SIZE 32
> +
> +struct tcm_vhost_cmd {
> + /* Descriptor from vhost_get_vq_desc() for virt_queue segment */
> + int tvc_vq_desc;
> + /* The Tag from include/linux/virtio_scsi.h:struct virtio_scsi_cmd_req */
> + u64 tvc_tag;
> + /* The number of scatterlists associated with this cmd */
> + u32 tvc_sgl_count;
> + /* Saved unpacked SCSI LUN for tcm_vhost_submission_work() */
> + u32 tvc_lun;
> + /* Pointer to the SGL formatted memory from virtio-scsi */
> + struct scatterlist *tvc_sgl;
> + /* Pointer to response */
> + struct virtio_scsi_cmd_resp __user *tvc_resp;
> + /* Pointer to vhost_scsi for our device */
> + struct vhost_scsi *tvc_vhost;
> + /* The TCM I/O descriptor that is accessed via container_of() */
> + struct se_cmd tvc_se_cmd;
> + /* work item used for cmwq dispatch to tcm_vhost_submission_work() */
> + struct work_struct work;
> + /* Copy of the incoming SCSI command descriptor block (CDB) */
> + unsigned char tvc_cdb[TCM_VHOST_MAX_CDB_SIZE];
> + /* Sense buffer that will be mapped into outgoing status */
> + unsigned char tvc_sense_buf[TRANSPORT_SENSE_BUFFER];
> + /* Completed commands list, serviced from vhost worker thread */
> + struct list_head tvc_completion_list;
> +};
> +
> +struct tcm_vhost_nexus {
> + /* Pointer to TCM session for I_T Nexus */
> + struct se_session *tvn_se_sess;
> +};
> +
> +struct tcm_vhost_nacl {
> + /* Binary World Wide unique Port Name for Vhost Initiator port */
> + u64 iport_wwpn;
> + /* ASCII formatted WWPN for Sas Initiator port */
> + char iport_name[TCM_VHOST_NAMELEN];
> + /* Returned by tcm_vhost_make_nodeacl() */
> + struct se_node_acl se_node_acl;
> +};
> +
> +struct tcm_vhost_tpg {
> + /* Vhost port target portal group tag for TCM */
> + u16 tport_tpgt;
> + /* Used to track number of TPG Port/Lun Links wrt to explict I_T Nexus shutdown */
> + atomic_t tv_tpg_port_count;
> + /* Used for vhost_scsi device reference to tpg_nexus */
> + atomic_t tv_tpg_vhost_count;
> + /* list for tcm_vhost_list */
> + struct list_head tv_tpg_list;
> + /* Used to protect access for tpg_nexus */
> + struct mutex tv_tpg_mutex;
would it be easier to just use dev mutex instead?
> + /* Pointer to the TCM VHost I_T Nexus for this TPG endpoint */
> + struct tcm_vhost_nexus *tpg_nexus;
> + /* Pointer back to tcm_vhost_tport */
> + struct tcm_vhost_tport *tport;
> + /* Returned by tcm_vhost_make_tpg() */
> + struct se_portal_group se_tpg;
> +};
> +
> +struct tcm_vhost_tport {
> + /* SCSI protocol the tport is providing */
> + u8 tport_proto_id;
> + /* Binary World Wide unique Port Name for Vhost Target port */
> + u64 tport_wwpn;
> + /* ASCII formatted WWPN for Vhost Target port */
> + char tport_name[TCM_VHOST_NAMELEN];
> + /* Returned by tcm_vhost_make_tport() */
> + struct se_wwn tport_wwn;
> +};
> +
> +/*
> + * As per request from MST, keep TCM_VHOST related ioctl defines out of
> + * linux/vhost.h (user-space) for now..
> + */
> +
> +#include <linux/vhost.h>
> +
> +/*
> + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> + *
> + * ABI Rev 0: July 2012 version starting point for v3.6-rc merge candidate +
> + * RFC-v2 vhost-scsi userspace. Add GET_ABI_VERSION ioctl usage
> + */
> +
> +#define VHOST_SCSI_ABI_VERSION 0
> +
> +struct vhost_scsi_target {
> + int abi_version;
> + unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> + unsigned short vhost_tpgt;
> +};
> +
> +/* VHOST_SCSI specific defines */
> +#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> +#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
CLEAR_ENDPOINT and SET_ENDPOINT commands get an abi version
too but it's ignored there. Intentional?
Why not
#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, int)
and then you avoid changing vhost_scsi_target ...
Also, let's add a comment like
/* Changing this breaks userspace. */
so we remember never to change this value.
> --
> 1.7.2.5
^ permalink raw reply
* Re: [net-next RFC V5 3/5] virtio: intorduce an API to set affinity for a virtqueue
From: Michael S. Tsirkin @ 2012-07-29 20:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: krkumar2, habanero, kvm, netdev, mashirle, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <5012A7D3.4040800@redhat.com>
On Fri, Jul 27, 2012 at 04:38:11PM +0200, Paolo Bonzini wrote:
> Il 05/07/2012 12:29, Jason Wang ha scritto:
> > Sometimes, virtio device need to configure irq affiniry hint to maximize the
> > performance. Instead of just exposing the irq of a virtqueue, this patch
> > introduce an API to set the affinity for a virtqueue.
> >
> > The api is best-effort, the affinity hint may not be set as expected due to
> > platform support, irq sharing or irq type. Currently, only pci method were
> > implemented and we set the affinity according to:
> >
> > - if device uses INTX, we just ignore the request
> > - if device has per vq vector, we force the affinity hint
> > - if the virtqueues share MSI, make the affinity OR over all affinities
> > requested
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Hmm, I don't see any benefit from this patch, I need to use
> irq_set_affinity (which however is not exported) to actually bind IRQs
> to CPUs. Example:
>
> with irq_set_affinity_hint:
> 43: 89 107 100 97 PCI-MSI-edge virtio0-request
> 44: 178 195 268 199 PCI-MSI-edge virtio0-request
> 45: 97 100 97 155 PCI-MSI-edge virtio0-request
> 46: 234 261 213 218 PCI-MSI-edge virtio0-request
>
> with irq_set_affinity:
> 43: 721 0 0 1 PCI-MSI-edge virtio0-request
> 44: 0 746 0 1 PCI-MSI-edge virtio0-request
> 45: 0 0 658 0 PCI-MSI-edge virtio0-request
> 46: 0 0 1 547 PCI-MSI-edge virtio0-request
>
> I gathered these quickly after boot, but real benchmarks show the same
> behavior, and performance gets actually worse with virtio-scsi
> multiqueue+irq_set_affinity_hint than with irq_set_affinity.
>
> I also tried adding IRQ_NO_BALANCING, but the only effect is that I
> cannot set the affinity
>
> The queue steering algorithm I use in virtio-scsi is extremely simple
> and based on your tx code. See how my nice pinning is destroyed:
>
> # taskset -c 0 dd if=/dev/sda bs=1M count=1000 of=/dev/null iflag=direct
> # cat /proc/interrupts
> 43: 2690 2709 2691 2696 PCI-MSI-edge virtio0-request
> 44: 109 122 199 124 PCI-MSI-edge virtio0-request
> 45: 170 183 170 237 PCI-MSI-edge virtio0-request
> 46: 143 166 125 125 PCI-MSI-edge virtio0-request
>
> All my requests come from CPU#0 and thus go to the first virtqueue, but
> the interrupts are serviced all over the place.
>
> Did you set the affinity manually in your experiments, or perhaps there
> is a difference between scsi and networking... (interrupt mitigation?)
>
> Paolo
You need to run irqbalancer in guest to make it actually work. Do you?
^ permalink raw reply
* RE: [PATCH 00/17] drivers: hv: kvp
From: KY Srinivasan @ 2012-07-29 22:50 UTC (permalink / raw)
To: Greg KH
Cc: olaf@aepfle.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
apw@canonical.com, devel@linuxdriverproject.org,
ben@decadent.org.uk
In-Reply-To: <20120724155412.GA23918@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, July 24, 2012 11:54 AM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; olaf@aepfle.de; apw@canonical.com;
> netdev@vger.kernel.org; ben@decadent.org.uk
> Subject: Re: [PATCH 00/17] drivers: hv: kvp
>
> On Tue, Jul 24, 2012 at 09:01:12AM -0700, K. Y. Srinivasan wrote:
> > This patchset expands the KVP (Key Value Pair) functionality to
> > implement the mechanism to GET/SET IP addresses in the guest. This
> > functionality is used in Windows Server 2012 to implement VM
> > replication functionality. The way IP configuration information
> > is managed is distro specific. Based on the feedback I have gotten
> > from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
> > distro specific code from this patch-set. Most of the GET operation
> > can be implemented in a way that is completely distro independent and
> > I have implemented that as such and is included in this patch-set.
> > Some of the attributes that can only be fetched in a distro
> > dependent way as well the mechanism for configuring an interface
> > (the SET operation) that is clearly distro specific is to be
> > implemented via external scripts that will be invoked via the KVP
> > code. We define here the interface to these scripts.
> >
> > Adding support for IP injection resulted in some changes to the
> > protocol between the user level daemon and the kernel driver.
> > These changes have been implemented in way that would retain
> > compatibility with older daemons. I would like to thank Olaf and
> > Greg for pointing out the compatibility issue.
>
> Due to this being the middle of the merge window, I will not be able to
> look at this until after 3.6-rc1 is out.
Thanks Greg. In the meantime, I have addressed all the comments that both Olaf
and Ben have posted on this patch-set. Since addressing these comments changed
some data structures, I think it will be best if you dropped this patch-set. I will post the
updated patch-set shortly.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Asias He @ 2012-07-30 0:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-kernel, virtualization, Paolo Bonzini,
Christoph Hellwig
In-Reply-To: <20120729125948.GA10041@redhat.com>
On 07/29/2012 08:59 PM, Michael S. Tsirkin wrote:
> On Sat, Jul 28, 2012 at 10:38:41AM +0800, Asias He wrote:
>> On 07/27/2012 08:33 AM, Rusty Russell wrote:
>>> On Fri, 13 Jul 2012 16:38:51 +0800, Asias He <asias@redhat.com> wrote:
>>>> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
>>>> use_bio=1' to enable ->make_request_fn() based I/O path.
>>>
>>> This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
>>> host cache between writeback and writethrough' which is also queued (see
>>> linux-next).
>>
>> Rebased against Paolo's patch in V4.
>>
>>> I'm not sure what the correct behavior for bio & cacheflush is, if any.
>>
>> REQ_FLUSH is not supported in the bio path.
>>
>>> But as to the patch itself: it's a hack.
>>>
>>> 1) Leaving the guest's admin to turn on the switch is a terrible choice.
>>> 2) The block layer should stop merging and sorting when a device is
>>> fast, not the driver.
>>> 3) I pointed out that slow disks have low IOPS, so why is this
>>> conditional? Sure, more guest exits, but it's still a small number
>>> for a slow device.
>>> 4) The only case where we want merging is on a slow device when the host
>>> isn't doing it.
>>>
>>> Now, despite this, I'm prepared to commit it. But in my mind it's a
>>> hack: we should aim for use_bio to be based on a feature bit fed from
>>> the host, and use the module parameter only if we want to override it.
>>
>> OK. A feature bit from host sound like a choice but a switch is also
>> needed on host side.
>
> qemu automatically gives you the ability to control
> any feature bit.
Automatically?
>> And for other OS, e.g. Windows, the bio thing
>> does not apply at all.
>
> Let's try to define when it's a good idea. Is it a hint to guest that
> backend handles small accesses efficiently so ok to disable batching?
Yes. It's also a hint for latency reduction.
>> Anyway, I have to admit that adding a module parameter here is not
>> the best choice. Let's think more.
>>
>> --
>> Asias
--
Asias
^ permalink raw reply
* Re: [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Rusty Russell @ 2012-07-30 1:55 UTC (permalink / raw)
To: Michael S. Tsirkin, Asias He
Cc: Christoph Hellwig, linux-kernel, kvm, virtualization
In-Reply-To: <20120729111115.GD8977@redhat.com>
On Sun, 29 Jul 2012 14:11:15 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, Jul 28, 2012 at 10:21:05AM +0800, Asias He wrote:
> > This patch introduces bio-based IO path for virtio-blk.
> >
> > Compared to request-based IO path, bio-based IO path uses driver
> > provided ->make_request_fn() method to bypasses the IO scheduler. It
> > handles the bio to device directly without allocating a request in block
> > layer. This reduces the IO path in guest kernel to achieve high IOPS
> > and lower latency. The downside is that guest can not use the IO
> > scheduler to merge and sort requests. However, this is not a big problem
> > if the backend disk in host side uses faster disk device.
>
> If this optimization depends on the host, then it
> should be reported to the guest using a feature bit,
> as opposed to being guest driven.
I consider this approach a half-way step. Quick attempts on my laptop
and I couldn't find a case where the bio path was a loss, but in theory
if the host wasn't doing any reordering and it was a slow device, you'd
want the guest to do so.
I'm not sure if current qemu can be configured to do such a thing?
Cheers,
Rusty.
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Jason Wang @ 2012-07-30 3:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, mashirle, kvm, netdev, sri, linux-kernel,
virtualization, edumazet, Sasha Levin, jwhan, davem, tahm
In-Reply-To: <20120729094451.GB8977@redhat.com>
On 07/29/2012 05:44 PM, Michael S. Tsirkin wrote:
> On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote:
>> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
>>>> - err = init_vqs(vi);
>>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>>>>> + vi->has_cvq = true;
>>>>> +
>>> How about we disable multiqueue if there's no cvq?
>>> Will make logic a bit simpler, won't it?
>> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
> Well !cvq support is a legacy feature: the reason we support it
> in driver is to avoid breaking on old hosts. Adding more code to that
> path just doesn't make much sense since old hosts won't have mq.
>
After some thought about this, maybe there's no need to the cvq for the
negotiation if we want support only two modes ( 1 tx/rx queue pair and N
tx/rx queue pairs). We can do this just through the feature bit negotiation.
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Jason Wang @ 2012-07-30 3:29 UTC (permalink / raw)
To: Sasha Levin
Cc: krkumar2, habanero, mashirle, kvm, Michael S. Tsirkin, netdev,
linux-kernel, virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <500D1926.5020407@gmail.com>
On 07/23/2012 05:28 PM, Sasha Levin wrote:
> On 07/23/2012 07:54 AM, Jason Wang wrote:
>> On 07/21/2012 08:02 PM, Sasha Levin wrote:
>>> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
>>>>> - err = init_vqs(vi);
>>>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>>>>>> + vi->has_cvq = true;
>>>>>> +
>>>> How about we disable multiqueue if there's no cvq?
>>>> Will make logic a bit simpler, won't it?
>>> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
>>>
>> Yes, it does not depends on cvq. Cvq were just used to negotiate the number of queues a guest wishes to use which is really useful (at least for now). Since multiqueue can not out-perform for single queue in every kinds of workloads or benchmark, so we want to let guest driver use single queue by default even when multiqueue were enabled by management software and let use to enalbe it through ethtool. So user could not feel regression when it switch to use a multiqueue capable driver and backend.
> Why would you limit it to a single vq if the user has specified a different number of vqs (>1) in the virtio-net device config?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
The only reason is to prevent the user from seeing the regression. The
performance of small packet sending is wrose than single queue, it tends
to send more but small packets when multiqueue is enabled. If we make
multiqueue bahave as good as single queue, we can remove this limit.
^ permalink raw reply
* Re: [net-next RFC V5 2/5] virtio_ring: move queue_index to vring_virtqueue
From: Jason Wang @ 2012-07-30 3:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: krkumar2, habanero, kvm, mst, netdev, mashirle, linux-kernel,
virtualization, edumazet, Sasha Levin, jwhan, sri, davem, tahm
In-Reply-To: <5010FDB2.5040505@redhat.com>
On 07/26/2012 04:20 PM, Paolo Bonzini wrote:
> Il 05/07/2012 13:40, Sasha Levin ha scritto:
>> @@ -275,7 +274,7 @@ static void vm_del_vq(struct virtqueue *vq)
>> vring_del_virtqueue(vq);
>>
>> /* Select and deactivate the queue */
>> - writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
>> + writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
>> writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
>>
> This accesses vq after vring_del_virtqueue has freed it.
>
> Paolo
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes, so need a temporary variable before vring_del_virtqueue().
Thanks.
^ permalink raw reply
* [PULL] virtio
From: Rusty Russell @ 2012-07-30 4:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: kvm, Michael S. Tsirkin, virtualization, Amit Shah, Paolo Bonzini
The following changes since commit f7da9cdf45cbbad5029d4858dcbc0134e06084ed:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2012-07-28 06:00:39 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git tags/virtio-for-linus
for you to fetch changes up to 6a743897144500fb4c4566ced3a498d5180fbb5b:
virtio-blk: return VIRTIO_BLK_F_FLUSH to header. (2012-07-30 13:30:52 +0930)
----------------------------------------------------------------
Virtio patches, mainly hotplugging fixes.
----------------------------------------------------------------
Amit Shah (5):
virtio ids: fix comment for virtio-rng
virtio: rng: allow tasks to be killed that are waiting for rng input
virtio: rng: don't wait on host when module is going away
virtio: rng: split out common code in probe / remove for s3/s4 ops
virtio: rng: s3/s4 support
Asias He (3):
virtio-blk: Call del_gendisk() before disable guest kick
virtio-blk: Reset device after blk_cleanup_queue()
virtio-blk: Use block layer provided spinlock
Paolo Bonzini (1):
virtio-blk: allow toggling host cache between writeback and writethrough
Rusty Russell (1):
virtio-blk: return VIRTIO_BLK_F_FLUSH to header.
drivers/block/virtio_blk.c | 115 ++++++++++++++++++++++++++++-------
drivers/char/hw_random/virtio-rng.c | 37 ++++++++++-
include/linux/virtio_blk.h | 10 ++-
include/linux/virtio_ids.h | 2 +-
4 files changed, 137 insertions(+), 27 deletions(-)
^ permalink raw reply
* Re: [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Asias He @ 2012-07-30 4:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, linux-kernel, virtualization, Christoph Hellwig
In-Reply-To: <20120729111115.GD8977@redhat.com>
On 07/29/2012 07:11 PM, Michael S. Tsirkin wrote:
> On Sat, Jul 28, 2012 at 10:21:05AM +0800, Asias He wrote:
>> This patch introduces bio-based IO path for virtio-blk.
>>
>> Compared to request-based IO path, bio-based IO path uses driver
>> provided ->make_request_fn() method to bypasses the IO scheduler. It
>> handles the bio to device directly without allocating a request in block
>> layer. This reduces the IO path in guest kernel to achieve high IOPS
>> and lower latency. The downside is that guest can not use the IO
>> scheduler to merge and sort requests. However, this is not a big problem
>> if the backend disk in host side uses faster disk device.
>
> If this optimization depends on the host, then it
> should be reported to the guest using a feature bit,
> as opposed to being guest driven.
>
>> When the bio-based IO path is not enabled, virtio-blk still uses the
>> original request-based IO path, no performance difference is observed.
>>
>> Performance evaluation:
>> -----------------------------
>> 1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
>> kvm tool.
>>
>> Short version:
>> With bio-based IO path, sequential read/write, random read/write
>> IOPS boost : 28%, 24%, 21%, 16%
>> Latency improvement: 32%, 17%, 21%, 16%
>>
>> Long version:
>> With bio-based IO path:
>> seq-read : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
>> seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
>> rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
>> rand-write: io=3095.7MB, bw=96198KB/s, iops=192396 , runt= 32952msec
>> clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
>> clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
>> clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
>> clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
>> cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
>> cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
>> cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
>> cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
>> With request-based IO path:
>> seq-read : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
>> seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
>> rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
>> rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
>> clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
>> clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
>> clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
>> clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
>> cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
>> cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
>> cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
>> cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985
>
>
> So bio based causes cpu to jump up by some 30%?
> What happens if you divide IOPS/CPU?
> Any improvement that comes from increasing the cpu share
> of the given guest on the host will not scale well on
> a typical overcommitted host.
if you add sys and usr time up, the jump is that much.
For ramdisk based device,
bio-based
------------------
>>> 74.08 + 703.84
777.9200000000001
>>> 70.92 + 702.81
773.7299999999999
>>> 72.23 + 695.37
767.6
>>> 69.69 + 654.13
723.8199999999999
>>> 53.28 + 724.19
777.47
req-based
------------------
>>> 53.28 + 724.19
777.47
>>> 49.03 + 633.20
682.23
>>> 55.78 + 722.40
778.18
>>> 56.55 + 690.83
747.38
And for real device(fusion io), the cpu time is smaller with bio path.
bio-based
------------------
>>> 56.79 + 421.70
478.49
>>> 61.81 + 455.53
517.3399999999999
>>> 63.10+455.38
518.48
>>> 62.04 + 453.58
515.62
req-based
------------------
>>> 44.08 + 590.71
634.7900000000001
>>> 48.73 + 610.78
659.51
>>> 45.58 + 581.16
626.74
>>> 48.40 + 599.65
648.05
>
>> 2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
>> kvm tool.
>>
>> Short version:
>> With bio-based IO path, sequential read/write, random read/write
>> IOPS boost : 11%, 11%, 13%, 10%
>> Latency improvement: 10%, 10%, 12%, 10%
>> Long Version:
>> With bio-based IO path:
>> read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
>> write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
>> read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
>> write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
>> clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
>> clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
>> clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
>> clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
>> cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
>> cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
>> cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
>> cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
>> With request-based IO path:
>> read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
>> write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
>> read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
>> write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
>> clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
>> clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
>> clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
>> clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
>> cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
>> cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
>> cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
>> cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409
>
>
> Is this host or guest cpu? We should probably measure host cpu
> as this includes device overhead which could vary by load.
It's guest cpu. Yes, host cpu is also interesting.
>
>> How to use:
>> -----------------------------
>> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
>> use_bio=1' to enable ->make_request_fn() based I/O path.
>>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
>> Signed-off-by: Asias He <asias@redhat.com>
>> ---
>> drivers/block/virtio_blk.c | 203 +++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 163 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index c0bbeb4..95cfeed 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -14,6 +14,9 @@
>>
>> #define PART_BITS 4
>>
>> +static bool use_bio;
>> +module_param(use_bio, bool, S_IRUGO);
>> +
>> static int major;
>> static DEFINE_IDA(vd_index_ida);
>>
>> @@ -23,6 +26,7 @@ struct virtio_blk
>> {
>> struct virtio_device *vdev;
>> struct virtqueue *vq;
>> + wait_queue_head_t queue_wait;
>>
>> /* The disk structure for the kernel. */
>> struct gendisk *disk;
>> @@ -51,53 +55,87 @@ struct virtio_blk
>> struct virtblk_req
>> {
>> struct request *req;
>> + struct bio *bio;
>> struct virtio_blk_outhdr out_hdr;
>> struct virtio_scsi_inhdr in_hdr;
>> u8 status;
>> + struct scatterlist sg[];
>> };
>>
>> -static void blk_done(struct virtqueue *vq)
>> +static inline int virtblk_result(struct virtblk_req *vbr)
>> +{
>> + switch (vbr->status) {
>> + case VIRTIO_BLK_S_OK:
>> + return 0;
>> + case VIRTIO_BLK_S_UNSUPP:
>> + return -ENOTTY;
>> + default:
>> + return -EIO;
>> + }
>> +}
>> +
>> +static inline void virtblk_request_done(struct virtio_blk *vblk,
>> + struct virtblk_req *vbr)
>> +{
>> + struct request *req = vbr->req;
>> + int error = virtblk_result(vbr);
>> +
>> + if (req->cmd_type == REQ_TYPE_BLOCK_PC) {
>> + req->resid_len = vbr->in_hdr.residual;
>> + req->sense_len = vbr->in_hdr.sense_len;
>> + req->errors = vbr->in_hdr.errors;
>> + } else if (req->cmd_type == REQ_TYPE_SPECIAL) {
>> + req->errors = (error != 0);
>> + }
>> +
>> + __blk_end_request_all(req, error);
>> + mempool_free(vbr, vblk->pool);
>> +}
>> +
>> +static inline void virtblk_bio_done(struct virtio_blk *vblk,
>> + struct virtblk_req *vbr)
>> +{
>> + bio_endio(vbr->bio, virtblk_result(vbr));
>> + mempool_free(vbr, vblk->pool);
>> +}
>> +
>> +static void virtblk_done(struct virtqueue *vq)
>> {
>> struct virtio_blk *vblk = vq->vdev->priv;
>> + unsigned long bio_done = 0, req_done = 0;
>> struct virtblk_req *vbr;
>> - unsigned int len;
>> unsigned long flags;
>> + unsigned int len;
>>
>> spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
>> while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>> - int error;
>> -
>> - switch (vbr->status) {
>> - case VIRTIO_BLK_S_OK:
>> - error = 0;
>> - break;
>> - case VIRTIO_BLK_S_UNSUPP:
>> - error = -ENOTTY;
>> - break;
>> - default:
>> - error = -EIO;
>> - break;
>> - }
>> -
>> - switch (vbr->req->cmd_type) {
>> - case REQ_TYPE_BLOCK_PC:
>> - vbr->req->resid_len = vbr->in_hdr.residual;
>> - vbr->req->sense_len = vbr->in_hdr.sense_len;
>> - vbr->req->errors = vbr->in_hdr.errors;
>> - break;
>> - case REQ_TYPE_SPECIAL:
>> - vbr->req->errors = (error != 0);
>> - break;
>> - default:
>> - break;
>> + if (vbr->bio) {
>> + virtblk_bio_done(vblk, vbr);
>> + bio_done++;
>> + } else {
>> + virtblk_request_done(vblk, vbr);
>> + req_done++;
>> }
>> -
>> - __blk_end_request_all(vbr->req, error);
>> - mempool_free(vbr, vblk->pool);
>> }
>> /* In case queue is stopped waiting for more buffers. */
>> - blk_start_queue(vblk->disk->queue);
>> + if (req_done)
>> + blk_start_queue(vblk->disk->queue);
>> spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
>> +
>> + if (bio_done)
>> + wake_up(&vblk->queue_wait);
>> +}
>> +
>> +static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
>> + gfp_t gfp_mask)
>> +{
>> + struct virtblk_req *vbr;
>> +
>> + vbr = mempool_alloc(vblk->pool, gfp_mask);
>> + if (vbr && use_bio)
>> + sg_init_table(vbr->sg, vblk->sg_elems);
>> +
>> + return vbr;
>> }
>>
>> static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
>> @@ -106,13 +144,13 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
>> unsigned long num, out = 0, in = 0;
>> struct virtblk_req *vbr;
>>
>> - vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
>> + vbr = virtblk_alloc_req(vblk, GFP_ATOMIC);
>> if (!vbr)
>> /* When another request finishes we'll try again. */
>> return false;
>>
>> vbr->req = req;
>> -
>> + vbr->bio = NULL;
>> if (req->cmd_flags & REQ_FLUSH) {
>> vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
>> vbr->out_hdr.sector = 0;
>> @@ -172,7 +210,8 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
>> }
>> }
>>
>> - if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr, GFP_ATOMIC)<0) {
>> + if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr,
>> + GFP_ATOMIC) < 0) {
>> mempool_free(vbr, vblk->pool);
>> return false;
>> }
>> @@ -180,7 +219,7 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
>> return true;
>> }
>>
>> -static void do_virtblk_request(struct request_queue *q)
>> +static void virtblk_request(struct request_queue *q)
>> {
>> struct virtio_blk *vblk = q->queuedata;
>> struct request *req;
>> @@ -203,6 +242,82 @@ static void do_virtblk_request(struct request_queue *q)
>> virtqueue_kick(vblk->vq);
>> }
>>
>> +static void virtblk_add_buf_wait(struct virtio_blk *vblk,
>> + struct virtblk_req *vbr,
>> + unsigned long out,
>> + unsigned long in)
>> +{
>> + DEFINE_WAIT(wait);
>> +
>> + for (;;) {
>> + prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
>> + TASK_UNINTERRUPTIBLE);
>> +
>> + spin_lock_irq(vblk->disk->queue->queue_lock);
>> + if (virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
>> + GFP_ATOMIC) < 0) {
>> + spin_unlock_irq(vblk->disk->queue->queue_lock);
>> + io_schedule();
>> + } else {
>> + virtqueue_kick(vblk->vq);
>> + spin_unlock_irq(vblk->disk->queue->queue_lock);
>> + break;
>> + }
>> +
>> + }
>> +
>> + finish_wait(&vblk->queue_wait, &wait);
>> +}
>> +
>> +static void virtblk_make_request(struct request_queue *q, struct bio *bio)
>> +{
>> + struct virtio_blk *vblk = q->queuedata;
>> + unsigned int num, out = 0, in = 0;
>> + struct virtblk_req *vbr;
>> +
>> + BUG_ON(bio->bi_phys_segments + 2 > vblk->sg_elems);
>> + BUG_ON(bio->bi_rw & (REQ_FLUSH | REQ_FUA));
>> +
>> + vbr = virtblk_alloc_req(vblk, GFP_NOIO);
>> + if (!vbr) {
>> + bio_endio(bio, -ENOMEM);
>> + return;
>> + }
>> +
>> + vbr->bio = bio;
>> + vbr->req = NULL;
>> + vbr->out_hdr.type = 0;
>> + vbr->out_hdr.sector = bio->bi_sector;
>> + vbr->out_hdr.ioprio = bio_prio(bio);
>> +
>> + sg_set_buf(&vbr->sg[out++], &vbr->out_hdr, sizeof(vbr->out_hdr));
>> +
>> + num = blk_bio_map_sg(q, bio, vbr->sg + out);
>> +
>> + sg_set_buf(&vbr->sg[num + out + in++], &vbr->status,
>> + sizeof(vbr->status));
>> +
>> + if (num) {
>> + if (bio->bi_rw & REQ_WRITE) {
>> + vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
>> + out += num;
>> + } else {
>> + vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
>> + in += num;
>> + }
>> + }
>> +
>> + spin_lock_irq(vblk->disk->queue->queue_lock);
>> + if (unlikely(virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
>> + GFP_ATOMIC) < 0)) {
>> + spin_unlock_irq(vblk->disk->queue->queue_lock);
>> + virtblk_add_buf_wait(vblk, vbr, out, in);
>> + return;
>> + }
>> + virtqueue_kick(vblk->vq);
>> + spin_unlock_irq(vblk->disk->queue->queue_lock);
>> +}
>> +
>> /* return id (s/n) string for *disk to *id_str
>> */
>> static int virtblk_get_id(struct gendisk *disk, char *id_str)
>> @@ -360,7 +475,7 @@ static int init_vq(struct virtio_blk *vblk)
>> int err = 0;
>>
>> /* We expect one virtqueue, for output. */
>> - vblk->vq = virtio_find_single_vq(vblk->vdev, blk_done, "requests");
>> + vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
>> if (IS_ERR(vblk->vq))
>> err = PTR_ERR(vblk->vq);
>>
>> @@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device *vdev)
>> u8 writeback = virtblk_get_cache_mode(vdev);
>> struct virtio_blk *vblk = vdev->priv;
>>
>> - if (writeback)
>> + if (writeback && !use_bio)
>> blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
>> else
>> blk_queue_flush(vblk->disk->queue, 0);
>> @@ -477,6 +592,8 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>> struct virtio_blk *vblk;
>> struct request_queue *q;
>> int err, index;
>> + int pool_size;
>> +
>> u64 cap;
>> u32 v, blk_size, sg_elems, opt_io_size;
>> u16 min_io_size;
>> @@ -506,10 +623,12 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>> goto out_free_index;
>> }
>>
>> + init_waitqueue_head(&vblk->queue_wait);
>> vblk->vdev = vdev;
>> vblk->sg_elems = sg_elems;
>> sg_init_table(vblk->sg, vblk->sg_elems);
>> mutex_init(&vblk->config_lock);
>> +
>> INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
>> vblk->config_enable = true;
>>
>> @@ -517,7 +636,10 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>> if (err)
>> goto out_free_vblk;
>>
>> - vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
>> + pool_size = sizeof(struct virtblk_req);
>> + if (use_bio)
>> + pool_size += sizeof(struct scatterlist) * sg_elems;
>> + vblk->pool = mempool_create_kmalloc_pool(1, pool_size);
>> if (!vblk->pool) {
>> err = -ENOMEM;
>> goto out_free_vq;
>> @@ -530,12 +652,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>> goto out_mempool;
>> }
>>
>> - q = vblk->disk->queue = blk_init_queue(do_virtblk_request, NULL);
>> + q = vblk->disk->queue = blk_init_queue(virtblk_request, NULL);
>> if (!q) {
>> err = -ENOMEM;
>> goto out_put_disk;
>> }
>>
>> + if (use_bio)
>> + blk_queue_make_request(q, virtblk_make_request);
>> q->queuedata = vblk;
>>
>> virtblk_name_format("vd", index, vblk->disk->disk_name, DISK_NAME_LEN);
>> @@ -620,7 +744,6 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>> if (!err && opt_io_size)
>> blk_queue_io_opt(q, blk_size * opt_io_size);
>>
>> -
>> add_disk(vblk->disk);
>> err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial);
>> if (err)
>> --
>> 1.7.10.4
--
Asias
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Asias He @ 2012-07-30 4:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <971756309.4742543.1343457753475.JavaMail.root@redhat.com>
On 07/28/2012 02:42 PM, Paolo Bonzini wrote:
>>> I'm not sure what the correct behavior for bio & cacheflush is, if
>>> any.
>>
>> REQ_FLUSH is not supported in the bio path.
>
> Ouch, that's correct:
>
> @@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device *vdev)
> u8 writeback = virtblk_get_cache_mode(vdev);
> struct virtio_blk *vblk = vdev->priv;
>
> - if (writeback)
> + if (writeback && !use_bio)
> blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
> else
> blk_queue_flush(vblk->disk->queue, 0);
>
> then it is not safe against power losses.
Yes. Something like this:
qemu -drive file=foo.img,cache=writeback/unsafe
is not safe against power losses also?
I think we can add REQ_FLUSH & REQ_FUA support to bio path and that
deserves another patch.
--
Asias
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Jason Wang @ 2012-07-30 5:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, mashirle, kvm, netdev, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <20120729095021.GC8977@redhat.com>
On 07/29/2012 05:50 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 23, 2012 at 01:48:35PM +0800, Jason Wang wrote:
>>>> + }
>>>>
>>>> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
>>>> + ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
>>>> + (const char **)names);
>>>> + if (ret)
>>>> + goto err;
>>>> +
>>>> + if (vi->has_cvq)
>>>> vi->cvq = vqs[2];
>>>>
>>>> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
>>>> - vi->dev->features |= NETIF_F_HW_VLAN_FILTER;
>>>> + for (i = 0; i< vi->num_queue_pairs * 2; i += 2) {
>>>> + int j = i == 0 ? i : i + vi->has_cvq;
>>>> + vi->rq[i / 2]->vq = vqs[j];
>>>> + vi->sq[i / 2]->vq = vqs[j + 1];
>>> Same here.
>> Consider the code is really simple, seem no need to use helpers.
> Well it was not simple to at least one reader :)
> The problem is not this logic is complex,
> it is that it is spread all over the code.
>
> If we had e.g. vnet_tx_vqn_to_queuenum vnet_tx_queuenum_to_vqn
> and same for rx, then the logic would all be
> in one place, and have a tidy comment on top explaining
> the VQ numbering scheme.
>
Looks reasonable, thanks.
^ permalink raw reply
* Re: [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Asias He @ 2012-07-30 6:27 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <50138834.5090907@gmail.com>
Hello Sasha,
On 07/28/2012 02:35 PM, Sasha Levin wrote:
> On 07/28/2012 04:21 AM, Asias He wrote:
>> This patch introduces bio-based IO path for virtio-blk.
>>
>> Compared to request-based IO path, bio-based IO path uses driver
>> provided ->make_request_fn() method to bypasses the IO scheduler. It
>> handles the bio to device directly without allocating a request in block
>> layer. This reduces the IO path in guest kernel to achieve high IOPS
>> and lower latency. The downside is that guest can not use the IO
>> scheduler to merge and sort requests. However, this is not a big problem
>> if the backend disk in host side uses faster disk device.
>>
>> When the bio-based IO path is not enabled, virtio-blk still uses the
>> original request-based IO path, no performance difference is observed.
>>
>> Performance evaluation:
>> -----------------------------
>> 1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
>> kvm tool.
>>
>> Short version:
>> With bio-based IO path, sequential read/write, random read/write
>> IOPS boost : 28%, 24%, 21%, 16%
>> Latency improvement: 32%, 17%, 21%, 16%
>>
>> Long version:
>> With bio-based IO path:
>> seq-read : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
>> seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
>> rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
>> rand-write: io=3095.7MB, bw=96198KB/s, iops=192396 , runt= 32952msec
>> clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
>> clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
>> clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
>> clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
>> cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
>> cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
>> cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
>> cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
>> With request-based IO path:
>> seq-read : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
>> seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
>> rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
>> rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
>> clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
>> clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
>> clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
>> clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
>> cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
>> cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
>> cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
>> cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985
>>
>> 2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
>> kvm tool.
>>
>> Short version:
>> With bio-based IO path, sequential read/write, random read/write
>> IOPS boost : 11%, 11%, 13%, 10%
>> Latency improvement: 10%, 10%, 12%, 10%
>> Long Version:
>> With bio-based IO path:
>> read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
>> write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
>> read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
>> write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
>> clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
>> clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
>> clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
>> clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
>> cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
>> cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
>> cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
>> cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
>> With request-based IO path:
>> read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
>> write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
>> read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
>> write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
>> clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
>> clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
>> clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
>> clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
>> cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
>> cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
>> cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
>> cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409
>
> What are the cases where we'll see a performance degradation with using the bio path? Could we measure performance for those as well?
>
>> How to use:
>> -----------------------------
>> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
>> use_bio=1' to enable ->make_request_fn() based I/O path.
>
> If there are, in fact, no cases where performance is degraded, can use_bio=1 be the default?
Here are some results on a SATA based image file. In this case, the bio
path is slower than req path doing sequential read/write.
qemu use_bio=0 (req-based)
-------------------------------------
read : io=120964KB, bw=26098KB/s, iops=6470 , runt= 4635msec
write: io=190236KB, bw=35505KB/s, iops=8829 , runt= 5358msec
read : io=257400KB, bw=1963.7KB/s, iops=488 , runt=131081msec
write: io=258944KB, bw=1302.7KB/s, iops=324 , runt=198872msec
clat (msec): min=1 , max=1527 , avg=30.73, stdev=144.73
clat (usec): min=811 , max=247072 , avg=28451.71, stdev=16107.22
clat (msec): min=6 , max=2519 , avg=513.91, stdev=231.07
clat (msec): min=33 , max=2621 , avg=772.33, stdev=348.39
cpu : usr=4.05%, sys=14.56%, ctx=38199, majf=0, minf=4
cpu : usr=4.02%, sys=15.48%, ctx=53724, majf=0, minf=0
cpu : usr=0.15%, sys=0.30%, ctx=20535, majf=0, minf=16
cpu : usr=0.32%, sys=0.96%, ctx=101465, majf=0, minf=0
qemu use_bio=1 (bio-based)
-------------------------------------
read : io=202736KB, bw=25569KB/s, iops=6360 , runt= 7929msec
write: io=217844KB, bw=20335KB/s, iops=5060 , runt= 10713msec
read : io=256980KB, bw=1958.2KB/s, iops=487 , runt=131235msec
write: io=258288KB, bw=1423.9KB/s, iops=354 , runt=181405msec
clat (usec): min=922 , max=1578.2K, avg=38702.18, stdev=99248.33
clat (usec): min=460 , max=241314 , avg=49326.52, stdev=18705.68
clat (msec): min=19 , max=2370 , avg=515.30, stdev=200.84
clat (msec): min=11 , max=3751 , avg=702.60, stdev=286.93
cpu : usr=2.54%, sys=8.75%, ctx=68522, majf=0, minf=6
cpu : usr=1.96%, sys=7.70%, ctx=70003, majf=0, minf=0
cpu : usr=0.39%, sys=1.46%, ctx=259459, majf=0, minf=16
cpu : usr=0.28%, sys=1.21%, ctx=265148, majf=0, minf=0
lkvm use_bio=0 (req-based)
-------------------------------------
read : io=150120KB, bw=40420KB/s, iops=10037 , runt= 3714msec
write: io=194932KB, bw=27029KB/s, iops=6722 , runt= 7212msec
read : io=257136KB, bw=2001.1KB/s, iops=498 , runt=128443msec
write: io=258276KB, bw=1537.2KB/s, iops=382 , runt=168028msec
clat (msec): min=1 , max=1542 , avg=24.84, stdev=32.45
clat (msec): min=3 , max=628 , avg=35.62, stdev=39.71
clat (msec): min=8 , max=2540 , avg=503.28, stdev=236.97
clat (msec): min=41 , max=4398 , avg=653.88, stdev=302.61
cpu : usr=3.91%, sys=15.75%, ctx=26968, majf=0, minf=23
cpu : usr=2.50%, sys=10.56%, ctx=19090, majf=0, minf=0
cpu : usr=0.16%, sys=0.43%, ctx=20159, majf=0, minf=16
cpu : usr=0.18%, sys=0.53%, ctx=81364, majf=0, minf=0
lkvm use_bio=1 (bio-based)
-------------------------------------
read : io=124812KB, bw=36537KB/s, iops=9060 , runt= 3416msec
write: io=169180KB, bw=24406KB/s, iops=6065 , runt= 6932msec
read : io=256200KB, bw=2089.3KB/s, iops=520 , runt=122630msec
write: io=257988KB, bw=1545.7KB/s, iops=384 , runt=166910msec
clat (msec): min=1 , max=1527 , avg=28.06, stdev=89.54
clat (msec): min=2 , max=344 , avg=41.12, stdev=38.70
clat (msec): min=8 , max=1984 , avg=490.63, stdev=207.28
clat (msec): min=33 , max=4131 , avg=659.19, stdev=304.71
cpu : usr=4.85%, sys=17.15%, ctx=31593, majf=0, minf=7
cpu : usr=3.04%, sys=11.45%, ctx=39377, majf=0, minf=0
cpu : usr=0.47%, sys=1.59%, ctx=262986, majf=0, minf=16
cpu : usr=0.47%, sys=1.46%, ctx=337410, majf=0, minf=0
--
Asias
^ permalink raw reply
* Re: [net-next RFC V5 3/5] virtio: intorduce an API to set affinity for a virtqueue
From: Paolo Bonzini @ 2012-07-30 6:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, kvm, netdev, mashirle, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <20120729204030.GB13802@redhat.com>
Il 29/07/2012 22:40, Michael S. Tsirkin ha scritto:
>> > Did you set the affinity manually in your experiments, or perhaps there
>> > is a difference between scsi and networking... (interrupt mitigation?)
>
> You need to run irqbalancer in guest to make it actually work. Do you?
Yes, of course, now on to debugging that one. I just wanted to ask
before the weekend if I was missing something as obvious as that.
Paolo
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Paolo Bonzini @ 2012-07-30 7:31 UTC (permalink / raw)
To: Asias He
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <501610E0.8000504@redhat.com>
Il 30/07/2012 06:43, Asias He ha scritto:
>>
>
> Yes. Something like this:
>
> qemu -drive file=foo.img,cache=writeback/unsafe
>
> is not safe against power losses also?
cache=writeback and cache=none are safe, cache=unsafe isn't.
> I think we can add REQ_FLUSH & REQ_FUA support to bio path and that
> deserves another patch.
You only need to add REQ_FLUSH support. The virtio-blk protocol does
not support REQ_FUA, because there's no easy way to do it in userspace.
Paolo
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Sasha Levin @ 2012-07-30 13:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, kvm, netdev, mashirle, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <20120729094451.GB8977@redhat.com>
On 07/29/2012 11:44 AM, Michael S. Tsirkin wrote:
> On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote:
>> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
>>>> - err = init_vqs(vi);
>>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>>>>> + vi->has_cvq = true;
>>>>> +
>>> How about we disable multiqueue if there's no cvq?
>>> Will make logic a bit simpler, won't it?
>>
>> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
>
> Well !cvq support is a legacy feature: the reason we support it
> in driver is to avoid breaking on old hosts. Adding more code to that
> path just doesn't make much sense since old hosts won't have mq.
Is it really a legacy feature? The spec suggests that its an optional queue which is not necessary for the operation of the device.
Which is why we never implemented it in lkvm - we weren't interested in any of the features it provided at that time and we could provide high performance with vhost support even without it.
^ permalink raw reply
* Re: [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Christoph Hellwig @ 2012-07-30 13:42 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <87a9yim2qg.fsf@rustcorp.com.au>
On Mon, Jul 30, 2012 at 11:25:51AM +0930, Rusty Russell wrote:
> I consider this approach a half-way step. Quick attempts on my laptop
> and I couldn't find a case where the bio path was a loss, but in theory
> if the host wasn't doing any reordering and it was a slow device, you'd
> want the guest to do so.
>
> I'm not sure if current qemu can be configured to do such a thing?
The host kernel will do the I/O scheduling for you unless you explicitly
disable it. And we should be able to assume an administrator will only
disable it when they have a reason for it - if not they'll get worse
performance for non-virtualized workloads as well.
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Christoph Hellwig @ 2012-07-30 13:43 UTC (permalink / raw)
To: Asias He
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Paolo Bonzini, Christoph Hellwig
In-Reply-To: <501610E0.8000504@redhat.com>
On Mon, Jul 30, 2012 at 12:43:12PM +0800, Asias He wrote:
> I think we can add REQ_FLUSH & REQ_FUA support to bio path and that
> deserves another patch.
Adding it is a requirement for merging the code.
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Christoph Hellwig @ 2012-07-30 13:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <5016383A.5090008@redhat.com>
On Mon, Jul 30, 2012 at 09:31:06AM +0200, Paolo Bonzini wrote:
> You only need to add REQ_FLUSH support. The virtio-blk protocol does
> not support REQ_FUA, because there's no easy way to do it in userspace.
A bio-based driver needs to handle both REQ_FLUSH and REQ_FUA as it does
not get the sequencing of REQ_FUA into REQ_FLUSH that request based drivers
can request. To what the REQ_FUA request gets translated is a different story.
^ permalink raw reply
* Re: [RFC-v5] tcm_vhost: Initial merge for vhost level target fabric driver
From: Nicholas A. Bellinger @ 2012-07-30 22:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, Anthony Liguori, Stefan Hajnoczi, kvm-devel,
Greg Kroah-Hartman, qemu-devel, lf-virt, Anthony Liguori,
target-devel, linux-scsi, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <20120729134051.GC10041@redhat.com>
On Sun, 2012-07-29 at 16:40 +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 26, 2012 at 11:43:20PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > Hello KVM + QEMU folks,
> >
> > This is -v5 of the in-flight tcm_vhost fabric driver for KVM host virtualized
> > target support using the in-kernel storage stack with for-3.6 code.
> >
> > The changes since -v4 have been pretty minimal. A new GET_ABI_VERSION ioctl
> > has been added as requested by Anthony & Co, and after some back <-> forth on
> > the lists we've decided upon a starting point of VHOST_SCSI_ABI_VERSION=0,
> > and vhost-scsi RFC userspace has been updated accordingly.
> >
> > Along with that, a few handfuls of vq related error messages where converted
> > to use vq_err as requested by MST, along with some minor checkpatch fixes.
> >
> > So this point everything required to run tcm_vhost from drivers/target +
> > drivers/vhost, and virtio-scsi LLD bugfix has been merged into v3.6-rc0 code.
> >
> > Please have a look and let us know if there are any last-minute pre-merge
> > comments ahead of MST returning from holiday this weekend.
> >
> > Thank you!
> >
> > --nab
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> some comments below but can be fixed in-tree I think.
>
Thanks alot MST!
I've added your ACK for -v5 RFC code that's in 07302012 linux-next, and
made the changes requested below as an incremental patch that will be
sending out shortly.
My comments are inline below..
> > Changelog v4 -> v5:
> >
> > Expose ABI version via VHOST_SCSI_GET_ABI_VERSION + use Rev 0 as
> > starting point for v3.6-rc code (Stefan + ALiguori + nab)
> > Convert vhost_scsi_handle_vq() to vq_err() (nab + MST)
> > Minor style fixes from checkpatch (nab)
> >
> > Changelog v3 -> v4:
> >
> > Rename vhost_vring_target -> vhost_scsi_target (mst + nab)
> > Use TRANSPORT_IQN_LEN in vhost_scsi_target->vhost_wwpn[] def (nab)
> > Move back to drivers/vhost/, and just use drivers/vhost/Kconfig.tcm (mst)
> > Move TCM_VHOST related ioctl defines from include/linux/vhost.h ->
> > drivers/vhost/tcm_vhost.h as requested by MST (nab)
> > Move Kbuild.tcm include from drivers/staging -> drivers/vhost/, and
> > just use 'if STAGING' around 'source drivers/vhost/Kbuild.tcm'
> >
> > Changelog v2 -> v3:
> >
> > Unlock on error in tcm_vhost_drop_nexus() (DanC)
> > Fix strlen() doesn't count the terminator (DanC)
> > Call kfree() on an error path (DanC)
> > Convert tcm_vhost_write_pending to use target_execute_cmd (hch + nab)
> > Fix another strlen() off by one in tcm_vhost_make_tport (DanC)
> > Add option under drivers/staging/Kconfig, and move to drivers/vhost/tcm/
> > as requested by MST (nab)
> >
> > Changelog v1 -> v2:
> >
> > Fix tv_cmd completion -> release SGL memory leak (nab)
> > Fix sparse warnings for static variable usage ((Fengguang Wu)
> > Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
> > Convert to cmwq submission for I/O dispatch (nab + hch)
> >
> > Changelog v0 -> v1:
> >
> > Merge into single source + header file, and move to drivers/vhost/
> >
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Cc: Zhi Yong Wu <wuzhy@cn.ibm.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> > drivers/vhost/Kconfig | 3 +
> > drivers/vhost/Kconfig.tcm | 6 +
> > drivers/vhost/Makefile | 2 +
> > drivers/vhost/tcm_vhost.c | 1628 +++++++++++++++++++++++++++++++++++++++++++++
> > drivers/vhost/tcm_vhost.h | 101 +++
> > 5 files changed, 1740 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/vhost/Kconfig.tcm
> > create mode 100644 drivers/vhost/tcm_vhost.c
> > create mode 100644 drivers/vhost/tcm_vhost.h
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index e4e2fd1..202bba6 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -9,3 +9,6 @@ config VHOST_NET
> > To compile this driver as a module, choose M here: the module will
> > be called vhost_net.
> >
> > +if STAGING
> > +source "drivers/vhost/Kconfig.tcm"
> > +endif
> > diff --git a/drivers/vhost/Kconfig.tcm b/drivers/vhost/Kconfig.tcm
> > new file mode 100644
> > index 0000000..a9c6f76
> > --- /dev/null
> > +++ b/drivers/vhost/Kconfig.tcm
> > @@ -0,0 +1,6 @@
> > +config TCM_VHOST
> > + tristate "TCM_VHOST fabric module (EXPERIMENTAL)"
> > + depends on TARGET_CORE && EVENTFD && EXPERIMENTAL && m
> > + default n
> > + ---help---
> > + Say M here to enable the TCM_VHOST fabric module for use with virtio-scsi guests
> > diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> > index 72dd020..a27b053 100644
> > --- a/drivers/vhost/Makefile
> > +++ b/drivers/vhost/Makefile
> > @@ -1,2 +1,4 @@
> > obj-$(CONFIG_VHOST_NET) += vhost_net.o
> > vhost_net-y := vhost.o net.o
> > +
> > +obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
> > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > new file mode 100644
> > index 0000000..fb36654
> > --- /dev/null
> > +++ b/drivers/vhost/tcm_vhost.c
> > @@ -0,0 +1,1628 @@
> > +/*******************************************************************************
> > + * Vhost kernel TCM fabric driver for virtio SCSI initiators
> > + *
> > + * (C) Copyright 2010-2012 RisingTide Systems LLC.
> > + * (C) Copyright 2010-2012 IBM Corp.
> > + *
> > + * Licensed to the Linux Foundation under the General Public License (GPL) version 2.
> > + *
> > + * Authors: Nicholas A. Bellinger <nab@risingtidesystems.com>
> > + * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + ****************************************************************************/
> > +
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <generated/utsrelease.h>
> > +#include <linux/utsname.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/kthread.h>
> > +#include <linux/types.h>
> > +#include <linux/string.h>
> > +#include <linux/configfs.h>
> > +#include <linux/ctype.h>
> > +#include <linux/compat.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/vhost.h>
> > +#include <linux/fs.h>
> > +#include <linux/miscdevice.h>
> > +#include <asm/unaligned.h>
> > +#include <scsi/scsi.h>
> > +#include <scsi/scsi_tcq.h>
> > +#include <target/target_core_base.h>
> > +#include <target/target_core_fabric.h>
> > +#include <target/target_core_fabric_configfs.h>
> > +#include <target/target_core_configfs.h>
> > +#include <target/configfs_macros.h>
> > +#include <linux/vhost.h>
> > +#include <linux/virtio_net.h> /* TODO vhost.h currently depends on this */
> > +#include <linux/virtio_scsi.h>
> > +
> > +#include "vhost.c"
> > +#include "vhost.h"
> > +#include "tcm_vhost.h"
> > +
> > +struct vhost_scsi {
> > + atomic_t vhost_ref_cnt;
> > + struct tcm_vhost_tpg *vs_tpg;
>
> Would b enice to add a note on locking about this field.
>
Done
> > + struct vhost_dev dev;
> > + struct vhost_virtqueue vqs[3];
> > +
> > + struct vhost_work vs_completion_work; /* cmd completion work item */
> > + struct list_head vs_completion_list; /* cmd completion queue */
> > + spinlock_t vs_completion_lock; /* protects s_completion_list */
> > +};
> > +
> > +/* Local pointer to allocated TCM configfs fabric module */
> > +static struct target_fabric_configfs *tcm_vhost_fabric_configfs;
> > +
> > +static struct workqueue_struct *tcm_vhost_workqueue;
> > +
> > +/* Global spinlock to protect tcm_vhost TPG list for vhost IOCTL access */
> > +static DEFINE_MUTEX(tcm_vhost_mutex);
> > +static LIST_HEAD(tcm_vhost_list);
> > +
> > +static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
> > +{
> > + return 1;
> > +}
> > +
> > +static int tcm_vhost_check_false(struct se_portal_group *se_tpg)
> > +{
> > + return 0;
> > +}
> > +
> > +static char *tcm_vhost_get_fabric_name(void)
> > +{
> > + return "vhost";
> > +}
> > +
> > +static u8 tcm_vhost_get_fabric_proto_ident(struct se_portal_group *se_tpg)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_tport *tport = tpg->tport;
> > +
> > + switch (tport->tport_proto_id) {
> > + case SCSI_PROTOCOL_SAS:
> > + return sas_get_fabric_proto_ident(se_tpg);
> > + case SCSI_PROTOCOL_FCP:
> > + return fc_get_fabric_proto_ident(se_tpg);
> > + case SCSI_PROTOCOL_ISCSI:
> > + return iscsi_get_fabric_proto_ident(se_tpg);
> > + default:
> > + pr_err("Unknown tport_proto_id: 0x%02x, using"
> > + " SAS emulation\n", tport->tport_proto_id);
> > + break;
> > + }
> > +
> > + return sas_get_fabric_proto_ident(se_tpg);
> > +}
> > +
> > +static char *tcm_vhost_get_fabric_wwn(struct se_portal_group *se_tpg)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_tport *tport = tpg->tport;
> > +
> > + return &tport->tport_name[0];
> > +}
> > +
> > +static u16 tcm_vhost_get_tag(struct se_portal_group *se_tpg)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + return tpg->tport_tpgt;
> > +}
> > +
> > +static u32 tcm_vhost_get_default_depth(struct se_portal_group *se_tpg)
> > +{
> > + return 1;
> > +}
> > +
> > +static u32 tcm_vhost_get_pr_transport_id(
> > + struct se_portal_group *se_tpg,
> > + struct se_node_acl *se_nacl,
> > + struct t10_pr_registration *pr_reg,
> > + int *format_code,
> > + unsigned char *buf)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_tport *tport = tpg->tport;
> > +
> > + switch (tport->tport_proto_id) {
> > + case SCSI_PROTOCOL_SAS:
> > + return sas_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> > + format_code, buf);
> > + case SCSI_PROTOCOL_FCP:
> > + return fc_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> > + format_code, buf);
> > + case SCSI_PROTOCOL_ISCSI:
> > + return iscsi_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> > + format_code, buf);
> > + default:
> > + pr_err("Unknown tport_proto_id: 0x%02x, using"
> > + " SAS emulation\n", tport->tport_proto_id);
> > + break;
> > + }
> > +
> > + return sas_get_pr_transport_id(se_tpg, se_nacl, pr_reg,
> > + format_code, buf);
> > +}
> > +
> > +static u32 tcm_vhost_get_pr_transport_id_len(
> > + struct se_portal_group *se_tpg,
> > + struct se_node_acl *se_nacl,
> > + struct t10_pr_registration *pr_reg,
> > + int *format_code)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_tport *tport = tpg->tport;
> > +
> > + switch (tport->tport_proto_id) {
> > + case SCSI_PROTOCOL_SAS:
> > + return sas_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> > + format_code);
> > + case SCSI_PROTOCOL_FCP:
> > + return fc_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> > + format_code);
> > + case SCSI_PROTOCOL_ISCSI:
> > + return iscsi_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> > + format_code);
> > + default:
> > + pr_err("Unknown tport_proto_id: 0x%02x, using"
> > + " SAS emulation\n", tport->tport_proto_id);
> > + break;
> > + }
> > +
> > + return sas_get_pr_transport_id_len(se_tpg, se_nacl, pr_reg,
> > + format_code);
> > +}
> > +
> > +static char *tcm_vhost_parse_pr_out_transport_id(
> > + struct se_portal_group *se_tpg,
> > + const char *buf,
> > + u32 *out_tid_len,
> > + char **port_nexus_ptr)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_tport *tport = tpg->tport;
> > +
> > + switch (tport->tport_proto_id) {
> > + case SCSI_PROTOCOL_SAS:
> > + return sas_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> > + port_nexus_ptr);
> > + case SCSI_PROTOCOL_FCP:
> > + return fc_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> > + port_nexus_ptr);
> > + case SCSI_PROTOCOL_ISCSI:
> > + return iscsi_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> > + port_nexus_ptr);
> > + default:
> > + pr_err("Unknown tport_proto_id: 0x%02x, using"
> > + " SAS emulation\n", tport->tport_proto_id);
> > + break;
> > + }
> > +
> > + return sas_parse_pr_out_transport_id(se_tpg, buf, out_tid_len,
> > + port_nexus_ptr);
> > +}
> > +
> > +static struct se_node_acl *tcm_vhost_alloc_fabric_acl(
> > + struct se_portal_group *se_tpg)
> > +{
> > + struct tcm_vhost_nacl *nacl;
> > +
> > + nacl = kzalloc(sizeof(struct tcm_vhost_nacl), GFP_KERNEL);
> > + if (!nacl) {
> > + pr_err("Unable to alocate struct tcm_vhost_nacl\n");
> > + return NULL;
> > + }
> > +
> > + return &nacl->se_node_acl;
> > +}
> > +
> > +static void tcm_vhost_release_fabric_acl(
> > + struct se_portal_group *se_tpg,
> > + struct se_node_acl *se_nacl)
> > +{
> > + struct tcm_vhost_nacl *nacl = container_of(se_nacl,
> > + struct tcm_vhost_nacl, se_node_acl);
> > + kfree(nacl);
> > +}
> > +
> > +static u32 tcm_vhost_tpg_get_inst_index(struct se_portal_group *se_tpg)
> > +{
> > + return 1;
> > +}
> > +
> > +static void tcm_vhost_release_cmd(struct se_cmd *se_cmd)
> > +{
> > + return;
> > +}
> > +
> > +static int tcm_vhost_shutdown_session(struct se_session *se_sess)
> > +{
> > + return 0;
> > +}
> > +
> > +static void tcm_vhost_close_session(struct se_session *se_sess)
> > +{
> > + return;
> > +}
> > +
> > +static u32 tcm_vhost_sess_get_index(struct se_session *se_sess)
> > +{
> > + return 0;
> > +}
> > +
> > +static int tcm_vhost_write_pending(struct se_cmd *se_cmd)
> > +{
> > + /* Go ahead and process the write immediately */
> > + target_execute_cmd(se_cmd);
> > + return 0;
> > +}
> > +
> > +static int tcm_vhost_write_pending_status(struct se_cmd *se_cmd)
> > +{
> > + return 0;
> > +}
> > +
> > +static void tcm_vhost_set_default_node_attrs(struct se_node_acl *nacl)
> > +{
> > + return;
> > +}
> > +
> > +static u32 tcm_vhost_get_task_tag(struct se_cmd *se_cmd)
> > +{
> > + return 0;
> > +}
> > +
> > +static int tcm_vhost_get_cmd_state(struct se_cmd *se_cmd)
> > +{
> > + return 0;
> > +}
> > +
> > +static void vhost_scsi_complete_cmd(struct tcm_vhost_cmd *);
>
> can we reorder code to avoid forward declarations?
>
<nod>, moved vhost_scsi_complete_cmd() up here..
> > +
> > +static int tcm_vhost_queue_data_in(struct se_cmd *se_cmd)
> > +{
> > + struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
> > + struct tcm_vhost_cmd, tvc_se_cmd);
> > + vhost_scsi_complete_cmd(tv_cmd);
> > + return 0;
> > +}
> > +
> > +static int tcm_vhost_queue_status(struct se_cmd *se_cmd)
> > +{
> > + struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
> > + struct tcm_vhost_cmd, tvc_se_cmd);
> > + vhost_scsi_complete_cmd(tv_cmd);
> > + return 0;
> > +}
> > +
> > +static int tcm_vhost_queue_tm_rsp(struct se_cmd *se_cmd)
> > +{
> > + return 0;
> > +}
> > +
> > +static u16 tcm_vhost_set_fabric_sense_len(struct se_cmd *se_cmd,
> > + u32 sense_length)
> > +{
> > + return 0;
> > +}
> > +
> > +static u16 tcm_vhost_get_fabric_sense_len(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> > +{
> > + struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
> > +
> > + /* TODO locking against target/backend threads? */
> > + transport_generic_free_cmd(se_cmd, 1);
> > +
> > + if (tv_cmd->tvc_sgl_count) {
> > + u32 i;
> > + for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
> > + put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> > +
> > + kfree(tv_cmd->tvc_sgl);
> > + }
> > +
> > + kfree(tv_cmd);
> > +}
> > +
> > +/* Dequeue a command from the completion list */
> > +static struct tcm_vhost_cmd *vhost_scsi_get_cmd_from_completion(
> > + struct vhost_scsi *vs)
> > +{
> > + struct tcm_vhost_cmd *tv_cmd = NULL;
> > +
> > + spin_lock_bh(&vs->vs_completion_lock);
> > + if (list_empty(&vs->vs_completion_list)) {
> > + spin_unlock_bh(&vs->vs_completion_lock);
> > + return NULL;
> > + }
> > +
> > + list_for_each_entry(tv_cmd, &vs->vs_completion_list,
> > + tvc_completion_list) {
> > + list_del(&tv_cmd->tvc_completion_list);
> > + break;
> > + }
> > + spin_unlock_bh(&vs->vs_completion_lock);
> > + return tv_cmd;
> > +}
> > +
> > +/* Fill in status and signal that we are done processing this command
> > + *
> > + * This is scheduled in the vhost work queue so we are called with the owner
> > + * process mm and can access the vring.
> > + */
> > +static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
> > +{
> > + struct vhost_scsi *vs = container_of(work, struct vhost_scsi,
> > + vs_completion_work);
> > + struct tcm_vhost_cmd *tv_cmd;
> > +
> > + while ((tv_cmd = vhost_scsi_get_cmd_from_completion(vs)) != NULL) {
>
> I normally prefer while ((tv_cmd = vhost_scsi_get_cmd_from_completion(vs)))
> without != NULL
>
Dropping the extra '!= NULL' conditional check here..
> > + struct virtio_scsi_cmd_resp v_rsp;
> > + struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
> > + int ret;
> > +
> > + pr_debug("%s tv_cmd %p resid %u status %#02x\n", __func__,
> > + tv_cmd, se_cmd->residual_count, se_cmd->scsi_status);
> > +
> > + memset(&v_rsp, 0, sizeof(v_rsp));
> > + v_rsp.resid = se_cmd->residual_count;
> > + /* TODO is status_qualifier field needed? */
> > + v_rsp.status = se_cmd->scsi_status;
> > + v_rsp.sense_len = se_cmd->scsi_sense_length;
> > + memcpy(v_rsp.sense, tv_cmd->tvc_sense_buf,
> > + v_rsp.sense_len);
> > + ret = copy_to_user(tv_cmd->tvc_resp, &v_rsp, sizeof(v_rsp));
> > + if (likely(ret == 0))
> > + vhost_add_used(&vs->vqs[2], tv_cmd->tvc_vq_desc, 0);
> > + else
> > + pr_err("Faulted on virtio_scsi_cmd_resp\n");
> > +
> > + vhost_scsi_free_cmd(tv_cmd);
> > + }
> > +
> > + vhost_signal(&vs->dev, &vs->vqs[2]);
> > +}
> > +
> > +static void vhost_scsi_complete_cmd(struct tcm_vhost_cmd *tv_cmd)
> > +{
> > + struct vhost_scsi *vs = tv_cmd->tvc_vhost;
> > +
> > + pr_debug("%s tv_cmd %p\n", __func__, tv_cmd);
>
> Is this doing something you can not already do with
> tracing? If all these pr_debug messages are useful,
> I think it's best to add a kconfig entry for them
> so that they are off for people even if they build
> debug kernels.
>
So I ended up just dropping this particular noisemaker while moving
vhost_scsi_complete_cmd().
As for CONFIG_DYNAMIC_DEBUG=y usage, this is what we've used for target
core + fabric debug messages since the v3.1 days when Andy Grover did
the conversion in the old lio-core tree..
IHMO they are pretty nice to have when your helping someone to debug an
issue they are experiencing. ;)
> > +
> > + spin_lock_bh(&vs->vs_completion_lock);
> > + list_add_tail(&tv_cmd->tvc_completion_list, &vs->vs_completion_list);
> > + spin_unlock_bh(&vs->vs_completion_lock);
> > +
> > + vhost_work_queue(&vs->dev, &vs->vs_completion_work);
> > +}
> > +
> > +static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
> > + struct tcm_vhost_tpg *tv_tpg,
> > + struct virtio_scsi_cmd_req *v_req,
> > + u32 exp_data_len,
> > + int data_direction)
> > +{
> > + struct tcm_vhost_cmd *tv_cmd;
> > + struct tcm_vhost_nexus *tv_nexus;
> > + struct se_portal_group *se_tpg = &tv_tpg->se_tpg;
> > + struct se_session *se_sess;
> > + struct se_cmd *se_cmd;
> > + int sam_task_attr;
> > +
> > + tv_nexus = tv_tpg->tpg_nexus;
> > + if (!tv_nexus) {
> > + pr_err("Unable to locate active struct tcm_vhost_nexus\n");
> > + return ERR_PTR(-EIO);
> > + }
> > + se_sess = tv_nexus->tvn_se_sess;
> > +
> > + tv_cmd = kzalloc(sizeof(struct tcm_vhost_cmd), GFP_ATOMIC);
> > + if (!tv_cmd) {
> > + pr_err("Unable to allocate struct tcm_vhost_cmd\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + INIT_LIST_HEAD(&tv_cmd->tvc_completion_list);
> > + tv_cmd->tvc_tag = v_req->tag;
> > +
> > + se_cmd = &tv_cmd->tvc_se_cmd;
> > + /*
> > + * Locate the SAM Task Attr from virtio_scsi_cmd_req
> > + */
> > + sam_task_attr = v_req->task_attr;
> > + /*
> > + * Initialize struct se_cmd descriptor from TCM infrastructure
> > + */
> > + transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, exp_data_len,
> > + data_direction, sam_task_attr,
> > + &tv_cmd->tvc_sense_buf[0]);
> > +
> > +#if 0 /* FIXME: vhost_scsi_allocate_cmd() BIDI operation */
> > + if (bidi)
> > + se_cmd->se_cmd_flags |= SCF_BIDI;
> > +#endif
> > + return tv_cmd;
> > +}
> > +
> > +/*
> > + * Map a user memory range into a scatterlist
> > + *
> > + * Returns the number of scatterlist entries used or -errno on error.
> > + */
> > +static int vhost_scsi_map_to_sgl(struct scatterlist *sgl,
> > + unsigned int sgl_count, void __user *ptr, size_t len, int write)
> > +{
> > + struct scatterlist *sg = sgl;
> > + unsigned int npages = 0;
> > + int ret;
> > +
> > + while (len > 0) {
> > + struct page *page;
> > + unsigned int offset = (uintptr_t)ptr & ~PAGE_MASK;
> > + unsigned int nbytes = min_t(unsigned int,
> > + PAGE_SIZE - offset, len);
> > +
> > + if (npages == sgl_count) {
> > + ret = -ENOBUFS;
> > + goto err;
> > + }
> > +
> > + ret = get_user_pages_fast((unsigned long)ptr, 1, write, &page);
> > + BUG_ON(ret == 0); /* we should either get our page or fail */
> > + if (ret < 0)
> > + goto err;
> > +
> > + sg_set_page(sg, page, nbytes, offset);
> > + ptr += nbytes;
> > + len -= nbytes;
> > + sg++;
> > + npages++;
> > + }
> > + return npages;
> > +
> > +err:
> > + /* Put pages that we hold */
> > + for (sg = sgl; sg != &sgl[npages]; sg++)
> > + put_page(sg_page(sg));
> > + return ret;
> > +}
> > +
> > +static int vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *tv_cmd,
> > + struct iovec *iov, unsigned int niov, int write)
> > +{
> > + int ret;
> > + unsigned int i;
> > + u32 sgl_count;
> > + struct scatterlist *sg;
> > +
> > + /*
> > + * Find out how long sglist needs to be
> > + */
> > + sgl_count = 0;
> > + for (i = 0; i < niov; i++) {
> > + sgl_count += (((uintptr_t)iov[i].iov_base + iov[i].iov_len +
> > + PAGE_SIZE - 1) >> PAGE_SHIFT) -
> > + ((uintptr_t)iov[i].iov_base >> PAGE_SHIFT);
> > + }
> > + /* TODO overflow checking */
> > +
> > + sg = kmalloc(sizeof(tv_cmd->tvc_sgl[0]) * sgl_count, GFP_ATOMIC);
> > + if (!sg)
> > + return -ENOMEM;
> > + pr_debug("%s sg %p sgl_count %u is_err %ld\n", __func__,
> > + sg, sgl_count, IS_ERR(sg));
> > + sg_init_table(sg, sgl_count);
> > +
> > + tv_cmd->tvc_sgl = sg;
> > + tv_cmd->tvc_sgl_count = sgl_count;
> > +
> > + pr_debug("Mapping %u iovecs for %u pages\n", niov, sgl_count);
> > + for (i = 0; i < niov; i++) {
> > + ret = vhost_scsi_map_to_sgl(sg, sgl_count, iov[i].iov_base,
> > + iov[i].iov_len, write);
> > + if (ret < 0) {
> > + for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
> > + put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> > + kfree(tv_cmd->tvc_sgl);
> > + tv_cmd->tvc_sgl = NULL;
> > + tv_cmd->tvc_sgl_count = 0;
> > + return ret;
> > + }
> > +
> > + sg += ret;
> > + sgl_count -= ret;
> > + }
> > + return 0;
> > +}
> > +
> > +static void tcm_vhost_submission_work(struct work_struct *work)
> > +{
> > + struct tcm_vhost_cmd *tv_cmd =
> > + container_of(work, struct tcm_vhost_cmd, work);
> > + struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
> > + struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
> > + int rc, sg_no_bidi = 0;
> > + /*
> > + * Locate the struct se_lun pointer based on v_req->lun, and
> > + * attach it to struct se_cmd
> > + */
> > + rc = transport_lookup_cmd_lun(&tv_cmd->tvc_se_cmd, tv_cmd->tvc_lun);
> > + if (rc < 0) {
> > + pr_err("Failed to look up lun: %d\n", tv_cmd->tvc_lun);
> > + transport_send_check_condition_and_sense(&tv_cmd->tvc_se_cmd,
> > + tv_cmd->tvc_se_cmd.scsi_sense_reason, 0);
> > + transport_generic_free_cmd(se_cmd, 0);
> > + return;
> > + }
> > +
> > + rc = target_setup_cmd_from_cdb(se_cmd, tv_cmd->tvc_cdb);
> > + if (rc == -ENOMEM) {
> > + transport_send_check_condition_and_sense(se_cmd,
> > + TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
> > + transport_generic_free_cmd(se_cmd, 0);
> > + return;
> > + } else if (rc < 0) {
> > + if (se_cmd->se_cmd_flags & SCF_SCSI_RESERVATION_CONFLICT)
> > + tcm_vhost_queue_status(se_cmd);
> > + else
> > + transport_send_check_condition_and_sense(se_cmd,
> > + se_cmd->scsi_sense_reason, 0);
> > + transport_generic_free_cmd(se_cmd, 0);
> > + return;
> > + }
> > +
> > + if (tv_cmd->tvc_sgl_count) {
> > + sg_ptr = tv_cmd->tvc_sgl;
> > + /*
> > + * For BIDI commands, pass in the extra READ buffer
> > + * to transport_generic_map_mem_to_cmd() below..
> > + */
> > +/* FIXME: Fix BIDI operation in tcm_vhost_submission_work() */
> > +#if 0
> > + if (se_cmd->se_cmd_flags & SCF_BIDI) {
> > + sg_bidi_ptr = NULL;
> > + sg_no_bidi = 0;
> > + }
> > +#endif
> > + } else {
> > + sg_ptr = NULL;
> > + }
> > +
> > + rc = transport_generic_map_mem_to_cmd(se_cmd, sg_ptr,
> > + tv_cmd->tvc_sgl_count, sg_bidi_ptr,
> > + sg_no_bidi);
> > + if (rc < 0) {
> > + transport_send_check_condition_and_sense(se_cmd,
> > + se_cmd->scsi_sense_reason, 0);
> > + transport_generic_free_cmd(se_cmd, 0);
> > + return;
> > + }
> > + transport_handle_cdb_direct(se_cmd);
> > +}
> > +
> > +static void vhost_scsi_handle_vq(struct vhost_scsi *vs)
> > +{
> > + struct vhost_virtqueue *vq = &vs->vqs[2];
> > + struct virtio_scsi_cmd_req v_req;
> > + struct tcm_vhost_tpg *tv_tpg;
> > + struct tcm_vhost_cmd *tv_cmd;
> > + u32 exp_data_len, data_first, data_num, data_direction;
> > + unsigned out, in, i;
> > + int head, ret;
> > +
> > + /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
> > + tv_tpg = vs->vs_tpg;
> > + if (unlikely(!tv_tpg)) {
> > + pr_err("%s endpoint not set\n", __func__);
> > + return;
> > + }
> > +
> > + mutex_lock(&vq->mutex);
> > + vhost_disable_notify(&vs->dev, vq);
> > +
> > + for (;;) {
> > + head = vhost_get_vq_desc(&vs->dev, vq, vq->iov,
> > + ARRAY_SIZE(vq->iov), &out, &in,
> > + NULL, NULL);
> > + pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n",
> > + head, out, in);
> > + /* On error, stop handling until the next kick. */
> > + if (unlikely(head < 0))
> > + break;
> > + /* Nothing new? Wait for eventfd to tell us they refilled. */
> > + if (head == vq->num) {
> > + if (unlikely(vhost_enable_notify(&vs->dev, vq))) {
> > + vhost_disable_notify(&vs->dev, vq);
> > + continue;
> > + }
> > + break;
> > + }
> > +
> > +/* FIXME: BIDI operation */
> > + if (out == 1 && in == 1) {
> > + data_direction = DMA_NONE;
> > + data_first = 0;
> > + data_num = 0;
> > + } else if (out == 1 && in > 1) {
> > + data_direction = DMA_FROM_DEVICE;
> > + data_first = out + 1;
> > + data_num = in - 1;
> > + } else if (out > 1 && in == 1) {
> > + data_direction = DMA_TO_DEVICE;
> > + data_first = 1;
> > + data_num = out - 1;
> > + } else {
> > + vq_err(vq, "Invalid buffer layout out: %u in: %u\n",
> > + out, in);
> > + break;
> > + }
> > +
> > + /*
> > + * Check for a sane resp buffer so we can report errors to
> > + * the guest.
> > + */
> > + if (unlikely(vq->iov[out].iov_len !=
> > + sizeof(struct virtio_scsi_cmd_resp))) {
> > + vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
> > + " bytes\n", vq->iov[out].iov_len);
> > + break;
> > + }
> > +
> > + if (unlikely(vq->iov[0].iov_len != sizeof(v_req))) {
> > + vq_err(vq, "Expecting virtio_scsi_cmd_req, got %zu"
> > + " bytes\n", vq->iov[0].iov_len);
> > + break;
> > + }
> > + pr_debug("Calling __copy_from_user: vq->iov[0].iov_base: %p,"
> > + " len: %zu\n", vq->iov[0].iov_base, sizeof(v_req));
> > + ret = __copy_from_user(&v_req, vq->iov[0].iov_base,
> > + sizeof(v_req));
> > + if (unlikely(ret)) {
> > + vq_err(vq, "Faulted on virtio_scsi_cmd_req\n");
> > + break;
> > + }
> > +
> > + exp_data_len = 0;
> > + for (i = 0; i < data_num; i++)
> > + exp_data_len += vq->iov[data_first + i].iov_len;
> > +
> > + tv_cmd = vhost_scsi_allocate_cmd(tv_tpg, &v_req,
> > + exp_data_len, data_direction);
> > + if (IS_ERR(tv_cmd)) {
> > + vq_err(vq, "vhost_scsi_allocate_cmd failed %ld\n",
> > + PTR_ERR(tv_cmd));
> > + break;
> > + }
> > + pr_debug("Allocated tv_cmd: %p exp_data_len: %d, data_direction"
> > + ": %d\n", tv_cmd, exp_data_len, data_direction);
> > +
> > + tv_cmd->tvc_vhost = vs;
> > +
> > + if (unlikely(vq->iov[out].iov_len !=
> > + sizeof(struct virtio_scsi_cmd_resp))) {
> > + vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
> > + " bytes, out: %d, in: %d\n",
> > + vq->iov[out].iov_len, out, in);
> > + break;
> > + }
> > +
> > + tv_cmd->tvc_resp = vq->iov[out].iov_base;
> > +
> > + /*
> > + * Copy in the recieved CDB descriptor into tv_cmd->tvc_cdb
> > + * that will be used by tcm_vhost_new_cmd_map() and down into
> > + * target_setup_cmd_from_cdb()
> > + */
> > + memcpy(tv_cmd->tvc_cdb, v_req.cdb, TCM_VHOST_MAX_CDB_SIZE);
> > + /*
> > + * Check that the recieved CDB size does not exceeded our
> > + * hardcoded max for tcm_vhost
> > + */
> > + /* TODO what if cdb was too small for varlen cdb header? */
> > + if (unlikely(scsi_command_size(tv_cmd->tvc_cdb) >
> > + TCM_VHOST_MAX_CDB_SIZE)) {
> > + vq_err(vq, "Received SCSI CDB with command_size: %d that"
> > + " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
> > + scsi_command_size(tv_cmd->tvc_cdb),
> > + TCM_VHOST_MAX_CDB_SIZE);
> > + break; /* TODO */
> > + }
> > + tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
> > +
> > + pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
> > + tv_cmd->tvc_cdb[0], tv_cmd->tvc_lun);
> > +
> > + if (data_direction != DMA_NONE) {
> > + ret = vhost_scsi_map_iov_to_sgl(tv_cmd,
> > + &vq->iov[data_first], data_num,
> > + data_direction == DMA_TO_DEVICE);
> > + if (unlikely(ret)) {
> > + vq_err(vq, "Failed to map iov to sgl\n");
> > + break; /* TODO */
> > + }
> > + }
> > +
> > + /*
> > + * Save the descriptor from vhost_get_vq_desc() to be used to
> > + * complete the virtio-scsi request in TCM callback context via
> > + * tcm_vhost_queue_data_in() and tcm_vhost_queue_status()
> > + */
> > + tv_cmd->tvc_vq_desc = head;
> > + /*
> > + * Dispatch tv_cmd descriptor for cmwq execution in process
> > + * context provided by tcm_vhost_workqueue. This also ensures
> > + * tv_cmd is executed on the same kworker CPU as this vhost
> > + * thread to gain positive L2 cache locality effects..
> > + */
> > + INIT_WORK(&tv_cmd->work, tcm_vhost_submission_work);
> > + queue_work(tcm_vhost_workqueue, &tv_cmd->work);
> > + }
> > +
> > + mutex_unlock(&vq->mutex);
> > +}
> > +
> > +static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
> > +{
> > + pr_err("%s: The handling func for control queue.\n", __func__);
> > +}
> > +
> > +static void vhost_scsi_evt_handle_kick(struct vhost_work *work)
> > +{
> > + pr_err("%s: The handling func for event queue.\n", __func__);
> > +}
> > +
>
> can userspace still trigger these? If not pls add a comment explaining
> why are above never triggering.
>
Yes, they are both triggered via userspace once as the virtio-scsi
enabled guest boots.
Also, changing these from pr_err -> pr_debug..
> > +static void vhost_scsi_handle_kick(struct vhost_work *work)
> > +{
> > + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> > + poll.work);
> > + struct vhost_scsi *vs = container_of(vq->dev, struct vhost_scsi, dev);
> > +
> > + vhost_scsi_handle_vq(vs);
> > +}
> > +
> > +/*
> > + * Called from vhost_scsi_ioctl() context to walk the list of available
> > + * tcm_vhost_tpg with an active struct tcm_vhost_nexus
> > + */
> > +static int vhost_scsi_set_endpoint(
> > + struct vhost_scsi *vs,
> > + struct vhost_scsi_target *t)
> > +{
> > + struct tcm_vhost_tport *tv_tport;
> > + struct tcm_vhost_tpg *tv_tpg;
> > + int index;
> > +
> > + mutex_lock(&vs->dev.mutex);
> > + /* Verify that ring has been setup correctly. */
> > + for (index = 0; index < vs->dev.nvqs; ++index) {
> > + /* Verify that ring has been setup correctly. */
> > + if (!vhost_vq_access_ok(&vs->vqs[index])) {
> > + mutex_unlock(&vs->dev.mutex);
> > + return -EFAULT;
> > + }
> > + }
> > +
> > + if (vs->vs_tpg) {
> > + mutex_unlock(&vs->dev.mutex);
> > + return -EEXIST;
> > + }
> > + mutex_unlock(&vs->dev.mutex);
>
> vs->vs_tpg can change at this point and be != NULL in loop below. Not an issue?
>
Mmmm, your right, and I don't think it is correct..
I think the right thing is to moving the 'if (vs->vs_tpg)' check down in
the vs->dev.mutex protected area before doing the vs->vs_tpg = tv_tpg
assignment..
> > + mutex_lock(&tcm_vhost_mutex);
> > + list_for_each_entry(tv_tpg, &tcm_vhost_list, tv_tpg_list) {
>
> If you move this loop to a function, then it can use goto to cleanup on error.
>
I've just inlined the one error case here in the loop..
> > + mutex_lock(&tv_tpg->tv_tpg_mutex);
> > + if (!tv_tpg->tpg_nexus) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + continue;
> > + }
> > + if (atomic_read(&tv_tpg->tv_tpg_vhost_count)) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + continue;
> > + }
> > + tv_tport = tv_tpg->tport;
> > +
> > + if (!strcmp(tv_tport->tport_name, t->vhost_wwpn) &&
> > + (tv_tpg->tport_tpgt == t->vhost_tpgt)) {
> > + atomic_inc(&tv_tpg->tv_tpg_vhost_count);
> > + smp_mb__after_atomic_inc();
>
> this an other uses of memory barriers need a comment: what are we
> flushing out and why. Also - could something like kref be used for
> reference counting? If this is not datapath this might be better.
>
Actually, ->tpg_vhost_count is always protected by ->tv_tpg_mutex, so
for this case it's unnecessary.. Changing this to a non atomic_t type
now..
Doing the same for ->tv_tpg_port_count, and obtaining/release
->tv_tpg_mutex in tcm_vhost_port_*link() as well..
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + mutex_unlock(&tcm_vhost_mutex);
> > +
> > + mutex_lock(&vs->dev.mutex);
> > + vs->vs_tpg = tv_tpg;
> > + atomic_inc(&vs->vhost_ref_cnt);
Also, vhost_scsi->vhost_ref_cnt doesn't appear to be used aside from
this location (it's never decremented), and AFAICT it's not necessary.
Dropping this for now at well, unless then is an objection from Stefan
as to why it's needed..
> > + smp_mb__after_atomic_inc();
> > + mutex_unlock(&vs->dev.mutex);
> > + return 0;
> > + }
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + }
> > + mutex_unlock(&tcm_vhost_mutex);
> > + return -EINVAL;
> > +}
> > +
> > +static int vhost_scsi_clear_endpoint(
> > + struct vhost_scsi *vs,
> > + struct vhost_scsi_target *t)
> > +{
> > + struct tcm_vhost_tport *tv_tport;
> > + struct tcm_vhost_tpg *tv_tpg;
> > + int index;
> > +
> > + mutex_lock(&vs->dev.mutex);
> > + /* Verify that ring has been setup correctly. */
> > + for (index = 0; index < vs->dev.nvqs; ++index) {
> > + if (!vhost_vq_access_ok(&vs->vqs[index])) {
> > + mutex_unlock(&vs->dev.mutex);
> > + return -EFAULT;
> > + }
> > + }
> > +
> > + if (!vs->vs_tpg) {
> > + mutex_unlock(&vs->dev.mutex);
> > + return -ENODEV;
> > + }
> > + tv_tpg = vs->vs_tpg;
> > + tv_tport = tv_tpg->tport;
> > +
> > + if (strcmp(tv_tport->tport_name, t->vhost_wwpn) ||
> > + (tv_tpg->tport_tpgt != t->vhost_tpgt)) {
> > + mutex_unlock(&vs->dev.mutex);
> > + pr_warn("tv_tport->tport_name: %s, tv_tpg->tport_tpgt: %hu"
> > + " does not match t->vhost_wwpn: %s, t->vhost_tpgt: %hu\n",
> > + tv_tport->tport_name, tv_tpg->tport_tpgt,
> > + t->vhost_wwpn, t->vhost_tpgt);
> > + return -EINVAL;
> > + }
> > + atomic_dec(&tv_tpg->tv_tpg_vhost_count);
> > + vs->vs_tpg = NULL;
> > + mutex_unlock(&vs->dev.mutex);
> > +
> > + return 0;
>
> We could do it like this:
>
> err:
> mutex_unlock(&vs->dev.mutex);
> return ret;
>
> to make it clear we do not exit this with lock taken
>
<nod>, Added err: label for vhost_scsi_clear_endpoint error path
> > +}
> > +
> > +static int vhost_scsi_open(struct inode *inode, struct file *f)
> > +{
> > + struct vhost_scsi *s;
> > + int r;
> > +
> > + s = kzalloc(sizeof(*s), GFP_KERNEL);
> > + if (!s)
> > + return -ENOMEM;
> > +
> > + vhost_work_init(&s->vs_completion_work, vhost_scsi_complete_cmd_work);
> > + INIT_LIST_HEAD(&s->vs_completion_list);
> > + spin_lock_init(&s->vs_completion_lock);
> > +
> > + s->vqs[0].handle_kick = vhost_scsi_ctl_handle_kick;
> > + s->vqs[1].handle_kick = vhost_scsi_evt_handle_kick;
> > + s->vqs[2].handle_kick = vhost_scsi_handle_kick;
>
> better to add an enum with VQ numbers.
>
<nod>, adding the following + updated usage in vhost_scsi_open()
enum {
VHOST_SCSI_VQ_CTL = 0,
VHOST_SCSI_VQ_EVT = 1,
VHOST_SCSI_VQ_IO = 2,
};
> > + r = vhost_dev_init(&s->dev, s->vqs, 3);
> > + if (r < 0) {
> > + kfree(s);
> > + return r;
> > + }
> > +
> > + f->private_data = s;
> > + return 0;
> > +}
> > +
> > +static int vhost_scsi_release(struct inode *inode, struct file *f)
> > +{
> > + struct vhost_scsi *s = f->private_data;
> > +
> > + if (s->vs_tpg && s->vs_tpg->tport) {
> > + struct vhost_scsi_target backend;
> > +
> > + memcpy(backend.vhost_wwpn, s->vs_tpg->tport->tport_name,
> > + sizeof(backend.vhost_wwpn));
> > + backend.vhost_tpgt = s->vs_tpg->tport_tpgt;
> > + vhost_scsi_clear_endpoint(s, &backend);
> > + }
> > +
> > + vhost_dev_cleanup(&s->dev, false);
> > + kfree(s);
> > + return 0;
> > +}
> > +
> > +static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> > +{
> > + if (features & ~VHOST_FEATURES)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&vs->dev.mutex);
> > + if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > + !vhost_log_access_ok(&vs->dev)) {
> > + mutex_unlock(&vs->dev.mutex);
> > + return -EFAULT;
> > + }
> > + vs->dev.acked_features = features;
> > + /* TODO possibly smp_wmb() and flush vqs */
>
> Either that or fail if already running.
> Current userspace does not change features once set,
> but it's cleaner not to make assumptions.
>
Ok, following what drivers/vhost/net.c does, how about the following..?
Add the basic vhost_scsi_flush() primative:
static void vhost_scsi_flush_vq(struct vhost_scsi *vs, int index)
{
vhost_poll_flush(&vs->dev.vqs[index].poll);
}
static void vhost_scsi_flush(struct vhost_scsi *s)
{
vhost_scsi_flush_vq(s, VHOST_SCSI_VQ_CTL);
vhost_scsi_flush_vq(s, VHOST_SCSI_VQ_EVT);
vhost_scsi_flush_vq(s, VHOST_SCSI_VQ_IO);
}
And update the set_features:
@@ -967,7 +980,8 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
return -EFAULT;
}
vs->dev.acked_features = features;
- /* TODO possibly smp_wmb() and flush vqs */
+ smp_wmb();
+ vhost_scsi_flush(vs);
mutex_unlock(&vs->dev.mutex);
return 0;
}
> > + mutex_unlock(&vs->dev.mutex);
> > + return 0;
> > +}
> > +
> > +static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
> > + unsigned long arg)
> > +{
> > + struct vhost_scsi *vs = f->private_data;
> > + struct vhost_scsi_target backend;
> > + void __user *argp = (void __user *)arg;
> > + u64 __user *featurep = argp;
> > + u64 features;
> > + int r;
> > +
> > + switch (ioctl) {
> > + case VHOST_SCSI_SET_ENDPOINT:
> > + if (copy_from_user(&backend, argp, sizeof backend))
> > + return -EFAULT;
> > +
> > + return vhost_scsi_set_endpoint(vs, &backend);
> > + case VHOST_SCSI_CLEAR_ENDPOINT:
> > + if (copy_from_user(&backend, argp, sizeof backend))
> > + return -EFAULT;
> > +
> > + return vhost_scsi_clear_endpoint(vs, &backend);
> > + case VHOST_SCSI_GET_ABI_VERSION:
> > + if (copy_from_user(&backend, argp, sizeof backend))
> > + return -EFAULT;
>
> Why are we reading this in BTW?
>
Dropping the unnecessary copy_from_user() usage here..
> > +
> > + backend.abi_version = VHOST_SCSI_ABI_VERSION;
> > +
> > + if (copy_to_user(argp, &backend, sizeof backend))
> > + return -EFAULT;
> > + return 0;
> > + case VHOST_GET_FEATURES:
> > + features = VHOST_FEATURES;
> > + if (copy_to_user(featurep, &features, sizeof features))
> > + return -EFAULT;
> > + return 0;
> > + case VHOST_SET_FEATURES:
> > + if (copy_from_user(&features, featurep, sizeof features))
> > + return -EFAULT;
> > + return vhost_scsi_set_features(vs, features);
> > + default:
> > + mutex_lock(&vs->dev.mutex);
> > + r = vhost_dev_ioctl(&vs->dev, ioctl, arg);
> > + mutex_unlock(&vs->dev.mutex);
> > + return r;
> > + }
> > +}
> > +
> > +static const struct file_operations vhost_scsi_fops = {
> > + .owner = THIS_MODULE,
> > + .release = vhost_scsi_release,
> > + .unlocked_ioctl = vhost_scsi_ioctl,
> > + /* TODO compat ioctl? */
>
> well?
>
<nod>, adding the missing compat ioctl handling now..
> > + .open = vhost_scsi_open,
> > + .llseek = noop_llseek,
> > +};
> > +
> > +static struct miscdevice vhost_scsi_misc = {
> > + MISC_DYNAMIC_MINOR,
> > + "vhost-scsi",
> > + &vhost_scsi_fops,
> > +};
> > +
> > +static int __init vhost_scsi_register(void)
> > +{
> > + return misc_register(&vhost_scsi_misc);
> > +}
> > +
> > +static int vhost_scsi_deregister(void)
> > +{
> > + return misc_deregister(&vhost_scsi_misc);
> > +}
> > +
> > +static char *tcm_vhost_dump_proto_id(struct tcm_vhost_tport *tport)
> > +{
> > + switch (tport->tport_proto_id) {
> > + case SCSI_PROTOCOL_SAS:
> > + return "SAS";
> > + case SCSI_PROTOCOL_FCP:
> > + return "FCP";
> > + case SCSI_PROTOCOL_ISCSI:
> > + return "iSCSI";
> > + default:
> > + break;
> > + }
> > +
> > + return "Unknown";
> > +}
> > +
> > +static int tcm_vhost_port_link(
> > + struct se_portal_group *se_tpg,
> > + struct se_lun *lun)
>
> I'd like this and other places to be like this:
> static int tcm_vhost_port_link(struct se_portal_group *se_tpg,
> struct se_lun *lun)
>
> or
> static int
> tcm_vhost_port_link(struct se_portal_group *se_tpg,
> struct se_lun *lun)
>
> so that it's more like the rest of vhost code.
>
<nod>, doing the former for all of the places that do not exceed 80
characters..
>
> > +{
> > + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > +
> > + atomic_inc(&tv_tpg->tv_tpg_port_count);
> > + smp_mb__after_atomic_inc();
> > +
> > + return 0;
> > +}
> > +
> > +static void tcm_vhost_port_unlink(
> > + struct se_portal_group *se_tpg,
> > + struct se_lun *se_lun)
> > +{
> > + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > +
> > + atomic_dec(&tv_tpg->tv_tpg_port_count);
> > + smp_mb__after_atomic_dec();
> > +}
> > +
> > +static struct se_node_acl *tcm_vhost_make_nodeacl(
> > + struct se_portal_group *se_tpg,
> > + struct config_group *group,
> > + const char *name)
> > +{
> > + struct se_node_acl *se_nacl, *se_nacl_new;
> > + struct tcm_vhost_nacl *nacl;
> > + u64 wwpn = 0;
> > + u32 nexus_depth;
> > +
> > + /* tcm_vhost_parse_wwn(name, &wwpn, 1) < 0)
> > + return ERR_PTR(-EINVAL); */
> > + se_nacl_new = tcm_vhost_alloc_fabric_acl(se_tpg);
> > + if (!se_nacl_new)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + nexus_depth = 1;
> > + /*
> > + * se_nacl_new may be released by core_tpg_add_initiator_node_acl()
> > + * when converting a NodeACL from demo mode -> explict
> > + */
> > + se_nacl = core_tpg_add_initiator_node_acl(se_tpg, se_nacl_new,
> > + name, nexus_depth);
> > + if (IS_ERR(se_nacl)) {
> > + tcm_vhost_release_fabric_acl(se_tpg, se_nacl_new);
> > + return se_nacl;
> > + }
> > + /*
> > + * Locate our struct tcm_vhost_nacl and set the FC Nport WWPN
> > + */
> > + nacl = container_of(se_nacl, struct tcm_vhost_nacl, se_node_acl);
> > + nacl->iport_wwpn = wwpn;
> > +
> > + return se_nacl;
> > +}
> > +
> > +static void tcm_vhost_drop_nodeacl(struct se_node_acl *se_acl)
> > +{
> > + struct tcm_vhost_nacl *nacl = container_of(se_acl,
> > + struct tcm_vhost_nacl, se_node_acl);
> > + core_tpg_del_initiator_node_acl(se_acl->se_tpg, se_acl, 1);
> > + kfree(nacl);
> > +}
> > +
> > +static int tcm_vhost_make_nexus(
> > + struct tcm_vhost_tpg *tv_tpg,
> > + const char *name)
> > +{
> > + struct se_portal_group *se_tpg;
> > + struct tcm_vhost_nexus *tv_nexus;
> > +
> > + mutex_lock(&tv_tpg->tv_tpg_mutex);
> > + if (tv_tpg->tpg_nexus) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + pr_debug("tv_tpg->tpg_nexus already exists\n");
> > + return -EEXIST;
> > + }
> > + se_tpg = &tv_tpg->se_tpg;
> > +
> > + tv_nexus = kzalloc(sizeof(struct tcm_vhost_nexus), GFP_KERNEL);
> > + if (!tv_nexus) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + pr_err("Unable to allocate struct tcm_vhost_nexus\n");
> > + return -ENOMEM;
> > + }
> > + /*
> > + * Initialize the struct se_session pointer
> > + */
> > + tv_nexus->tvn_se_sess = transport_init_session();
> > + if (IS_ERR(tv_nexus->tvn_se_sess)) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + kfree(tv_nexus);
> > + return -ENOMEM;
> > + }
> > + /*
> > + * Since we are running in 'demo mode' this call with generate a
> > + * struct se_node_acl for the tcm_vhost struct se_portal_group with
> > + * the SCSI Initiator port name of the passed configfs group 'name'.
> > + */
> > + tv_nexus->tvn_se_sess->se_node_acl = core_tpg_check_initiator_node_acl(
> > + se_tpg, (unsigned char *)name);
> > + if (!tv_nexus->tvn_se_sess->se_node_acl) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + pr_debug("core_tpg_check_initiator_node_acl() failed"
> > + " for %s\n", name);
> > + transport_free_session(tv_nexus->tvn_se_sess);
> > + kfree(tv_nexus);
> > + return -ENOMEM;
> > + }
> > + /*
> > + * Now register the TCM vHost virtual I_T Nexus as active with the
>
> I think s/vHost/vhost/ would be nice :)
>
Done
> > + * call to __transport_register_session()
> > + */
> > + __transport_register_session(se_tpg, tv_nexus->tvn_se_sess->se_node_acl,
> > + tv_nexus->tvn_se_sess, tv_nexus);
> > + tv_tpg->tpg_nexus = tv_nexus;
> > +
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + return 0;
> > +}
> > +
> > +static int tcm_vhost_drop_nexus(
> > + struct tcm_vhost_tpg *tpg)
> > +{
> > + struct se_session *se_sess;
> > + struct tcm_vhost_nexus *tv_nexus;
> > +
> > + mutex_lock(&tpg->tv_tpg_mutex);
> > + tv_nexus = tpg->tpg_nexus;
> > + if (!tv_nexus) {
> > + mutex_unlock(&tpg->tv_tpg_mutex);
> > + return -ENODEV;
> > + }
> > +
> > + se_sess = tv_nexus->tvn_se_sess;
> > + if (!se_sess) {
> > + mutex_unlock(&tpg->tv_tpg_mutex);
> > + return -ENODEV;
> > + }
> > +
> > + if (atomic_read(&tpg->tv_tpg_port_count)) {
> > + mutex_unlock(&tpg->tv_tpg_mutex);
> > + pr_err("Unable to remove TCM_vHost I_T Nexus with"
> > + " active TPG port count: %d\n",
> > + atomic_read(&tpg->tv_tpg_port_count));
> > + return -EPERM;
>
> EBUSY?
>
>
Fixed
> > + }
> > +
> > + if (atomic_read(&tpg->tv_tpg_vhost_count)) {
> > + mutex_unlock(&tpg->tv_tpg_mutex);
> > + pr_err("Unable to remove TCM_vHost I_T Nexus with"
> > + " active TPG vhost count: %d\n",
> > + atomic_read(&tpg->tv_tpg_vhost_count));
> > + return -EPERM;
>
> Same?
>
Fixed
> > + }
> > +
> > + pr_debug("TCM_vHost_ConfigFS: Removing I_T Nexus to emulated"
> > + " %s Initiator Port: %s\n", tcm_vhost_dump_proto_id(tpg->tport),
> > + tv_nexus->tvn_se_sess->se_node_acl->initiatorname);
> > + /*
> > + * Release the SCSI I_T Nexus to the emulated vHost Target Port
> > + */
> > + transport_deregister_session(tv_nexus->tvn_se_sess);
> > + tpg->tpg_nexus = NULL;
> > + mutex_unlock(&tpg->tv_tpg_mutex);
> > +
> > + kfree(tv_nexus);
> > + return 0;
> > +}
> > +
> > +static ssize_t tcm_vhost_tpg_show_nexus(
> > + struct se_portal_group *se_tpg,
> > + char *page)
> > +{
> > + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_nexus *tv_nexus;
> > + ssize_t ret;
> > +
> > + mutex_lock(&tv_tpg->tv_tpg_mutex);
> > + tv_nexus = tv_tpg->tpg_nexus;
> > + if (!tv_nexus) {
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > + return -ENODEV;
> > + }
> > + ret = snprintf(page, PAGE_SIZE, "%s\n",
> > + tv_nexus->tvn_se_sess->se_node_acl->initiatorname);
> > + mutex_unlock(&tv_tpg->tv_tpg_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t tcm_vhost_tpg_store_nexus(
> > + struct se_portal_group *se_tpg,
> > + const char *page,
> > + size_t count)
> > +{
> > + struct tcm_vhost_tpg *tv_tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > + struct tcm_vhost_tport *tport_wwn = tv_tpg->tport;
> > + unsigned char i_port[TCM_VHOST_NAMELEN], *ptr, *port_ptr;
> > + int ret;
> > + /*
> > + * Shutdown the active I_T nexus if 'NULL' is passed..
> > + */
> > + if (!strncmp(page, "NULL", 4)) {
> > + ret = tcm_vhost_drop_nexus(tv_tpg);
> > + return (!ret) ? count : ret;
> > + }
> > + /*
> > + * Otherwise make sure the passed virtual Initiator port WWN matches
> > + * the fabric protocol_id set in tcm_vhost_make_tport(), and call
> > + * tcm_vhost_make_nexus().
> > + */
> > + if (strlen(page) >= TCM_VHOST_NAMELEN) {
> > + pr_err("Emulated NAA Sas Address: %s, exceeds"
> > + " max: %d\n", page, TCM_VHOST_NAMELEN);
> > + return -EINVAL;
> > + }
> > + snprintf(&i_port[0], TCM_VHOST_NAMELEN, "%s", page);
> > +
> > + ptr = strstr(i_port, "naa.");
> > + if (ptr) {
> > + if (tport_wwn->tport_proto_id != SCSI_PROTOCOL_SAS) {
> > + pr_err("Passed SAS Initiator Port %s does not"
> > + " match target port protoid: %s\n", i_port,
> > + tcm_vhost_dump_proto_id(tport_wwn));
> > + return -EINVAL;
> > + }
> > + port_ptr = &i_port[0];
> > + goto check_newline;
> > + }
> > + ptr = strstr(i_port, "fc.");
> > + if (ptr) {
> > + if (tport_wwn->tport_proto_id != SCSI_PROTOCOL_FCP) {
> > + pr_err("Passed FCP Initiator Port %s does not"
> > + " match target port protoid: %s\n", i_port,
> > + tcm_vhost_dump_proto_id(tport_wwn));
> > + return -EINVAL;
> > + }
> > + port_ptr = &i_port[3]; /* Skip over "fc." */
> > + goto check_newline;
> > + }
> > + ptr = strstr(i_port, "iqn.");
> > + if (ptr) {
> > + if (tport_wwn->tport_proto_id != SCSI_PROTOCOL_ISCSI) {
> > + pr_err("Passed iSCSI Initiator Port %s does not"
> > + " match target port protoid: %s\n", i_port,
> > + tcm_vhost_dump_proto_id(tport_wwn));
> > + return -EINVAL;
> > + }
> > + port_ptr = &i_port[0];
> > + goto check_newline;
> > + }
> > + pr_err("Unable to locate prefix for emulated Initiator Port:"
> > + " %s\n", i_port);
> > + return -EINVAL;
> > + /*
> > + * Clear any trailing newline for the NAA WWN
> > + */
> > +check_newline:
> > + if (i_port[strlen(i_port)-1] == '\n')
> > + i_port[strlen(i_port)-1] = '\0';
> > +
> > + ret = tcm_vhost_make_nexus(tv_tpg, port_ptr);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return count;
> > +}
> > +
> > +TF_TPG_BASE_ATTR(tcm_vhost, nexus, S_IRUGO | S_IWUSR);
> > +
> > +static struct configfs_attribute *tcm_vhost_tpg_attrs[] = {
> > + &tcm_vhost_tpg_nexus.attr,
> > + NULL,
> > +};
> > +
> > +static struct se_portal_group *tcm_vhost_make_tpg(
> > + struct se_wwn *wwn,
> > + struct config_group *group,
> > + const char *name)
> > +{
> > + struct tcm_vhost_tport *tport = container_of(wwn,
> > + struct tcm_vhost_tport, tport_wwn);
> > +
> > + struct tcm_vhost_tpg *tpg;
> > + unsigned long tpgt;
> > + int ret;
> > +
> > + if (strstr(name, "tpgt_") != name)
> > + return ERR_PTR(-EINVAL);
> > + if (kstrtoul(name + 5, 10, &tpgt) || tpgt > UINT_MAX)
> > + return ERR_PTR(-EINVAL);
> > +
> > + tpg = kzalloc(sizeof(struct tcm_vhost_tpg), GFP_KERNEL);
> > + if (!tpg) {
> > + pr_err("Unable to allocate struct tcm_vhost_tpg");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + mutex_init(&tpg->tv_tpg_mutex);
> > + INIT_LIST_HEAD(&tpg->tv_tpg_list);
> > + tpg->tport = tport;
> > + tpg->tport_tpgt = tpgt;
> > +
> > + ret = core_tpg_register(&tcm_vhost_fabric_configfs->tf_ops, wwn,
> > + &tpg->se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
> > + if (ret < 0) {
> > + kfree(tpg);
> > + return NULL;
> > + }
> > + mutex_lock(&tcm_vhost_mutex);
> > + list_add_tail(&tpg->tv_tpg_list, &tcm_vhost_list);
> > + mutex_unlock(&tcm_vhost_mutex);
> > +
> > + return &tpg->se_tpg;
> > +}
> > +
> > +static void tcm_vhost_drop_tpg(struct se_portal_group *se_tpg)
> > +{
> > + struct tcm_vhost_tpg *tpg = container_of(se_tpg,
> > + struct tcm_vhost_tpg, se_tpg);
> > +
> > + mutex_lock(&tcm_vhost_mutex);
> > + list_del(&tpg->tv_tpg_list);
> > + mutex_unlock(&tcm_vhost_mutex);
> > + /*
> > + * Release the virtual I_T Nexus for this vHost TPG
> > + */
> > + tcm_vhost_drop_nexus(tpg);
> > + /*
> > + * Deregister the se_tpg from TCM..
> > + */
> > + core_tpg_deregister(se_tpg);
> > + kfree(tpg);
> > +}
> > +
> > +static struct se_wwn *tcm_vhost_make_tport(
> > + struct target_fabric_configfs *tf,
> > + struct config_group *group,
> > + const char *name)
> > +{
> > + struct tcm_vhost_tport *tport;
> > + char *ptr;
> > + u64 wwpn = 0;
> > + int off = 0;
> > +
> > + /* if (tcm_vhost_parse_wwn(name, &wwpn, 1) < 0)
> > + return ERR_PTR(-EINVAL); */
> > +
> > + tport = kzalloc(sizeof(struct tcm_vhost_tport), GFP_KERNEL);
> > + if (!tport) {
> > + pr_err("Unable to allocate struct tcm_vhost_tport");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + tport->tport_wwpn = wwpn;
> > + /*
> > + * Determine the emulated Protocol Identifier and Target Port Name
> > + * based on the incoming configfs directory name.
> > + */
> > + ptr = strstr(name, "naa.");
> > + if (ptr) {
> > + tport->tport_proto_id = SCSI_PROTOCOL_SAS;
> > + goto check_len;
> > + }
> > + ptr = strstr(name, "fc.");
> > + if (ptr) {
> > + tport->tport_proto_id = SCSI_PROTOCOL_FCP;
> > + off = 3; /* Skip over "fc." */
> > + goto check_len;
> > + }
> > + ptr = strstr(name, "iqn.");
> > + if (ptr) {
> > + tport->tport_proto_id = SCSI_PROTOCOL_ISCSI;
> > + goto check_len;
> > + }
> > +
> > + pr_err("Unable to locate prefix for emulated Target Port:"
> > + " %s\n", name);
> > + kfree(tport);
> > + return ERR_PTR(-EINVAL);
> > +
> > +check_len:
> > + if (strlen(name) >= TCM_VHOST_NAMELEN) {
> > + pr_err("Emulated %s Address: %s, exceeds"
> > + " max: %d\n", name, tcm_vhost_dump_proto_id(tport),
> > + TCM_VHOST_NAMELEN);
> > + kfree(tport);
> > + return ERR_PTR(-EINVAL);
> > + }
> > + snprintf(&tport->tport_name[0], TCM_VHOST_NAMELEN, "%s", &name[off]);
> > +
> > + pr_debug("TCM_VHost_ConfigFS: Allocated emulated Target"
> > + " %s Address: %s\n", tcm_vhost_dump_proto_id(tport), name);
> > +
> > + return &tport->tport_wwn;
> > +}
> > +
> > +static void tcm_vhost_drop_tport(struct se_wwn *wwn)
> > +{
> > + struct tcm_vhost_tport *tport = container_of(wwn,
> > + struct tcm_vhost_tport, tport_wwn);
> > +
> > + pr_debug("TCM_VHost_ConfigFS: Deallocating emulated Target"
> > + " %s Address: %s\n", tcm_vhost_dump_proto_id(tport),
> > + tport->tport_name);
> > +
> > + kfree(tport);
> > +}
> > +
> > +static ssize_t tcm_vhost_wwn_show_attr_version(
> > + struct target_fabric_configfs *tf,
> > + char *page)
> > +{
> > + return sprintf(page, "TCM_VHOST fabric module %s on %s/%s"
> > + "on "UTS_RELEASE"\n", TCM_VHOST_VERSION, utsname()->sysname,
> > + utsname()->machine);
> > +}
> > +
> > +TF_WWN_ATTR_RO(tcm_vhost, version);
> > +
> > +static struct configfs_attribute *tcm_vhost_wwn_attrs[] = {
> > + &tcm_vhost_wwn_version.attr,
> > + NULL,
> > +};
> > +
> > +static struct target_core_fabric_ops tcm_vhost_ops = {
> > + .get_fabric_name = tcm_vhost_get_fabric_name,
> > + .get_fabric_proto_ident = tcm_vhost_get_fabric_proto_ident,
> > + .tpg_get_wwn = tcm_vhost_get_fabric_wwn,
> > + .tpg_get_tag = tcm_vhost_get_tag,
> > + .tpg_get_default_depth = tcm_vhost_get_default_depth,
> > + .tpg_get_pr_transport_id = tcm_vhost_get_pr_transport_id,
> > + .tpg_get_pr_transport_id_len = tcm_vhost_get_pr_transport_id_len,
> > + .tpg_parse_pr_out_transport_id = tcm_vhost_parse_pr_out_transport_id,
> > + .tpg_check_demo_mode = tcm_vhost_check_true,
> > + .tpg_check_demo_mode_cache = tcm_vhost_check_true,
> > + .tpg_check_demo_mode_write_protect = tcm_vhost_check_false,
> > + .tpg_check_prod_mode_write_protect = tcm_vhost_check_false,
> > + .tpg_alloc_fabric_acl = tcm_vhost_alloc_fabric_acl,
> > + .tpg_release_fabric_acl = tcm_vhost_release_fabric_acl,
> > + .tpg_get_inst_index = tcm_vhost_tpg_get_inst_index,
> > + .release_cmd = tcm_vhost_release_cmd,
> > + .shutdown_session = tcm_vhost_shutdown_session,
> > + .close_session = tcm_vhost_close_session,
> > + .sess_get_index = tcm_vhost_sess_get_index,
> > + .sess_get_initiator_sid = NULL,
> > + .write_pending = tcm_vhost_write_pending,
> > + .write_pending_status = tcm_vhost_write_pending_status,
> > + .set_default_node_attributes = tcm_vhost_set_default_node_attrs,
> > + .get_task_tag = tcm_vhost_get_task_tag,
> > + .get_cmd_state = tcm_vhost_get_cmd_state,
> > + .queue_data_in = tcm_vhost_queue_data_in,
> > + .queue_status = tcm_vhost_queue_status,
> > + .queue_tm_rsp = tcm_vhost_queue_tm_rsp,
> > + .get_fabric_sense_len = tcm_vhost_get_fabric_sense_len,
> > + .set_fabric_sense_len = tcm_vhost_set_fabric_sense_len,
> > + /*
> > + * Setup callers for generic logic in target_core_fabric_configfs.c
> > + */
> > + .fabric_make_wwn = tcm_vhost_make_tport,
> > + .fabric_drop_wwn = tcm_vhost_drop_tport,
> > + .fabric_make_tpg = tcm_vhost_make_tpg,
> > + .fabric_drop_tpg = tcm_vhost_drop_tpg,
> > + .fabric_post_link = tcm_vhost_port_link,
> > + .fabric_pre_unlink = tcm_vhost_port_unlink,
> > + .fabric_make_np = NULL,
> > + .fabric_drop_np = NULL,
> > + .fabric_make_nodeacl = tcm_vhost_make_nodeacl,
> > + .fabric_drop_nodeacl = tcm_vhost_drop_nodeacl,
> > +};
> > +
> > +static int tcm_vhost_register_configfs(void)
> > +{
> > + struct target_fabric_configfs *fabric;
> > + int ret;
> > +
> > + pr_debug("TCM_VHOST fabric module %s on %s/%s"
> > + " on "UTS_RELEASE"\n", TCM_VHOST_VERSION, utsname()->sysname,
> > + utsname()->machine);
> > + /*
> > + * Register the top level struct config_item_type with TCM core
> > + */
> > + fabric = target_fabric_configfs_init(THIS_MODULE, "vhost");
> > + if (IS_ERR(fabric)) {
> > + pr_err("target_fabric_configfs_init() failed\n");
> > + return PTR_ERR(fabric);
> > + }
> > + /*
> > + * Setup fabric->tf_ops from our local tcm_vhost_ops
> > + */
> > + fabric->tf_ops = tcm_vhost_ops;
> > + /*
> > + * Setup default attribute lists for various fabric->tf_cit_tmpl
> > + */
> > + TF_CIT_TMPL(fabric)->tfc_wwn_cit.ct_attrs = tcm_vhost_wwn_attrs;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_base_cit.ct_attrs = tcm_vhost_tpg_attrs;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_attrib_cit.ct_attrs = NULL;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_param_cit.ct_attrs = NULL;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_np_base_cit.ct_attrs = NULL;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_base_cit.ct_attrs = NULL;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_attrib_cit.ct_attrs = NULL;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_auth_cit.ct_attrs = NULL;
> > + TF_CIT_TMPL(fabric)->tfc_tpg_nacl_param_cit.ct_attrs = NULL;
> > + /*
> > + * Register the fabric for use within TCM
> > + */
> > + ret = target_fabric_configfs_register(fabric);
> > + if (ret < 0) {
> > + pr_err("target_fabric_configfs_register() failed"
> > + " for TCM_VHOST\n");
> > + return ret;
> > + }
> > + /*
> > + * Setup our local pointer to *fabric
> > + */
> > + tcm_vhost_fabric_configfs = fabric;
> > + pr_debug("TCM_VHOST[0] - Set fabric -> tcm_vhost_fabric_configfs\n");
> > + return 0;
> > +};
> > +
> > +static void tcm_vhost_deregister_configfs(void)
> > +{
> > + if (!tcm_vhost_fabric_configfs)
> > + return;
> > +
> > + target_fabric_configfs_deregister(tcm_vhost_fabric_configfs);
> > + tcm_vhost_fabric_configfs = NULL;
> > + pr_debug("TCM_VHOST[0] - Cleared tcm_vhost_fabric_configfs\n");
> > +};
> > +
> > +static int __init tcm_vhost_init(void)
> > +{
> > + int ret = -ENOMEM;
> > +
> > + tcm_vhost_workqueue = alloc_workqueue("tcm_vhost", 0, 0);
> > + if (!tcm_vhost_workqueue)
> > + goto out;
>
> Pls add a comment explaining why we need our own workqueue
> and aren't using the system one instead.
>
<nod>, loopback + tcm_qla2xxx use their own dedicated workqueue(s) as
well..
> > +
> > + ret = vhost_scsi_register();
> > + if (ret < 0)
> > + goto out_destroy_workqueue;
> > +
> > + ret = tcm_vhost_register_configfs();
> > + if (ret < 0)
> > + goto out_vhost_scsi_deregister;
> > +
> > + return 0;
> > +
> > +out_vhost_scsi_deregister:
> > + vhost_scsi_deregister();
> > +out_destroy_workqueue:
> > + destroy_workqueue(tcm_vhost_workqueue);
> > +out:
> > + return ret;
> > +};
> > +
> > +static void tcm_vhost_exit(void)
> > +{
> > + tcm_vhost_deregister_configfs();
> > + vhost_scsi_deregister();
> > + destroy_workqueue(tcm_vhost_workqueue);
> > +};
> > +
> > +MODULE_DESCRIPTION("TCM_VHOST series fabric driver");
> > +MODULE_LICENSE("GPL");
> > +module_init(tcm_vhost_init);
> > +module_exit(tcm_vhost_exit);
> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > new file mode 100644
> > index 0000000..c983ed2
> > --- /dev/null
> > +++ b/drivers/vhost/tcm_vhost.h
> > @@ -0,0 +1,101 @@
> > +#define TCM_VHOST_VERSION "v0.1"
> > +#define TCM_VHOST_NAMELEN 256
> > +#define TCM_VHOST_MAX_CDB_SIZE 32
> > +
> > +struct tcm_vhost_cmd {
> > + /* Descriptor from vhost_get_vq_desc() for virt_queue segment */
> > + int tvc_vq_desc;
> > + /* The Tag from include/linux/virtio_scsi.h:struct virtio_scsi_cmd_req */
> > + u64 tvc_tag;
> > + /* The number of scatterlists associated with this cmd */
> > + u32 tvc_sgl_count;
> > + /* Saved unpacked SCSI LUN for tcm_vhost_submission_work() */
> > + u32 tvc_lun;
> > + /* Pointer to the SGL formatted memory from virtio-scsi */
> > + struct scatterlist *tvc_sgl;
> > + /* Pointer to response */
> > + struct virtio_scsi_cmd_resp __user *tvc_resp;
> > + /* Pointer to vhost_scsi for our device */
> > + struct vhost_scsi *tvc_vhost;
> > + /* The TCM I/O descriptor that is accessed via container_of() */
> > + struct se_cmd tvc_se_cmd;
> > + /* work item used for cmwq dispatch to tcm_vhost_submission_work() */
> > + struct work_struct work;
> > + /* Copy of the incoming SCSI command descriptor block (CDB) */
> > + unsigned char tvc_cdb[TCM_VHOST_MAX_CDB_SIZE];
> > + /* Sense buffer that will be mapped into outgoing status */
> > + unsigned char tvc_sense_buf[TRANSPORT_SENSE_BUFFER];
> > + /* Completed commands list, serviced from vhost worker thread */
> > + struct list_head tvc_completion_list;
> > +};
> > +
> > +struct tcm_vhost_nexus {
> > + /* Pointer to TCM session for I_T Nexus */
> > + struct se_session *tvn_se_sess;
> > +};
> > +
> > +struct tcm_vhost_nacl {
> > + /* Binary World Wide unique Port Name for Vhost Initiator port */
> > + u64 iport_wwpn;
> > + /* ASCII formatted WWPN for Sas Initiator port */
> > + char iport_name[TCM_VHOST_NAMELEN];
> > + /* Returned by tcm_vhost_make_nodeacl() */
> > + struct se_node_acl se_node_acl;
> > +};
> > +
> > +struct tcm_vhost_tpg {
> > + /* Vhost port target portal group tag for TCM */
> > + u16 tport_tpgt;
> > + /* Used to track number of TPG Port/Lun Links wrt to explict I_T Nexus shutdown */
> > + atomic_t tv_tpg_port_count;
> > + /* Used for vhost_scsi device reference to tpg_nexus */
> > + atomic_t tv_tpg_vhost_count;
> > + /* list for tcm_vhost_list */
> > + struct list_head tv_tpg_list;
> > + /* Used to protect access for tpg_nexus */
> > + struct mutex tv_tpg_mutex;
>
> would it be easier to just use dev mutex instead?
>
> > + /* Pointer to the TCM VHost I_T Nexus for this TPG endpoint */
> > + struct tcm_vhost_nexus *tpg_nexus;
> > + /* Pointer back to tcm_vhost_tport */
> > + struct tcm_vhost_tport *tport;
> > + /* Returned by tcm_vhost_make_tpg() */
> > + struct se_portal_group se_tpg;
> > +};
> > +
> > +struct tcm_vhost_tport {
> > + /* SCSI protocol the tport is providing */
> > + u8 tport_proto_id;
> > + /* Binary World Wide unique Port Name for Vhost Target port */
> > + u64 tport_wwpn;
> > + /* ASCII formatted WWPN for Vhost Target port */
> > + char tport_name[TCM_VHOST_NAMELEN];
> > + /* Returned by tcm_vhost_make_tport() */
> > + struct se_wwn tport_wwn;
> > +};
> > +
> > +/*
> > + * As per request from MST, keep TCM_VHOST related ioctl defines out of
> > + * linux/vhost.h (user-space) for now..
> > + */
> > +
> > +#include <linux/vhost.h>
> > +
> > +/*
> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > + *
> > + * ABI Rev 0: July 2012 version starting point for v3.6-rc merge candidate +
> > + * RFC-v2 vhost-scsi userspace. Add GET_ABI_VERSION ioctl usage
> > + */
> > +
> > +#define VHOST_SCSI_ABI_VERSION 0
> > +
> > +struct vhost_scsi_target {
> > + int abi_version;
> > + unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> > + unsigned short vhost_tpgt;
> > +};
> > +
> > +/* VHOST_SCSI specific defines */
> > +#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> > +#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> > +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
>
> CLEAR_ENDPOINT and SET_ENDPOINT commands get an abi version
> too but it's ignored there. Intentional?
>
Not quite..
>
> Why not
> #define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, int)
> and then you avoid changing vhost_scsi_target ...
>
Ah yes, sorry, that makes alot more sense.. Changing GET_ABI_VERSION
to return 'int abi_version' to userspace now.
(As you may have noticed, I gave up ioctls for configfs for doing
compatible control plane code w/ python code a long time ago.. ;)
>
> Also, let's add a comment like
>
> /* Changing this breaks userspace. */
>
> so we remember never to change this value.
>
<nod>
^ permalink raw reply
* Re: [RFC PATCH 4/6] ftrace: Allow stealing pages from pipe buffer
From: Steven Rostedt @ 2012-07-30 22:12 UTC (permalink / raw)
To: Yoshihiro YUNOMAE
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
yrl.pp-manager.tt, linux-kernel, Borislav Petkov, virtualization,
Masami Hiramatsu, Franch Ch. Eigler, Ingo Molnar,
Mathieu Desnoyers, Anthony Liguori, Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023738.6600.59837.stgit@ltc189.sdl.hitachi.co.jp>
On Tue, 2012-07-24 at 11:37 +0900, Yoshihiro YUNOMAE wrote:
> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>
> Use generic steal operation on pipe buffer to allow stealing
> ring buffer's read page from pipe buffer.
>
> Note that this could reduce the performance of splice on the
> splice_write side operation without affinity setting.
> Since the ring buffer's read pages are allocated on the
> tracing-node, but the splice user does not always execute
> splice write side operation on the same node. In this case,
> the page will be accessed from the another node.
> Thus, it is strongly recommended to assign the splicing
> thread to corresponding node.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> ---
>
> kernel/trace/trace.c | 8 +-------
> 1 files changed, 1 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index a120f98..ae01930 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4194,12 +4194,6 @@ static void buffer_pipe_buf_release(struct pipe_inode_info *pipe,
> buf->private = 0;
> }
>
> -static int buffer_pipe_buf_steal(struct pipe_inode_info *pipe,
> - struct pipe_buffer *buf)
> -{
> - return 1;
> -}
> -
> static void buffer_pipe_buf_get(struct pipe_inode_info *pipe,
> struct pipe_buffer *buf)
> {
> @@ -4215,7 +4209,7 @@ static const struct pipe_buf_operations buffer_pipe_buf_ops = {
> .unmap = generic_pipe_buf_unmap,
> .confirm = generic_pipe_buf_confirm,
> .release = buffer_pipe_buf_release,
> - .steal = buffer_pipe_buf_steal,
> + .steal = generic_pipe_buf_steal,
> .get = buffer_pipe_buf_get,
> };
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox