Message ID | 20190630150855.1016-2-mlevitsk@redhat.com |
---|---|
State | New |
Headers | show |
Series | RFC: don't obey the block device max transfer len / max segments for block devices | expand |
On 6/30/19 10:08 AM, Maxim Levitsky wrote: > Regular block devices (/dev/sda*, /dev/nvme*, etc) interface is not limited The regular block device interface is or Regular block devices interfaces are > by the underlying storage limits, but rather the kernel block layer > takes care to split the requests that are too large/fragmented. > > Doing so allows us to have less overhead in qemu. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > block/file-posix.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index ab05b51a66..66dad34f8a 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1038,15 +1038,13 @@ static void raw_reopen_abort(BDRVReopenState *state) > s->reopen_state = NULL; > } > > -static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) > +static int sg_get_max_transfer_length(BlockDriverState *bs, int fd) > { > #ifdef BLKSECTGET > int max_bytes = 0; > - short max_sectors = 0; > - if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) { > + > + if (ioctl(fd, BLKSECTGET, &max_bytes) == 0) { > return max_bytes; > - } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { > - return max_sectors << BDRV_SECTOR_BITS; > } else { > return -errno; > } > @@ -1055,7 +1053,7 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) > #endif > } > > -static int hdev_get_max_segments(const struct stat *st) > +static int sg_get_max_segments(const struct stat *st) > { > #ifdef CONFIG_LINUX > char buf[32]; > @@ -1106,12 +1104,12 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) > struct stat st; > > if (!fstat(s->fd, &st)) { > - if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) { > - int ret = hdev_get_max_transfer_length(bs, s->fd); Is it worth delaying the fstat()... > + if (bs->sg) { > + int ret = sg_get_max_transfer_length(bs, s->fd); > if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) { > bs->bl.max_transfer = pow2floor(ret); > } > - ret = hdev_get_max_segments(&st); > + ret = sg_get_max_segments(&st); ...until inside the if (bs->sg) condition, to avoid wasted work for other scenarios? > if (ret > 0) { > bs->bl.max_transfer = MIN(bs->bl.max_transfer, > ret * getpagesize()); > Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed, 2019-07-03 at 09:50 -0500, Eric Blake wrote: > On 6/30/19 10:08 AM, Maxim Levitsky wrote: > > Regular block devices (/dev/sda*, /dev/nvme*, etc) interface is not limited > > The regular block device interface is > > or > > Regular block devices interfaces are > > > by the underlying storage limits, but rather the kernel block layer > > takes care to split the requests that are too large/fragmented. > > > > Doing so allows us to have less overhead in qemu. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > block/file-posix.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index ab05b51a66..66dad34f8a 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -1038,15 +1038,13 @@ static void raw_reopen_abort(BDRVReopenState *state) > > s->reopen_state = NULL; > > } > > > > -static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) > > +static int sg_get_max_transfer_length(BlockDriverState *bs, int fd) > > { > > #ifdef BLKSECTGET > > int max_bytes = 0; > > - short max_sectors = 0; > > - if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) { > > + > > + if (ioctl(fd, BLKSECTGET, &max_bytes) == 0) { > > return max_bytes; > > - } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { > > - return max_sectors << BDRV_SECTOR_BITS; > > } else { > > return -errno; > > } > > @@ -1055,7 +1053,7 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) > > #endif > > } > > > > -static int hdev_get_max_segments(const struct stat *st) > > +static int sg_get_max_segments(const struct stat *st) > > { > > #ifdef CONFIG_LINUX > > char buf[32]; > > @@ -1106,12 +1104,12 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) > > struct stat st; > > > > if (!fstat(s->fd, &st)) { > > - if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) { > > - int ret = hdev_get_max_transfer_length(bs, s->fd); > > Is it worth delaying the fstat()... > > > + if (bs->sg) { > > + int ret = sg_get_max_transfer_length(bs, s->fd); > > if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) { > > bs->bl.max_transfer = pow2floor(ret); > > } > > - ret = hdev_get_max_segments(&st); > > + ret = sg_get_max_segments(&st); > > ...until inside the if (bs->sg) condition, to avoid wasted work for > other scenarios? > > > if (ret > 0) { > > bs->bl.max_transfer = MIN(bs->bl.max_transfer, > > ret * getpagesize()); > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > Thank you very much for the review. I'll send a V2 soon. Best regards, Maxim Levitsky
diff --git a/block/file-posix.c b/block/file-posix.c index ab05b51a66..66dad34f8a 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1038,15 +1038,13 @@ static void raw_reopen_abort(BDRVReopenState *state) s->reopen_state = NULL; } -static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) +static int sg_get_max_transfer_length(BlockDriverState *bs, int fd) { #ifdef BLKSECTGET int max_bytes = 0; - short max_sectors = 0; - if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) { + + if (ioctl(fd, BLKSECTGET, &max_bytes) == 0) { return max_bytes; - } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { - return max_sectors << BDRV_SECTOR_BITS; } else { return -errno; } @@ -1055,7 +1053,7 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) #endif } -static int hdev_get_max_segments(const struct stat *st) +static int sg_get_max_segments(const struct stat *st) { #ifdef CONFIG_LINUX char buf[32]; @@ -1106,12 +1104,12 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) struct stat st; if (!fstat(s->fd, &st)) { - if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) { - int ret = hdev_get_max_transfer_length(bs, s->fd); + if (bs->sg) { + int ret = sg_get_max_transfer_length(bs, s->fd); if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) { bs->bl.max_transfer = pow2floor(ret); } - ret = hdev_get_max_segments(&st); + ret = sg_get_max_segments(&st); if (ret > 0) { bs->bl.max_transfer = MIN(bs->bl.max_transfer, ret * getpagesize());
Regular block devices (/dev/sda*, /dev/nvme*, etc) interface is not limited by the underlying storage limits, but rather the kernel block layer takes care to split the requests that are too large/fragmented. Doing so allows us to have less overhead in qemu. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- block/file-posix.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)