* [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API
2009-02-08 17:59 [Qemu-devel] [PATCH 0/3] Remove ->bdrv_pread() internal block layer API Avi Kivity
@ 2009-02-08 17:59 ` Avi Kivity
2009-02-08 17:59 ` [Qemu-devel] [PATCH 2/3] Add internal scsi generic block API Avi Kivity
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Avi Kivity @ 2009-02-08 17:59 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel
When a scsi device is backed by a scsi generic device instead of an
ordinary host block device, the block API is abused in a couple of annoying
ways:
- nb_sectors is negative, and specifies a byte count instead of a sector count
- offset is ignored, since scsi-generic is essentially a packet protocol
This overloading makes hacking the block layer difficult. Remove it by
introducing a new explicit API for scsi-generic devices. The new API
is still backed by the old implementation, but at least the users are
insulated.
Signed-off-by: Avi Kivity <avi@redhat.com>
---
block.c | 22 ++++++++++++++++++++++
block.h | 8 ++++++++
hw/scsi-generic.c | 39 ++++++++++++++++++++++-----------------
3 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/block.c b/block.c
index 4f4bf7c..774ac2d 100644
--- a/block.c
+++ b/block.c
@@ -1594,3 +1594,25 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
return drv->bdrv_ioctl(bs, req, buf);
return -ENOTSUP;
}
+
+int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count)
+{
+ return bdrv_pwrite(bs, -1, buf, count);
+}
+
+int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count)
+{
+ return bdrv_pread(bs, -1, buf, count);
+}
+
+BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
+ BlockDriverCompletionFunc *cb, void *opaque)
+{
+ return bdrv_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
+BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
+ BlockDriverCompletionFunc *cb, void *opaque)
+{
+ return bdrv_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
diff --git a/block.h b/block.h
index e1927dd..f06d98a 100644
--- a/block.h
+++ b/block.h
@@ -103,6 +103,14 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
BlockDriverCompletionFunc *cb, void *opaque);
void bdrv_aio_cancel(BlockDriverAIOCB *acb);
+/* sg packet commands */
+int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count);
+int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count);
+BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
+ BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
+ BlockDriverCompletionFunc *cb, void *opaque);
+
int qemu_key_check(BlockDriverState *bs, const char *name);
/* Ensure contents are flushed to disk. */
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 5bf160a..53e8951 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -201,6 +201,7 @@ static int execute_command(BlockDriverState *bdrv,
SCSIRequest *r, int direction,
BlockDriverCompletionFunc *complete)
{
+ int ret;
r->io_header.interface_id = 'S';
r->io_header.dxfer_direction = direction;
@@ -214,25 +215,27 @@ static int execute_command(BlockDriverState *bdrv,
r->io_header.usr_ptr = r;
r->io_header.flags |= SG_FLAG_DIRECT_IO;
- if (bdrv_pwrite(bdrv, -1, &r->io_header, sizeof(r->io_header)) == -1) {
+ ret = bdrv_sg_send_command(bdrv, &r->io_header, sizeof(r->io_header));
+ if (ret < 0) {
BADF("execute_command: write failed ! (%d)\n", errno);
return -1;
}
if (complete == NULL) {
int ret;
r->aiocb = NULL;
- while ((ret = bdrv_pread(bdrv, -1, &r->io_header,
- sizeof(r->io_header))) == -1 &&
- errno == EINTR);
- if (ret == -1) {
+ while ((ret = bdrv_sg_recv_response(bdrv, &r->io_header,
+ sizeof(r->io_header))) < 0 &&
+ ret == -EINTR)
+ ;
+ if (ret < 0) {
BADF("execute_command: read failed !\n");
return -1;
}
return 0;
}
- r->aiocb = bdrv_aio_read(bdrv, 0, (uint8_t*)&r->io_header,
- -(int64_t)sizeof(r->io_header), complete, r);
+ r->aiocb = bdrv_sg_aio_read(bdrv, (uint8_t*)&r->io_header,
+ sizeof(r->io_header), complete, r);
if (r->aiocb == NULL) {
BADF("execute_command: read failed !\n");
return -1;
@@ -634,14 +637,15 @@ static int get_blocksize(BlockDriverState *bdrv)
io_header.sbp = sensebuf;
io_header.timeout = 6000; /* XXX */
- ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
- if (ret == -1)
+ ret = bdrv_sg_send_command(bdrv, &io_header, sizeof(io_header));
+ if (ret < 0)
return -1;
- while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 &&
- errno == EINTR);
+ while ((ret = bdrv_sg_recv_response(bdrv, &io_header, sizeof(io_header))) < 0 &&
+ ret == -EINTR)
+ ;
- if (ret == -1)
+ if (ret < 0)
return -1;
return (buf[4] << 24) | (buf[5] << 16) | (buf[6] << 8) | buf[7];
@@ -671,14 +675,15 @@ static int get_stream_blocksize(BlockDriverState *bdrv)
io_header.sbp = sensebuf;
io_header.timeout = 6000; /* XXX */
- ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
- if (ret == -1)
+ ret = bdrv_sg_send_command(bdrv, &io_header, sizeof(io_header));
+ if (ret < 0)
return -1;
- while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 &&
- errno == EINTR);
+ while ((ret = bdrv_sg_recv_response(bdrv, &io_header, sizeof(io_header))) < 0 &&
+ ret == -EINTR)
+ ;
- if (ret == -1)
+ if (ret < 0)
return -1;
return (buf[9] << 16) | (buf[10] << 8) | buf[11];
--
1.6.1.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 2/3] Add internal scsi generic block API
2009-02-08 17:59 [Qemu-devel] [PATCH 0/3] Remove ->bdrv_pread() internal block layer API Avi Kivity
2009-02-08 17:59 ` [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API Avi Kivity
@ 2009-02-08 17:59 ` Avi Kivity
2009-02-08 17:59 ` [Qemu-devel] [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs Avi Kivity
2009-02-08 19:05 ` [Qemu-devel] Re: [PATCH 0/3] Remove ->bdrv_pread() internal block layer API Anthony Liguori
3 siblings, 0 replies; 8+ messages in thread
From: Avi Kivity @ 2009-02-08 17:59 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel
Add an internal API for the generic block layer to send scsi generic commands
to block format driver. This means block format drivers no longer need
to consider overloaded nb_sectors parameters.
Signed-off-by: Avi Kivity <avi@redhat.com>
---
block-raw-posix.c | 30 ++++++++++++++++++++++++++++++
block.c | 8 ++++----
block_int.h | 10 ++++++++++
3 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/block-raw-posix.c b/block-raw-posix.c
index 620791b..6b8adc4 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -1164,6 +1164,32 @@ static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
}
#endif /* !linux */
+static int raw_sg_send_command(BlockDriverState *bs, void *buf, int count)
+{
+ return raw_pwrite(bs, -1, buf, count);
+}
+
+static int raw_sg_recv_response(BlockDriverState *bs, void *buf, int count)
+{
+ return raw_pread(bs, -1, buf, count);
+}
+
+static BlockDriverAIOCB *raw_sg_aio_read(BlockDriverState *bs,
+ void *buf, int count,
+ BlockDriverCompletionFunc *cb,
+ void *opaque)
+{
+ return raw_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
+static BlockDriverAIOCB *raw_sg_aio_write(BlockDriverState *bs,
+ void *buf, int count,
+ BlockDriverCompletionFunc *cb,
+ void *opaque)
+{
+ return raw_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
BlockDriver bdrv_host_device = {
"host_device",
sizeof(BDRVRawState),
@@ -1193,4 +1219,8 @@ BlockDriver bdrv_host_device = {
.bdrv_set_locked = raw_set_locked,
/* generic scsi device */
.bdrv_ioctl = raw_ioctl,
+ .bdrv_sg_send_command = raw_sg_send_command,
+ .bdrv_sg_recv_response = raw_sg_recv_response,
+ .bdrv_sg_aio_read = raw_sg_aio_read,
+ .bdrv_sg_aio_write = raw_sg_aio_write,
};
diff --git a/block.c b/block.c
index 774ac2d..4aaea09 100644
--- a/block.c
+++ b/block.c
@@ -1597,22 +1597,22 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count)
{
- return bdrv_pwrite(bs, -1, buf, count);
+ return bs->drv->bdrv_sg_send_command(bs, buf, count);
}
int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count)
{
- return bdrv_pread(bs, -1, buf, count);
+ return bs->drv->bdrv_sg_recv_response(bs, buf, count);
}
BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
BlockDriverCompletionFunc *cb, void *opaque)
{
- return bdrv_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+ return bs->drv->bdrv_sg_aio_read(bs, buf, count, cb, opaque);
}
BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
BlockDriverCompletionFunc *cb, void *opaque)
{
- return bdrv_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+ return bs->drv->bdrv_sg_aio_write(bs, buf, count, cb, opaque);
}
diff --git a/block_int.h b/block_int.h
index e83fd2c..e4630f0 100644
--- a/block_int.h
+++ b/block_int.h
@@ -84,6 +84,16 @@ struct BlockDriver {
/* to control generic scsi devices */
int (*bdrv_ioctl)(BlockDriverState *bs, unsigned long int req, void *buf);
+ int (*bdrv_sg_send_command)(BlockDriverState *bs, void *buf, int count);
+ int (*bdrv_sg_recv_response)(BlockDriverState *bs, void *buf, int count);
+ BlockDriverAIOCB *(*bdrv_sg_aio_read)(BlockDriverState *bs,
+ void *buf, int count,
+ BlockDriverCompletionFunc *cb,
+ void *opaque);
+ BlockDriverAIOCB *(*bdrv_sg_aio_write)(BlockDriverState *bs,
+ void *buf, int count,
+ BlockDriverCompletionFunc *cb,
+ void *opaque);
BlockDriverAIOCB *free_aiocb;
struct BlockDriver *next;
--
1.6.1.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs
2009-02-08 17:59 [Qemu-devel] [PATCH 0/3] Remove ->bdrv_pread() internal block layer API Avi Kivity
2009-02-08 17:59 ` [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API Avi Kivity
2009-02-08 17:59 ` [Qemu-devel] [PATCH 2/3] Add internal scsi generic block API Avi Kivity
@ 2009-02-08 17:59 ` Avi Kivity
2009-02-08 19:10 ` [Qemu-devel] " Anthony Liguori
2009-02-08 19:05 ` [Qemu-devel] Re: [PATCH 0/3] Remove ->bdrv_pread() internal block layer API Anthony Liguori
3 siblings, 1 reply; 8+ messages in thread
From: Avi Kivity @ 2009-02-08 17:59 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel
Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), we can
drop the corresponding internal APIs, which overlap bdrv_read()/bdrv_write()
and, being byte oriented, are unnatural for a block device.
Signed-off-by: Avi Kivity <avi@redhat.com>
---
block-raw-posix.c | 20 +++++++++++---
block-raw-win32.c | 20 ++++++++------
block.c | 73 ++++------------------------------------------------
block_int.h | 4 ---
4 files changed, 34 insertions(+), 83 deletions(-)
diff --git a/block-raw-posix.c b/block-raw-posix.c
index 6b8adc4..9b32615 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -353,6 +353,12 @@ static int raw_pread(BlockDriverState *bs, int64_t offset,
return raw_pread_aligned(bs, offset, buf, count) + sum;
}
+static int raw_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ return raw_pread(bs, sector_num * 512, buf, (uint64_t)nb_sectors * 512);
+}
+
/*
* offset and count are in bytes and possibly not aligned. For files opened
* with O_DIRECT, necessary alignments are ensured before calling
@@ -431,6 +437,12 @@ static int raw_pwrite(BlockDriverState *bs, int64_t offset,
return raw_pwrite_aligned(bs, offset, buf, count) + sum;
}
+static int raw_write(BlockDriverState *bs, int64_t sector_num,
+ const uint8_t *buf, int nb_sectors)
+{
+ return raw_pwrite(bs, sector_num * 512, buf, (uint64_t)nb_sectors * 512);
+}
+
#ifdef CONFIG_AIO
/***********************************************************/
/* Unix AIO using POSIX AIO */
@@ -828,8 +840,8 @@ BlockDriver bdrv_raw = {
.aiocb_size = sizeof(RawAIOCB),
#endif
- .bdrv_pread = raw_pread,
- .bdrv_pwrite = raw_pwrite,
+ .bdrv_read = raw_read,
+ .bdrv_write = raw_write,
.bdrv_truncate = raw_truncate,
.bdrv_getlength = raw_getlength,
};
@@ -1208,8 +1220,8 @@ BlockDriver bdrv_host_device = {
.aiocb_size = sizeof(RawAIOCB),
#endif
- .bdrv_pread = raw_pread,
- .bdrv_pwrite = raw_pwrite,
+ .bdrv_read = raw_read,
+ .bdrv_write = raw_write,
.bdrv_getlength = raw_getlength,
/* removable device support */
diff --git a/block-raw-win32.c b/block-raw-win32.c
index 892f2d1..f37ab68 100644
--- a/block-raw-win32.c
+++ b/block-raw-win32.c
@@ -121,13 +121,15 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
return 0;
}
-static int raw_pread(BlockDriverState *bs, int64_t offset,
- uint8_t *buf, int count)
+static int raw_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
{
BDRVRawState *s = bs->opaque;
OVERLAPPED ov;
DWORD ret_count;
int ret;
+ int64_t offset = sector_num * 512;
+ int count = nb_sectors * 512;
memset(&ov, 0, sizeof(ov));
ov.Offset = offset;
@@ -145,13 +147,15 @@ static int raw_pread(BlockDriverState *bs, int64_t offset,
return ret_count;
}
-static int raw_pwrite(BlockDriverState *bs, int64_t offset,
- const uint8_t *buf, int count)
+static int raw_write(BlockDriverState *bs, int64_t sector_num,
+ const uint8_t *buf, int nb_sectors)
{
BDRVRawState *s = bs->opaque;
OVERLAPPED ov;
DWORD ret_count;
int ret;
+ int64_t offset = sector_num * 512;
+ int count = nb_sectors * 512;
memset(&ov, 0, sizeof(ov));
ov.Offset = offset;
@@ -358,8 +362,8 @@ BlockDriver bdrv_raw = {
.bdrv_aio_cancel = raw_aio_cancel,
.aiocb_size = sizeof(RawAIOCB);
#endif
- .bdrv_pread = raw_pread,
- .bdrv_pwrite = raw_pwrite,
+ .bdrv_read = raw_read,
+ .bdrv_write = raw_write,
.bdrv_truncate = raw_truncate,
.bdrv_getlength = raw_getlength,
};
@@ -511,7 +515,7 @@ BlockDriver bdrv_host_device = {
.bdrv_aio_cancel = raw_aio_cancel,
.aiocb_size = sizeof(RawAIOCB);
#endif
- .bdrv_pread = raw_pread,
- .bdrv_pwrite = raw_pwrite,
+ .bdrv_read = raw_read,
+ .bdrv_write = raw_write,
.bdrv_getlength = raw_getlength,
};
diff --git a/block.c b/block.c
index 4aaea09..5963d54 100644
--- a/block.c
+++ b/block.c
@@ -136,7 +136,7 @@ static void bdrv_register(BlockDriver *bdrv)
bdrv->bdrv_aio_write = bdrv_aio_write_em;
bdrv->bdrv_aio_cancel = bdrv_aio_cancel_em;
bdrv->aiocb_size = sizeof(BlockDriverAIOCBSync);
- } else if (!bdrv->bdrv_read && !bdrv->bdrv_pread) {
+ } else if (!bdrv->bdrv_read) {
/* add synchronous IO emulation layer */
bdrv->bdrv_read = bdrv_read_em;
bdrv->bdrv_write = bdrv_write_em;
@@ -528,22 +528,7 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
if (!drv)
return -ENOMEDIUM;
- if (drv->bdrv_pread) {
- int ret, len;
- len = nb_sectors * 512;
- ret = drv->bdrv_pread(bs, sector_num * 512, buf, len);
- if (ret < 0)
- return ret;
- else if (ret != len)
- return -EINVAL;
- else {
- bs->rd_bytes += (unsigned) len;
- bs->rd_ops ++;
- return 0;
- }
- } else {
- return drv->bdrv_read(bs, sector_num, buf, nb_sectors);
- }
+ return drv->bdrv_read(bs, sector_num, buf, nb_sectors);
}
/* Return < 0 if error. Important errors are:
@@ -560,27 +545,11 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
return -ENOMEDIUM;
if (bs->read_only)
return -EACCES;
- if (drv->bdrv_pwrite) {
- int ret, len, count = 0;
- len = nb_sectors * 512;
- do {
- ret = drv->bdrv_pwrite(bs, sector_num * 512, buf, len - count);
- if (ret < 0) {
- printf("bdrv_write ret=%d\n", ret);
- return ret;
- }
- count += ret;
- buf += ret;
- } while (count != len);
- bs->wr_bytes += (unsigned) len;
- bs->wr_ops ++;
- return 0;
- }
return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
}
-static int bdrv_pread_em(BlockDriverState *bs, int64_t offset,
- uint8_t *buf, int count1)
+int bdrv_pread(BlockDriverState *bs, int64_t offset,
+ void *buf, int count1)
{
uint8_t tmp_buf[SECTOR_SIZE];
int len, nb_sectors, count;
@@ -623,8 +592,8 @@ static int bdrv_pread_em(BlockDriverState *bs, int64_t offset,
return count1;
}
-static int bdrv_pwrite_em(BlockDriverState *bs, int64_t offset,
- const uint8_t *buf, int count1)
+int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
+ const void *buf, int count1)
{
uint8_t tmp_buf[SECTOR_SIZE];
int len, nb_sectors, count;
@@ -672,36 +641,6 @@ static int bdrv_pwrite_em(BlockDriverState *bs, int64_t offset,
}
/**
- * Read with byte offsets (needed only for file protocols)
- */
-int bdrv_pread(BlockDriverState *bs, int64_t offset,
- void *buf1, int count1)
-{
- BlockDriver *drv = bs->drv;
-
- if (!drv)
- return -ENOMEDIUM;
- if (!drv->bdrv_pread)
- return bdrv_pread_em(bs, offset, buf1, count1);
- return drv->bdrv_pread(bs, offset, buf1, count1);
-}
-
-/**
- * Write with byte offsets (needed only for file protocols)
- */
-int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
- const void *buf1, int count1)
-{
- BlockDriver *drv = bs->drv;
-
- if (!drv)
- return -ENOMEDIUM;
- if (!drv->bdrv_pwrite)
- return bdrv_pwrite_em(bs, offset, buf1, count1);
- return drv->bdrv_pwrite(bs, offset, buf1, count1);
-}
-
-/**
* Truncate file to 'offset' bytes (needed only for file protocols)
*/
int bdrv_truncate(BlockDriverState *bs, int64_t offset)
diff --git a/block_int.h b/block_int.h
index e4630f0..cc9966b 100644
--- a/block_int.h
+++ b/block_int.h
@@ -58,10 +58,6 @@ struct BlockDriver {
int aiocb_size;
const char *protocol_name;
- int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
- uint8_t *buf, int count);
- int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
- const uint8_t *buf, int count);
int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
int64_t (*bdrv_getlength)(BlockDriverState *bs);
int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
--
1.6.1.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs
2009-02-08 17:59 ` [Qemu-devel] [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs Avi Kivity
@ 2009-02-08 19:10 ` Anthony Liguori
2009-02-08 19:36 ` Avi Kivity
2009-02-08 19:37 ` Avi Kivity
0 siblings, 2 replies; 8+ messages in thread
From: Anthony Liguori @ 2009-02-08 19:10 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel
Avi Kivity wrote:
> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), we can
> drop the corresponding internal APIs, which overlap bdrv_read()/bdrv_write()
> and, being byte oriented, are unnatural for a block device.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
>
> int bdrv_truncate(BlockDriverState *bs, int64_t offset)
> diff --git a/block_int.h b/block_int.h
> index e4630f0..cc9966b 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -58,10 +58,6 @@ struct BlockDriver {
> int aiocb_size;
>
> const char *protocol_name;
> - int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
> - uint8_t *buf, int count);
> - int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
> - const uint8_t *buf, int count);
>
$ grep -l bdrv_pwrite *.c hw/*.c
block.c
block-qcow2.c
block-qcow.c
block-raw-posix.c
block-raw-win32.c
block-vmdk.c
block-vpc.c
savevm.c
hw/scsi-generic.c
So there's a lot of users other than scsi-generic. Usually, these
callers are in the block layer to read/write metadata that isn't always
block aligned. Some buffer adjustment could fix savevm.c to ensure
alignment.
These users are now relegated to using emulated pread/pwrite? Won't
that have a noticable impact on performance if updating small bits of
metadata. For instance, I think updating qcow2 refcounts would look bad
since you have to read/write the full block to update 4 bytes of data.
Granted it'll be cached, but...
Regards,
Anthony Liguori
> int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
> int64_t (*bdrv_getlength)(BlockDriverState *bs);
> int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs
2009-02-08 19:10 ` [Qemu-devel] " Anthony Liguori
@ 2009-02-08 19:36 ` Avi Kivity
2009-02-08 19:37 ` Avi Kivity
1 sibling, 0 replies; 8+ messages in thread
From: Avi Kivity @ 2009-02-08 19:36 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel
Anthony Liguori wrote:
> Avi Kivity wrote:
>> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(),
>> we can
>> drop the corresponding internal APIs, which overlap
>> bdrv_read()/bdrv_write()
>> and, being byte oriented, are unnatural for a block device.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>
>> int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>> diff --git a/block_int.h b/block_int.h
>> index e4630f0..cc9966b 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -58,10 +58,6 @@ struct BlockDriver {
>> int aiocb_size;
>>
>> const char *protocol_name;
>> - int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
>> - uint8_t *buf, int count);
>> - int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
>> - const uint8_t *buf, int count);
>>
>
> $ grep -l bdrv_pwrite *.c hw/*.c
> block.c
> block-qcow2.c
> block-qcow.c
> block-raw-posix.c
> block-raw-win32.c
> block-vmdk.c
> block-vpc.c
> savevm.c
> hw/scsi-generic.c
>
> So there's a lot of users other than scsi-generic. Usually, these
> callers are in the block layer to read/write metadata that isn't
> always block aligned. Some buffer adjustment could fix savevm.c to
> ensure alignment.
It's more accurate to say that there are now no users that depend on the
request size. The other users will happily allow expansion of a small
request to a sector.
> These users are now relegated to using emulated pread/pwrite? Won't
> that have a noticable impact on performance if updating small bits of
> metadata. For instance, I think updating qcow2 refcounts would look
> bad since you have to read/write the full block to update 4 bytes of
> data. Granted it'll be cached, but...
I haven't measured but I'll bet the impact is unnoticable. We're
doubling the syscall count for writes, but the actual transfer (if
cached) or the I/O (if uncached) will swamp that. For reads, we're
copying a bit more data, but that won't even tickle performance.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs
2009-02-08 19:10 ` [Qemu-devel] " Anthony Liguori
2009-02-08 19:36 ` Avi Kivity
@ 2009-02-08 19:37 ` Avi Kivity
1 sibling, 0 replies; 8+ messages in thread
From: Avi Kivity @ 2009-02-08 19:37 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel
Anthony Liguori wrote:
> Avi Kivity wrote:
>> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(),
>> we can
>> drop the corresponding internal APIs, which overlap
>> bdrv_read()/bdrv_write()
>> and, being byte oriented, are unnatural for a block device.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>
>> int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>> diff --git a/block_int.h b/block_int.h
>> index e4630f0..cc9966b 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -58,10 +58,6 @@ struct BlockDriver {
>> int aiocb_size;
>>
>> const char *protocol_name;
>> - int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
>> - uint8_t *buf, int count);
>> - int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
>> - const uint8_t *buf, int count);
>>
>
> $ grep -l bdrv_pwrite *.c hw/*.c
> block.c
> block-qcow2.c
> block-qcow.c
> block-raw-posix.c
> block-raw-win32.c
> block-vmdk.c
> block-vpc.c
> savevm.c
> hw/scsi-generic.c
>
> So there's a lot of users other than scsi-generic. Usually, these
> callers are in the block layer to read/write metadata that isn't
> always block aligned. Some buffer adjustment could fix savevm.c to
> ensure alignment.
It's more accurate to say that there are now no users that depend on the
request size. The other users will happily allow expansion of a small
request to a sector.
> These users are now relegated to using emulated pread/pwrite? Won't
> that have a noticable impact on performance if updating small bits of
> metadata. For instance, I think updating qcow2 refcounts would look
> bad since you have to read/write the full block to update 4 bytes of
> data. Granted it'll be cached, but...
I haven't measured but I'll bet the impact is unnoticable. We're
doubling the syscall count for writes, but the actual transfer (if
cached) or the I/O (if uncached) will swamp that. For reads, we're
copying a bit more data, but that won't even tickle performance.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH 0/3] Remove ->bdrv_pread() internal block layer API
2009-02-08 17:59 [Qemu-devel] [PATCH 0/3] Remove ->bdrv_pread() internal block layer API Avi Kivity
` (2 preceding siblings ...)
2009-02-08 17:59 ` [Qemu-devel] [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs Avi Kivity
@ 2009-02-08 19:05 ` Anthony Liguori
3 siblings, 0 replies; 8+ messages in thread
From: Anthony Liguori @ 2009-02-08 19:05 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel
Avi Kivity wrote:
> The internal block layer API has a wart in the form of the
> ->bdrv_pread() and ->bdrv_pwrite() methods. These duplicate
> bdrv_read() and bdrv_write(), and are unnatural for a block
> device in that they are not sector oriented.
>
> One of the reasons for their existence is the scsi-generic host support.
> That is also a wart, as scsi-generic is really a packet (command/response)
> rather than a block (random access to sectors) API. There is also the
> wierd negative sector count == byte count thing.
>
Great stuff, this has always bugged me.
Regards,
Anthony Liguori
> This patchset addresses these warts by adding a new API for scsi-generic
> and dropping ->bdrv_pread() and bdrv_pwrite(). While the improvement
> falls short of spectacular, it does make further hacking on the block
> layer easier as there are fewer callbacks to worry about.
>
> Avi Kivity (3):
> Add specialized block driver scsi generic API
> Add internal scsi generic block API
> Drop internal bdrv_pread()/bdrv_pwrite() APIs
>
> block-raw-posix.c | 50 +++++++++++++++++++++++++--
> block-raw-win32.c | 20 +++++++----
> block.c | 95 +++++++++++++++-------------------------------------
> block.h | 8 ++++
> block_int.h | 14 +++++--
> hw/scsi-generic.c | 39 ++++++++++++---------
> 6 files changed, 126 insertions(+), 100 deletions(-)
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread