| Message ID | 20170119205134.50112-4-farman@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 01/19 21:51, Eric Farman wrote: > Commit 6f607174 introduced a routine to get the maximum number > of bytes for a single I/O transfer for block devices, however > scsi generic devices are character devices, not block. Add > a condition for this, such that scsi generic devices can view > the same data. > > Some tweaking of data is required, because the block and sg > ioctls return values in different scales (sectors versus > bytes). So adjust hdev_get_max_transfer_length such that it > always returns a value in bytes. > > Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> > --- > block/file-posix.c | 10 ++++++---- > include/block/block.h | 1 + > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 2115155..94068ca 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) > int max_sectors = 0; > short max_sectors_short = 0; > if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { > + /* sg returns a value in bytes */ > return max_sectors; The variable name is now misleading, maybe use "bytes" instead? > } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors_short) == 0) { > - return max_sectors_short; > + /* block returns a value in sectors */ > + return max_sectors_short << BDRV_SECTOR_BITS; > } else { > return -errno; > } > @@ -674,10 +676,10 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) > struct stat st; > > if (!fstat(s->fd, &st)) { > - if (S_ISBLK(st.st_mode)) { > + if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) { > int ret = hdev_get_max_transfer_length(bs, s->fd); > - if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) { > - bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS); > + if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) { > + bs->bl.max_transfer = pow2floor(ret); > } > } > } > diff --git a/include/block/block.h b/include/block/block.h > index 8b0dcda..4e81f20 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -116,6 +116,7 @@ typedef struct HDGeometry { > > #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ > INT_MAX >> BDRV_SECTOR_BITS) > +#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > > /* > * Allocation status flags > -- > 2.8.4 > >
On Fri, 01/20 17:31, Fam Zheng wrote: > > diff --git a/block/file-posix.c b/block/file-posix.c > > index 2115155..94068ca 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) > > int max_sectors = 0; > > short max_sectors_short = 0; > > if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { > > + /* sg returns a value in bytes */ > > return max_sectors; > > The variable name is now misleading, maybe use "bytes" instead? BTW patch 2 should already make the function return bytes consistently. Doing it here is meaningless code churn. Fam
On 01/20/2017 04:53 AM, Fam Zheng wrote: > On Fri, 01/20 17:31, Fam Zheng wrote: >>> diff --git a/block/file-posix.c b/block/file-posix.c >>> index 2115155..94068ca 100644 >>> --- a/block/file-posix.c >>> +++ b/block/file-posix.c >>> @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) >>> int max_sectors = 0; >>> short max_sectors_short = 0; >>> if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { >>> + /* sg returns a value in bytes */ >>> return max_sectors; >> >> The variable name is now misleading, maybe use "bytes" instead? > > BTW patch 2 should already make the function return bytes consistently. Doing > it here is meaningless code churn. Excellent points. Spinning v3 now. - Eric
diff --git a/block/file-posix.c b/block/file-posix.c index 2115155..94068ca 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd) int max_sectors = 0; short max_sectors_short = 0; if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) { + /* sg returns a value in bytes */ return max_sectors; } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors_short) == 0) { - return max_sectors_short; + /* block returns a value in sectors */ + return max_sectors_short << BDRV_SECTOR_BITS; } else { return -errno; } @@ -674,10 +676,10 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) struct stat st; if (!fstat(s->fd, &st)) { - if (S_ISBLK(st.st_mode)) { + if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) { int ret = hdev_get_max_transfer_length(bs, s->fd); - if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) { - bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS); + if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) { + bs->bl.max_transfer = pow2floor(ret); } } } diff --git a/include/block/block.h b/include/block/block.h index 8b0dcda..4e81f20 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -116,6 +116,7 @@ typedef struct HDGeometry { #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ INT_MAX >> BDRV_SECTOR_BITS) +#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) /* * Allocation status flags
Commit 6f607174 introduced a routine to get the maximum number of bytes for a single I/O transfer for block devices, however scsi generic devices are character devices, not block. Add a condition for this, such that scsi generic devices can view the same data. Some tweaking of data is required, because the block and sg ioctls return values in different scales (sectors versus bytes). So adjust hdev_get_max_transfer_length such that it always returns a value in bytes. Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com> --- block/file-posix.c | 10 ++++++---- include/block/block.h | 1 + 2 files changed, 7 insertions(+), 4 deletions(-)