Cong Meng
2012-Aug-21 08:23 UTC
[PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
This patch adds some queue limit parameters into block drive. And inits them for sg block drive. Some interfaces are also added for accessing them. Signed-off-by: Cong Meng <mc at linux.vnet.ibm.com> --- block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 4 +++ blockdev.c | 15 +++++++++++++ hw/block-common.h | 3 ++ 4 files changed, 80 insertions(+), 0 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 0dce089..a086f89 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -53,6 +53,7 @@ #include <linux/cdrom.h> #include <linux/fd.h> #include <linux/fs.h> +#include <dirent.h> #endif #ifdef CONFIG_FIEMAP #include <linux/fiemap.h> @@ -829,6 +830,62 @@ static int hdev_probe_device(const char *filename) return 0; } +static void read_queue_limit(char *path, const char *filename, unsigned int *val) +{ + FILE *f; + char *tail = path + strlen(path); + + pstrcat(path, MAXPATHLEN, filename); + f = fopen(path, "r"); + if (!f) { + goto out; + } + + fscanf(f, "%u", val); + fclose(f); + +out: + *tail = 0; +} + +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename) +{ + DIR *ffs; + struct dirent *d; + char path[MAXPATHLEN]; + + snprintf(path, MAXPATHLEN, + "/sys/class/scsi_generic/sg%s/device/block/", + filename + strlen("/dev/sg")); + + ffs = opendir(path); + if (!ffs) { + return; + } + + for (;;) { + d = readdir(ffs); + if (!d) { + return; + } + + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) { + continue; + } + + break; + } + + closedir(ffs); + + pstrcat(path, MAXPATHLEN, d->d_name); + pstrcat(path, MAXPATHLEN, "/queue/"); + + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors); + read_queue_limit(path, "max_segments", &bs->max_segments); + read_queue_limit(path, "max_segment_size", &bs->max_segment_size); +} + static int hdev_open(BlockDriverState *bs, const char *filename, int flags) { BDRVRawState *s = bs->opaque; @@ -868,6 +925,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) temp = realpath(filename, resolved_path); if (temp && strstart(temp, "/dev/sg", NULL)) { bs->sg = 1; + sg_get_queue_limits(bs, temp); } } #endif diff --git a/block_int.h b/block_int.h index d72317f..a9d07a2 100644 --- a/block_int.h +++ b/block_int.h @@ -333,6 +333,10 @@ struct BlockDriverState { /* long-running background operation */ BlockJob *job; + + unsigned int max_sectors; + unsigned int max_segments; + unsigned int max_segment_size; }; int get_tmp_filename(char *filename, int size); diff --git a/blockdev.c b/blockdev.c index 3d75015..e17edbf 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1191,3 +1191,18 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) bdrv_iterate(do_qmp_query_block_jobs_one, &prev); return dummy.next; } + +unsigned int get_queue_max_sectors(BlockDriverState *bs) +{ + return (bs->file && bs->file->sg) ? bs->file->max_sectors : 0; +} + +unsigned int get_queue_max_segments(BlockDriverState *bs) +{ + return (bs->file && bs->file->sg) ? bs->file->max_segments : 0; +} + +unsigned int get_queue_max_segment_size(BlockDriverState *bs) +{ + return (bs->file && bs->file->sg) ? bs->file->max_segment_size : 0; +} diff --git a/hw/block-common.h b/hw/block-common.h index bb808f7..d47fcd7 100644 --- a/hw/block-common.h +++ b/hw/block-common.h @@ -76,4 +76,7 @@ void hd_geometry_guess(BlockDriverState *bs, int *ptrans); int hd_bios_chs_auto_trans(uint32_t cyls, uint32_t heads, uint32_t secs); +unsigned int get_queue_max_sectors(BlockDriverState *bs); +unsigned int get_queue_max_segments(BlockDriverState *bs); +unsigned int get_queue_max_segment_size(BlockDriverState *bs); #endif -- 1.7.7.6
Cong Meng
2012-Aug-21 08:23 UTC
[PATCH 2/2 v1] virtio-scsi: set per-LUN queue limits for sg devices
Each virtio scsi HBA has global request queue limits. But the passthrough LUNs (scsi-generic) come from different host HBAs may have different request queue limits. If the guest sends commands that exceed the host limits, the commands will be rejected by host HAB. To address the issue, this patch responses the newly added virtio control queue request by returning the per-LUN queue limits. Signed-off-by: Cong Meng <mc at linux.vnet.ibm.com> --- hw/virtio-scsi.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 65 insertions(+), 0 deletions(-) diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c index c4a5b22..3c0bd99 100644 --- a/hw/virtio-scsi.c +++ b/hw/virtio-scsi.c @@ -28,6 +28,7 @@ #define VIRTIO_SCSI_F_INOUT 0 #define VIRTIO_SCSI_F_HOTPLUG 1 #define VIRTIO_SCSI_F_CHANGE 2 +#define VIRTIO_SCSI_F_LUN_QUERY 3 /* Response codes */ #define VIRTIO_SCSI_S_OK 0 @@ -48,6 +49,7 @@ #define VIRTIO_SCSI_T_TMF 0 #define VIRTIO_SCSI_T_AN_QUERY 1 #define VIRTIO_SCSI_T_AN_SUBSCRIBE 2 +#define VIRTIO_SCSI_T_LUN_QUERY 3 /* Valid TMF subtypes. */ #define VIRTIO_SCSI_T_TMF_ABORT_TASK 0 @@ -66,6 +68,11 @@ #define VIRTIO_SCSI_T_ASYNC_NOTIFY 2 #define VIRTIO_SCSI_T_PARAM_CHANGE 3 +/* LUN Query */ +#define VIRTIO_SCSI_T_LQ_MAX_SECTORS 0 +#define VIRTIO_SCSI_T_LQ_MAX_SEGMENTS 1 +#define VIRTIO_SCSI_T_LQ_MAX_SEGMENT_SIZE 2 + /* Reasons for transport reset event */ #define VIRTIO_SCSI_EVT_RESET_HARD 0 #define VIRTIO_SCSI_EVT_RESET_RESCAN 1 @@ -115,6 +122,18 @@ typedef struct { uint8_t response; } QEMU_PACKED VirtIOSCSICtrlANResp; +/* LUN qeury */ +typedef struct { + uint32_t type; + uint8_t lun[8]; + uint32_t subtype; +} QEMU_PACKED VirtIOSCSICtrlLQReq; + +typedef struct { + uint32_t response; + uint32_t value; +} QEMU_PACKED VirtIOSCSICtrlLQResp; + typedef struct { uint32_t event; uint8_t lun[8]; @@ -160,6 +179,7 @@ typedef struct VirtIOSCSIReq { VirtIOSCSICmdReq *cmd; VirtIOSCSICtrlTMFReq *tmf; VirtIOSCSICtrlANReq *an; + VirtIOSCSICtrlLQReq *lq; } req; union { char *buf; @@ -167,6 +187,7 @@ typedef struct VirtIOSCSIReq { VirtIOSCSICtrlTMFResp *tmf; VirtIOSCSICtrlANResp *an; VirtIOSCSIEvent *event; + VirtIOSCSICtrlLQResp *lq; } resp; } VirtIOSCSIReq; @@ -285,6 +306,43 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq) return req; } +static void virtio_scsi_do_lun_query(VirtIOSCSI *s, VirtIOSCSIReq *req) +{ + SCSIDevice *d = virtio_scsi_device_find(s, req->req.lq->lun); + + if (!d) { + goto fail; + } + + switch (req->req.lq->subtype) { + case VIRTIO_SCSI_T_LQ_MAX_SECTORS: + req->resp.lq->value = get_queue_max_sectors(d->conf.bs); + if (!req->resp.lq->value) { + goto fail; + } + break; + case VIRTIO_SCSI_T_LQ_MAX_SEGMENTS: + req->resp.lq->value = get_queue_max_segments(d->conf.bs); + if (!req->resp.lq->value) { + goto fail; + } + break; + case VIRTIO_SCSI_T_LQ_MAX_SEGMENT_SIZE: + req->resp.lq->value = get_queue_max_segment_size(d->conf.bs); + if (!req->resp.lq->value) { + goto fail; + } + break; + default: + goto fail; + } + + req->resp.lq->response = VIRTIO_SCSI_S_OK; + return; +fail: + req->resp.lq->response = VIRTIO_SCSI_S_FAILURE; +} + static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) { SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf->lun); @@ -414,6 +472,12 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) } req->resp.an->event_actual = 0; req->resp.an->response = VIRTIO_SCSI_S_OK; + } else if (req->req.lq->type == VIRTIO_SCSI_T_LUN_QUERY) { + if (out_size < sizeof(VirtIOSCSICtrlLQReq) || + in_size < sizeof(VirtIOSCSICtrlLQResp)) { + virtio_scsi_bad_req(); + } + virtio_scsi_do_lun_query(s, req); } virtio_scsi_complete_req(req); } @@ -557,6 +621,7 @@ static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, { requested_features |= (1UL << VIRTIO_SCSI_F_HOTPLUG); requested_features |= (1UL << VIRTIO_SCSI_F_CHANGE); + requested_features |= (1UL << VIRTIO_SCSI_F_LUN_QUERY); return requested_features; } -- 1.7.7.6
Paolo Bonzini
2012-Aug-21 08:48 UTC
[PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
Il 21/08/2012 10:23, Cong Meng ha scritto:> +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename) > +{ > + DIR *ffs; > + struct dirent *d; > + char path[MAXPATHLEN]; > + > + snprintf(path, MAXPATHLEN, > + "/sys/class/scsi_generic/sg%s/device/block/", > + filename + strlen("/dev/sg")); > + > + ffs = opendir(path); > + if (!ffs) { > + return; > + } > + > + for (;;) { > + d = readdir(ffs); > + if (!d) { > + return; > + } > + > + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) { > + continue; > + } > + > + break; > + } > + > + closedir(ffs); > + > + pstrcat(path, MAXPATHLEN, d->d_name); > + pstrcat(path, MAXPATHLEN, "/queue/"); > + > + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors); > + read_queue_limit(path, "max_segments", &bs->max_segments); > + read_queue_limit(path, "max_segment_size", &bs->max_segment_size); > +}Using /sys/dev/block or /sys/dev/char seems easier, and lets you retrieve the parameters for block devices too. However, I'm worried of the consequences this has for migration. You could have the same physical disk accessed with two different HBAs, with different limits. So I don't know if this can really be solved at all. Paolo
Stefan Hajnoczi
2012-Aug-21 09:49 UTC
[PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
On Tue, Aug 21, 2012 at 9:23 AM, Cong Meng <mc at linux.vnet.ibm.com> wrote:> This patch adds some queue limit parameters into block drive. And inits them > for sg block drive. Some interfaces are also added for accessing them. > > Signed-off-by: Cong Meng <mc at linux.vnet.ibm.com> > --- > block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > block_int.h | 4 +++ > blockdev.c | 15 +++++++++++++ > hw/block-common.h | 3 ++ > 4 files changed, 80 insertions(+), 0 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 0dce089..a086f89 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -53,6 +53,7 @@ > #include <linux/cdrom.h> > #include <linux/fd.h> > #include <linux/fs.h> > +#include <dirent.h> > #endif > #ifdef CONFIG_FIEMAP > #include <linux/fiemap.h> > @@ -829,6 +830,62 @@ static int hdev_probe_device(const char *filename) > return 0; > } > > +static void read_queue_limit(char *path, const char *filename, unsigned int *val) > +{ > + FILE *f; > + char *tail = path + strlen(path); > + > + pstrcat(path, MAXPATHLEN, filename); > + f = fopen(path, "r"); > + if (!f) { > + goto out; > + } > + > + fscanf(f, "%u", val); > + fclose(f); > + > +out: > + *tail = 0; > +} > + > +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename) > +{ > + DIR *ffs; > + struct dirent *d; > + char path[MAXPATHLEN]; > + > + snprintf(path, MAXPATHLEN, > + "/sys/class/scsi_generic/sg%s/device/block/", > + filename + strlen("/dev/sg")); > + > + ffs = opendir(path); > + if (!ffs) { > + return; > + } > + > + for (;;) { > + d = readdir(ffs); > + if (!d) { > + return;Leaks ffs> + } > + > + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) { > + continue; > + } > + > + break; > + }Not sure where in the kernel the block/ sysfs directory is created for the device. I wasn't able to check that there is only ever 1 directory entry for a SCSI device's block/. Any ideas whether it's safe to grab the first directory entry?> + > + closedir(ffs); > + > + pstrcat(path, MAXPATHLEN, d->d_name); > + pstrcat(path, MAXPATHLEN, "/queue/"); > + > + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors); > + read_queue_limit(path, "max_segments", &bs->max_segments); > + read_queue_limit(path, "max_segment_size", &bs->max_segment_size); > +} > + > static int hdev_open(BlockDriverState *bs, const char *filename, int flags) > { > BDRVRawState *s = bs->opaque; > @@ -868,6 +925,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) > temp = realpath(filename, resolved_path); > if (temp && strstart(temp, "/dev/sg", NULL)) { > bs->sg = 1; > + sg_get_queue_limits(bs, temp); > } > } > #endif > diff --git a/block_int.h b/block_int.h > index d72317f..a9d07a2 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -333,6 +333,10 @@ struct BlockDriverState { > > /* long-running background operation */ > BlockJob *job; > + > + unsigned int max_sectors; > + unsigned int max_segments; > + unsigned int max_segment_size; > }; > > int get_tmp_filename(char *filename, int size); > diff --git a/blockdev.c b/blockdev.c > index 3d75015..e17edbf 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1191,3 +1191,18 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) > bdrv_iterate(do_qmp_query_block_jobs_one, &prev); > return dummy.next; > } > + > +unsigned int get_queue_max_sectors(BlockDriverState *bs)These should be bdrv_get_queue_max_sectors(BlockDriverState *bs) and should live in block.c.> +{ > + return (bs->file && bs->file->sg) ? bs->file->max_sectors : 0;The BlockDriver should be able to provide these values, we shouldn't reach into bs->file.
Stefan Hajnoczi
2012-Aug-21 09:56 UTC
[PATCH 2/2 v1] virtio-scsi: set per-LUN queue limits for sg devices
On Tue, Aug 21, 2012 at 9:23 AM, Cong Meng <mc at linux.vnet.ibm.com> wrote:> @@ -557,6 +621,7 @@ static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, > { > requested_features |= (1UL << VIRTIO_SCSI_F_HOTPLUG); > requested_features |= (1UL << VIRTIO_SCSI_F_CHANGE); > + requested_features |= (1UL << VIRTIO_SCSI_F_LUN_QUERY);This should be a QEMU 1.3 and later feature bit. VMs running with -m pc-1.2 or earlier should not see it by default, this ensure the device ABI is stable. For more info, see: http://patchwork.ozlabs.org/patch/177924/ Stefan
Blue Swirl
2012-Aug-21 18:31 UTC
[Qemu-devel] [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
On Tue, Aug 21, 2012 at 8:23 AM, Cong Meng <mc at linux.vnet.ibm.com> wrote:> This patch adds some queue limit parameters into block drive. And inits them > for sg block drive. Some interfaces are also added for accessing them. > > Signed-off-by: Cong Meng <mc at linux.vnet.ibm.com> > --- > block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > block_int.h | 4 +++ > blockdev.c | 15 +++++++++++++ > hw/block-common.h | 3 ++ > 4 files changed, 80 insertions(+), 0 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 0dce089..a086f89 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -53,6 +53,7 @@ > #include <linux/cdrom.h> > #include <linux/fd.h> > #include <linux/fs.h> > +#include <dirent.h> > #endif > #ifdef CONFIG_FIEMAP > #include <linux/fiemap.h> > @@ -829,6 +830,62 @@ static int hdev_probe_device(const char *filename) > return 0; > } > > +static void read_queue_limit(char *path, const char *filename, unsigned int *val) > +{ > + FILE *f; > + char *tail = path + strlen(path); > + > + pstrcat(path, MAXPATHLEN, filename); > + f = fopen(path, "r"); > + if (!f) { > + goto out; > + } > + > + fscanf(f, "%u", val);Please handle errors, otherwise *val may be left to uninitialized state.> + fclose(f); > + > +out: > + *tail = 0; > +} > + > +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename) > +{ > + DIR *ffs; > + struct dirent *d; > + char path[MAXPATHLEN]; > + > + snprintf(path, MAXPATHLEN, > + "/sys/class/scsi_generic/sg%s/device/block/", > + filename + strlen("/dev/sg")); > +I'd init bs->max_sectors etc. here so they are not left uninitialized.> + ffs = opendir(path); > + if (!ffs) { > + return; > + } > + > + for (;;) { > + d = readdir(ffs); > + if (!d) { > + return; > + } > + > + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) { > + continue; > + } > + > + break; > + } > + > + closedir(ffs); > + > + pstrcat(path, MAXPATHLEN, d->d_name); > + pstrcat(path, MAXPATHLEN, "/queue/"); > + > + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors); > + read_queue_limit(path, "max_segments", &bs->max_segments); > + read_queue_limit(path, "max_segment_size", &bs->max_segment_size); > +} > + > static int hdev_open(BlockDriverState *bs, const char *filename, int flags) > { > BDRVRawState *s = bs->opaque; > @@ -868,6 +925,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) > temp = realpath(filename, resolved_path); > if (temp && strstart(temp, "/dev/sg", NULL)) { > bs->sg = 1; > + sg_get_queue_limits(bs, temp); > } > } > #endif > diff --git a/block_int.h b/block_int.h > index d72317f..a9d07a2 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -333,6 +333,10 @@ struct BlockDriverState { > > /* long-running background operation */ > BlockJob *job; > + > + unsigned int max_sectors;With 32 bit ints and even with 4k sector size, the maximum disk size would be 16TB which may be soon exceeded by disks on the market. Please use 64 bit values, probably also for segment values below.> + unsigned int max_segments; > + unsigned int max_segment_size; > }; > > int get_tmp_filename(char *filename, int size); > diff --git a/blockdev.c b/blockdev.c > index 3d75015..e17edbf 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1191,3 +1191,18 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) > bdrv_iterate(do_qmp_query_block_jobs_one, &prev); > return dummy.next; > } > + > +unsigned int get_queue_max_sectors(BlockDriverState *bs) > +{ > + return (bs->file && bs->file->sg) ? bs->file->max_sectors : 0; > +} > + > +unsigned int get_queue_max_segments(BlockDriverState *bs) > +{ > + return (bs->file && bs->file->sg) ? bs->file->max_segments : 0; > +} > + > +unsigned int get_queue_max_segment_size(BlockDriverState *bs) > +{ > + return (bs->file && bs->file->sg) ? bs->file->max_segment_size : 0; > +} > diff --git a/hw/block-common.h b/hw/block-common.h > index bb808f7..d47fcd7 100644 > --- a/hw/block-common.h > +++ b/hw/block-common.h > @@ -76,4 +76,7 @@ void hd_geometry_guess(BlockDriverState *bs, > int *ptrans); > int hd_bios_chs_auto_trans(uint32_t cyls, uint32_t heads, uint32_t secs); > > +unsigned int get_queue_max_sectors(BlockDriverState *bs); > +unsigned int get_queue_max_segments(BlockDriverState *bs); > +unsigned int get_queue_max_segment_size(BlockDriverState *bs); > #endif > -- > 1.7.7.6 > >
Possibly Parallel Threads
- [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
- [PATCH 3/4] scsi-disk: Factor out SCSI command emulation
- [PATCH 3/4] scsi-disk: Factor out SCSI command emulation
- [blkfront] max_segments & max_segment_size
- [RFC][PATCH] Use ioemu block drivers through blktap