| Message ID | 1417802181-20834-4-git-send-email-tumanova@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 5 Dec 2014 18:56:19 +0100 Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote: > This patch introduces driver methods of defining disk blocksizes > (physical and logical) and hard drive geometry. > The method is only implemented for "host_device". For "raw" devices > driver calls child's method. > > For the time being geometry detection will only work for DASD devices. > In order to check that a local check_for_dasd function was introduced, > which calls BIODASDINFO2 ioctl and returns its rc. > > Blocksizes detection fuction will probe sizes for DASD devices and > set default for other devices. > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> > --- > block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block/raw_bsd.c | 14 +++++++++ > 2 files changed, 105 insertions(+) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 633d5bc..33f9983 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -56,6 +56,7 @@ > #include <linux/cdrom.h> > #include <linux/fd.h> > #include <linux/fs.h> > +#include <linux/hdreg.h> > #ifndef FS_NOCOW_FL > #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ > #endif > @@ -90,6 +91,10 @@ > #include <xfs/xfs.h> > #endif > > +#ifdef __s390__ > +#include <asm/dasd.h> > +#endif > + > //#define DEBUG_FLOPPY > > //#define DEBUG_BLOCK > @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size) > return 0; > } > > +/* > + * Set physical block size via ioctl. On success return 0. Otherwise -errno. > + */ > +static int probe_physical_blocksize(int fd, unsigned int *blk_size) > +{ > +#ifdef BLKPBSZGET > + if (ioctl(fd, BLKPBSZGET, blk_size) < 0) { > + return -errno; > + } > +#endif > + > + return 0; > +} > + > static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) > { > BDRVRawState *s = bs->opaque; > @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) > bs->bl.opt_mem_alignment = s->buf_align; > } > > +static int check_for_dasd(int fd) > +{ > +#ifdef BIODASDINFO2 > + struct dasd_information2_t info = {0}; > + > + return ioctl(fd, BIODASDINFO2, &info); > +#endif > + return -1; I'd put the "return -1" line into an #else branch of the #ifdef, so that you do not end up with two consecutive return statements in case BIODASDINFO2 is defined. > +} > + > +/* > + * Try to get the device blocksize. On success 0. On failure return -errno. > + * Currently only implemented for DASD drives. > + */ > +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > +{ > + BDRVRawState *s = bs->opaque; > + int ret; > + > + /* If DASD, get blocksizes */ > + if (check_for_dasd(s->fd) < 0) { > + return -1; > + } > + ret = probe_logical_blocksize(s->fd, &bsz->log); > + if (ret < 0) { > + return ret; > + } > + return probe_physical_blocksize(s->fd, &bsz->phys); > +} > + > +/* > + * Try to get the device geometry. On success 0. On failure return -errno. "On success return 0" > + * Currently only implemented for DASD drives. > + */ > +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo) > +{ > + BDRVRawState *s = bs->opaque; > + struct hd_geometry ioctl_geo = {0}; > + uint32_t blksize; > + > + /* If DASD, get it's geometry */ > + if (check_for_dasd(s->fd) < 0) { > + return -1; > + } > + if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) { > + return -errno; > + } > + /* HDIO_GETGEO may return success even though geo contains zeros > + (e.g. certain multipath setups) */ > + if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) { > + return -1; > + } > + /* Do not return a geometry for partition */ > + if (ioctl_geo.start != 0) { > + return -1; > + } > + geo->heads = ioctl_geo.heads; > + geo->sectors = ioctl_geo.sectors; > + if (bs->total_sectors) { Maybe add a comment here why you've got to calculate the cylinders here instead of using ioctl_geo.cylinders ? > + if (!probe_physical_blocksize(s->fd, &blksize)) { > + geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE) > + / (geo->heads * geo->sectors); > + return 0; > + } > + } > + geo->cylinders = ioctl_geo.cylinders; > + > + return 0; > +} > + > static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb) > { > int ret; > @@ -2127,6 +2216,8 @@ static BlockDriver bdrv_host_device = { > .bdrv_get_info = raw_get_info, > .bdrv_get_allocated_file_size > = raw_get_allocated_file_size, > + .bdrv_probe_blocksizes = hdev_probe_blocksizes, > + .bdrv_probe_geometry = hdev_probe_geometry, > > .bdrv_detach_aio_context = raw_detach_aio_context, > .bdrv_attach_aio_context = raw_attach_aio_context, > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index 401b967..cfd5249 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) > return 1; > } > > +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > +{ > + bdrv_probe_blocksizes(bs->file, bsz); > + > + return 0; > +} > + > +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) > +{ > + return bdrv_probe_geometry(bs->file, geo); > +} > + > static BlockDriver bdrv_raw = { > .format_name = "raw", > .bdrv_probe = &raw_probe, > @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { > .has_variable_length = true, > .bdrv_get_info = &raw_get_info, > .bdrv_refresh_limits = &raw_refresh_limits, > + .bdrv_probe_blocksizes = &raw_probe_blocksizes, > + .bdrv_probe_geometry = &raw_probe_geometry, > .bdrv_is_inserted = &raw_is_inserted, > .bdrv_media_changed = &raw_media_changed, > .bdrv_eject = &raw_eject, Hmmm, raw_probe_blocksizes() calls bdrv_probe_blocksizes(), but when I look at your patch 1/5, bdrv_probe_blocksizes() wants to call drv->bdrv_probe_blocksizes() (i.e. raw_probe_blocksizes()) again? Don't you get an endless recursive loop here? Or did I miss something? *confused* Thomas
On 12/10/2014 04:14 PM, Thomas Huth wrote: > On Fri, 5 Dec 2014 18:56:19 +0100 > Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote: > >> This patch introduces driver methods of defining disk blocksizes >> (physical and logical) and hard drive geometry. >> The method is only implemented for "host_device". For "raw" devices >> driver calls child's method. >> >> For the time being geometry detection will only work for DASD devices. >> In order to check that a local check_for_dasd function was introduced, >> which calls BIODASDINFO2 ioctl and returns its rc. >> >> Blocksizes detection fuction will probe sizes for DASD devices and >> set default for other devices. >> >> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> >> --- >> block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/raw_bsd.c | 14 +++++++++ >> 2 files changed, 105 insertions(+) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 633d5bc..33f9983 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -56,6 +56,7 @@ >> #include <linux/cdrom.h> >> #include <linux/fd.h> >> #include <linux/fs.h> >> +#include <linux/hdreg.h> >> #ifndef FS_NOCOW_FL >> #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> #endif >> @@ -90,6 +91,10 @@ >> #include <xfs/xfs.h> >> #endif >> >> +#ifdef __s390__ >> +#include <asm/dasd.h> >> +#endif >> + >> //#define DEBUG_FLOPPY >> >> //#define DEBUG_BLOCK >> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size) >> return 0; >> } >> >> +/* >> + * Set physical block size via ioctl. On success return 0. Otherwise -errno. >> + */ >> +static int probe_physical_blocksize(int fd, unsigned int *blk_size) >> +{ >> +#ifdef BLKPBSZGET >> + if (ioctl(fd, BLKPBSZGET, blk_size) < 0) { >> + return -errno; >> + } >> +#endif >> + >> + return 0; >> +} >> + >> static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) >> { >> BDRVRawState *s = bs->opaque; >> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) >> bs->bl.opt_mem_alignment = s->buf_align; >> } >> >> +static int check_for_dasd(int fd) >> +{ >> +#ifdef BIODASDINFO2 >> + struct dasd_information2_t info = {0}; >> + >> + return ioctl(fd, BIODASDINFO2, &info); >> +#endif >> + return -1; > > I'd put the "return -1" line into an #else branch of the #ifdef, so > that you do not end up with two consecutive return statements in case > BIODASDINFO2 is defined. > >> +} >> + >> +/* >> + * Try to get the device blocksize. On success 0. On failure return -errno. >> + * Currently only implemented for DASD drives. >> + */ >> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) >> +{ >> + BDRVRawState *s = bs->opaque; >> + int ret; >> + >> + /* If DASD, get blocksizes */ >> + if (check_for_dasd(s->fd) < 0) { >> + return -1; >> + } >> + ret = probe_logical_blocksize(s->fd, &bsz->log); >> + if (ret < 0) { >> + return ret; >> + } >> + return probe_physical_blocksize(s->fd, &bsz->phys); >> +} >> + >> +/* >> + * Try to get the device geometry. On success 0. On failure return -errno. > > "On success return 0" > >> + * Currently only implemented for DASD drives. >> + */ >> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo) >> +{ >> + BDRVRawState *s = bs->opaque; >> + struct hd_geometry ioctl_geo = {0}; >> + uint32_t blksize; >> + >> + /* If DASD, get it's geometry */ >> + if (check_for_dasd(s->fd) < 0) { >> + return -1; >> + } >> + if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) { >> + return -errno; >> + } >> + /* HDIO_GETGEO may return success even though geo contains zeros >> + (e.g. certain multipath setups) */ >> + if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) { >> + return -1; >> + } >> + /* Do not return a geometry for partition */ >> + if (ioctl_geo.start != 0) { >> + return -1; >> + } >> + geo->heads = ioctl_geo.heads; >> + geo->sectors = ioctl_geo.sectors; >> + if (bs->total_sectors) { > > Maybe add a comment here why you've got to calculate the cylinders here > instead of using ioctl_geo.cylinders ? > >> + if (!probe_physical_blocksize(s->fd, &blksize)) { >> + geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE) >> + / (geo->heads * geo->sectors); >> + return 0; >> + } >> + } >> + geo->cylinders = ioctl_geo.cylinders; >> + >> + return 0; >> +} >> + >> static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb) >> { >> int ret; >> @@ -2127,6 +2216,8 @@ static BlockDriver bdrv_host_device = { >> .bdrv_get_info = raw_get_info, >> .bdrv_get_allocated_file_size >> = raw_get_allocated_file_size, >> + .bdrv_probe_blocksizes = hdev_probe_blocksizes, >> + .bdrv_probe_geometry = hdev_probe_geometry, >> >> .bdrv_detach_aio_context = raw_detach_aio_context, >> .bdrv_attach_aio_context = raw_attach_aio_context, >> diff --git a/block/raw_bsd.c b/block/raw_bsd.c >> index 401b967..cfd5249 100644 >> --- a/block/raw_bsd.c >> +++ b/block/raw_bsd.c >> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) >> return 1; >> } >> >> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) >> +{ >> + bdrv_probe_blocksizes(bs->file, bsz); >> + >> + return 0; >> +} >> + >> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) >> +{ >> + return bdrv_probe_geometry(bs->file, geo); >> +} >> + >> static BlockDriver bdrv_raw = { >> .format_name = "raw", >> .bdrv_probe = &raw_probe, >> @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { >> .has_variable_length = true, >> .bdrv_get_info = &raw_get_info, >> .bdrv_refresh_limits = &raw_refresh_limits, >> + .bdrv_probe_blocksizes = &raw_probe_blocksizes, >> + .bdrv_probe_geometry = &raw_probe_geometry, >> .bdrv_is_inserted = &raw_is_inserted, >> .bdrv_media_changed = &raw_media_changed, >> .bdrv_eject = &raw_eject, > > Hmmm, raw_probe_blocksizes() calls bdrv_probe_blocksizes(), but when I > look at your patch 1/5, bdrv_probe_blocksizes() wants to call > drv->bdrv_probe_blocksizes() (i.e. raw_probe_blocksizes()) again? > Don't you get an endless recursive loop here? Or did I miss something? > *confused* > > Thomas > > No I don't :) Because raw_probe_blocksizes indeed calls bdrv_probe_blocksizes() dispatcher, but it calls it against different driver: "bs->file". This child points to other driver, which represents the actual nature of the device. So the 2nd drv->bdrv_probe_blocksizes() call will actually be a call to either hdev_probe_blocksizes() for "host_device" driver or will be null for other drivers. Kate.
On Thu, 11 Dec 2014 14:17:21 +0300 Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote: > On 12/10/2014 04:14 PM, Thomas Huth wrote: > > On Fri, 5 Dec 2014 18:56:19 +0100 > > Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote: ... > >> diff --git a/block/raw_bsd.c b/block/raw_bsd.c > >> index 401b967..cfd5249 100644 > >> --- a/block/raw_bsd.c > >> +++ b/block/raw_bsd.c > >> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) > >> return 1; > >> } > >> > >> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > >> +{ > >> + bdrv_probe_blocksizes(bs->file, bsz); > >> + > >> + return 0; > >> +} > >> + > >> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) > >> +{ > >> + return bdrv_probe_geometry(bs->file, geo); > >> +} > >> + > >> static BlockDriver bdrv_raw = { > >> .format_name = "raw", > >> .bdrv_probe = &raw_probe, > >> @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { > >> .has_variable_length = true, > >> .bdrv_get_info = &raw_get_info, > >> .bdrv_refresh_limits = &raw_refresh_limits, > >> + .bdrv_probe_blocksizes = &raw_probe_blocksizes, > >> + .bdrv_probe_geometry = &raw_probe_geometry, > >> .bdrv_is_inserted = &raw_is_inserted, > >> .bdrv_media_changed = &raw_media_changed, > >> .bdrv_eject = &raw_eject, > > > > Hmmm, raw_probe_blocksizes() calls bdrv_probe_blocksizes(), but when I > > look at your patch 1/5, bdrv_probe_blocksizes() wants to call > > drv->bdrv_probe_blocksizes() (i.e. raw_probe_blocksizes()) again? > > Don't you get an endless recursive loop here? Or did I miss something? > > *confused* > > > > Thomas > > No I don't :) Because raw_probe_blocksizes indeed calls > bdrv_probe_blocksizes() dispatcher, but it calls it against different > driver: "bs->file". This child points to other driver, which represents > the actual nature of the device. > > So the 2nd drv->bdrv_probe_blocksizes() call will actually be > a call to either hdev_probe_blocksizes() for "host_device" driver or > will be null for other drivers. Ah, ok, you're right of course! Thanks for the explanation and sorry for the confusion! Thomas
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > This patch introduces driver methods of defining disk blocksizes > (physical and logical) and hard drive geometry. > The method is only implemented for "host_device". For "raw" devices > driver calls child's method. > > For the time being geometry detection will only work for DASD devices. > In order to check that a local check_for_dasd function was introduced, > which calls BIODASDINFO2 ioctl and returns its rc. > > Blocksizes detection fuction will probe sizes for DASD devices and > set default for other devices. > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> > --- > block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block/raw_bsd.c | 14 +++++++++ > 2 files changed, 105 insertions(+) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 633d5bc..33f9983 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -56,6 +56,7 @@ > #include <linux/cdrom.h> > #include <linux/fd.h> > #include <linux/fs.h> > +#include <linux/hdreg.h> > #ifndef FS_NOCOW_FL > #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ > #endif > @@ -90,6 +91,10 @@ > #include <xfs/xfs.h> > #endif > > +#ifdef __s390__ > +#include <asm/dasd.h> > +#endif > + > //#define DEBUG_FLOPPY > > //#define DEBUG_BLOCK > @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size) > return 0; > } > > +/* > + * Set physical block size via ioctl. On success return 0. Otherwise -errno. > + */ Set? Ah, now I get your logic, you mean "set *blk_size"! Suggest to say it like this: /** * Get physical block size of @fd. * On success, store it in @blk_size and return 0. * On failure, return -errno. */ > +static int probe_physical_blocksize(int fd, unsigned int *blk_size) > +{ > +#ifdef BLKPBSZGET > + if (ioctl(fd, BLKPBSZGET, blk_size) < 0) { > + return -errno; > + } > +#endif > + > + return 0; If !defined(BLKPBSZGET), you return 0 without setting *blk_size. I think you need to fail then. -ENOTSUP should do. > +} > + > static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) > { > BDRVRawState *s = bs->opaque; > @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) > bs->bl.opt_mem_alignment = s->buf_align; > } > > +static int check_for_dasd(int fd) > +{ > +#ifdef BIODASDINFO2 > + struct dasd_information2_t info = {0}; > + > + return ioctl(fd, BIODASDINFO2, &info); > +#endif > + return -1; > +} > + > +/* > + * Try to get the device blocksize. On success 0. On failure return -errno. > + * Currently only implemented for DASD drives. > + */ Compare to the function contract I wrote for the callback in my review of PATCH 1. If you like that one better, feel free to reuse it here. > +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > +{ > + BDRVRawState *s = bs->opaque; > + int ret; > + > + /* If DASD, get blocksizes */ > + if (check_for_dasd(s->fd) < 0) { > + return -1; This is not a negative error code! -ENOTSUP. > + } > + ret = probe_logical_blocksize(s->fd, &bsz->log); > + if (ret < 0) { > + return ret; > + } > + return probe_physical_blocksize(s->fd, &bsz->phys); > +} > + > +/* > + * Try to get the device geometry. On success 0. On failure return -errno. > + * Currently only implemented for DASD drives. > + */ Compare to the function contract I wrote for the callback in my review of PATCH 1. If you like that one better, feel free to reuse it here. > +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo) > +{ > + BDRVRawState *s = bs->opaque; > + struct hd_geometry ioctl_geo = {0}; > + uint32_t blksize; > + > + /* If DASD, get it's geometry */ > + if (check_for_dasd(s->fd) < 0) { > + return -1; This is not a negative error code! -ENOTSUP. > + } > + if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) { > + return -errno; > + } > + /* HDIO_GETGEO may return success even though geo contains zeros > + (e.g. certain multipath setups) */ > + if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) { > + return -1; Need an error code. > + } > + /* Do not return a geometry for partition */ > + if (ioctl_geo.start != 0) { > + return -1; Need an error code. > + } > + geo->heads = ioctl_geo.heads; > + geo->sectors = ioctl_geo.sectors; > + if (bs->total_sectors) { > + if (!probe_physical_blocksize(s->fd, &blksize)) { > + geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE) > + / (geo->heads * geo->sectors); > + return 0; > + } > + } How could !bs->total_sectors happen? > + geo->cylinders = ioctl_geo.cylinders; > + > + return 0; > +} > + > static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb) > { > int ret; > @@ -2127,6 +2216,8 @@ static BlockDriver bdrv_host_device = { > .bdrv_get_info = raw_get_info, > .bdrv_get_allocated_file_size > = raw_get_allocated_file_size, > + .bdrv_probe_blocksizes = hdev_probe_blocksizes, > + .bdrv_probe_geometry = hdev_probe_geometry, > > .bdrv_detach_aio_context = raw_detach_aio_context, > .bdrv_attach_aio_context = raw_attach_aio_context, > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index 401b967..cfd5249 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) > return 1; > } > > +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > +{ > + bdrv_probe_blocksizes(bs->file, bsz); > + > + return 0; > +} Correct, just a bit awkward due to the difference between bdrv_probe_blocksizes() and ->bdrv_probe_blocksizes(). I guess I'd make them the same, but it's your patch. > + > +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) > +{ > + return bdrv_probe_geometry(bs->file, geo); > +} > + > static BlockDriver bdrv_raw = { > .format_name = "raw", > .bdrv_probe = &raw_probe, > @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { > .has_variable_length = true, > .bdrv_get_info = &raw_get_info, > .bdrv_refresh_limits = &raw_refresh_limits, > + .bdrv_probe_blocksizes = &raw_probe_blocksizes, > + .bdrv_probe_geometry = &raw_probe_geometry, > .bdrv_is_inserted = &raw_is_inserted, > .bdrv_media_changed = &raw_media_changed, > .bdrv_eject = &raw_eject,
On 12/15/2014 04:29 PM, Markus Armbruster wrote: > Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > >> This patch introduces driver methods of defining disk blocksizes >> (physical and logical) and hard drive geometry. >> The method is only implemented for "host_device". For "raw" devices >> driver calls child's method. >> >> For the time being geometry detection will only work for DASD devices. >> In order to check that a local check_for_dasd function was introduced, >> which calls BIODASDINFO2 ioctl and returns its rc. >> >> Blocksizes detection fuction will probe sizes for DASD devices and >> set default for other devices. >> >> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> >> --- >> block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/raw_bsd.c | 14 +++++++++ >> 2 files changed, 105 insertions(+) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 633d5bc..33f9983 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -56,6 +56,7 @@ >> #include <linux/cdrom.h> >> #include <linux/fd.h> >> #include <linux/fs.h> >> +#include <linux/hdreg.h> >> #ifndef FS_NOCOW_FL >> #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> #endif >> @@ -90,6 +91,10 @@ >> #include <xfs/xfs.h> >> #endif >> >> +#ifdef __s390__ >> +#include <asm/dasd.h> >> +#endif >> + >> //#define DEBUG_FLOPPY >> >> //#define DEBUG_BLOCK >> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size) >> return 0; >> } >> >> +/* >> + * Set physical block size via ioctl. On success return 0. Otherwise -errno. >> + */ > > Set? > > Ah, now I get your logic, you mean "set *blk_size"! > > Suggest to say it like this: > > /** > * Get physical block size of @fd. > * On success, store it in @blk_size and return 0. > * On failure, return -errno. > */ > >> +static int probe_physical_blocksize(int fd, unsigned int *blk_size) >> +{ >> +#ifdef BLKPBSZGET >> + if (ioctl(fd, BLKPBSZGET, blk_size) < 0) { >> + return -errno; >> + } >> +#endif >> + >> + return 0; > > If !defined(BLKPBSZGET), you return 0 without setting *blk_size. I > think you need to fail then. -ENOTSUP should do. > >> +} >> + >> static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) >> { >> BDRVRawState *s = bs->opaque; >> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) >> bs->bl.opt_mem_alignment = s->buf_align; >> } >> >> +static int check_for_dasd(int fd) >> +{ >> +#ifdef BIODASDINFO2 >> + struct dasd_information2_t info = {0}; >> + >> + return ioctl(fd, BIODASDINFO2, &info); >> +#endif >> + return -1; >> +} >> + >> +/* >> + * Try to get the device blocksize. On success 0. On failure return -errno. >> + * Currently only implemented for DASD drives. >> + */ > > Compare to the function contract I wrote for the callback in my review > of PATCH 1. If you like that one better, feel free to reuse it here. > >> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) >> +{ >> + BDRVRawState *s = bs->opaque; >> + int ret; >> + >> + /* If DASD, get blocksizes */ >> + if (check_for_dasd(s->fd) < 0) { >> + return -1; > > This is not a negative error code! -ENOTSUP. > >> + } >> + ret = probe_logical_blocksize(s->fd, &bsz->log); >> + if (ret < 0) { >> + return ret; >> + } >> + return probe_physical_blocksize(s->fd, &bsz->phys); >> +} >> + >> +/* >> + * Try to get the device geometry. On success 0. On failure return -errno. >> + * Currently only implemented for DASD drives. >> + */ > > Compare to the function contract I wrote for the callback in my review > of PATCH 1. If you like that one better, feel free to reuse it here. > >> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo) >> +{ >> + BDRVRawState *s = bs->opaque; >> + struct hd_geometry ioctl_geo = {0}; >> + uint32_t blksize; >> + >> + /* If DASD, get it's geometry */ >> + if (check_for_dasd(s->fd) < 0) { >> + return -1; > > This is not a negative error code! -ENOTSUP. > >> + } >> + if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) { >> + return -errno; >> + } >> + /* HDIO_GETGEO may return success even though geo contains zeros >> + (e.g. certain multipath setups) */ >> + if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) { >> + return -1; > > Need an error code. > >> + } >> + /* Do not return a geometry for partition */ >> + if (ioctl_geo.start != 0) { >> + return -1; > > Need an error code. > >> + } >> + geo->heads = ioctl_geo.heads; >> + geo->sectors = ioctl_geo.sectors; >> + if (bs->total_sectors) { >> + if (!probe_physical_blocksize(s->fd, &blksize)) { >> + geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE) >> + / (geo->heads * geo->sectors); >> + return 0; >> + } >> + } > > How could !bs->total_sectors happen? > >> + geo->cylinders = ioctl_geo.cylinders; >> + >> + return 0; >> +} >> + >> static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb) >> { >> int ret; >> @@ -2127,6 +2216,8 @@ static BlockDriver bdrv_host_device = { >> .bdrv_get_info = raw_get_info, >> .bdrv_get_allocated_file_size >> = raw_get_allocated_file_size, >> + .bdrv_probe_blocksizes = hdev_probe_blocksizes, >> + .bdrv_probe_geometry = hdev_probe_geometry, >> >> .bdrv_detach_aio_context = raw_detach_aio_context, >> .bdrv_attach_aio_context = raw_attach_aio_context, >> diff --git a/block/raw_bsd.c b/block/raw_bsd.c >> index 401b967..cfd5249 100644 >> --- a/block/raw_bsd.c >> +++ b/block/raw_bsd.c >> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) >> return 1; >> } >> >> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) >> +{ >> + bdrv_probe_blocksizes(bs->file, bsz); >> + >> + return 0; >> +} > > Correct, just a bit awkward due to the difference between > bdrv_probe_blocksizes() and ->bdrv_probe_blocksizes(). I guess I'd make > them the same, but it's your patch. > I think would stick with my approach. You mentioned during review of v2, that blk_probe_blocksizes should always just work. And I think it's right. An error code from a method allows me to set a default in the wrapper. So wrapper will never fail. Thanks a lot for your review! I'll try to post a new version ASAP. >> + >> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) >> +{ >> + return bdrv_probe_geometry(bs->file, geo); >> +} >> + >> static BlockDriver bdrv_raw = { >> .format_name = "raw", >> .bdrv_probe = &raw_probe, >> @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { >> .has_variable_length = true, >> .bdrv_get_info = &raw_get_info, >> .bdrv_refresh_limits = &raw_refresh_limits, >> + .bdrv_probe_blocksizes = &raw_probe_blocksizes, >> + .bdrv_probe_geometry = &raw_probe_geometry, >> .bdrv_is_inserted = &raw_is_inserted, >> .bdrv_media_changed = &raw_media_changed, >> .bdrv_eject = &raw_eject, >
diff --git a/block/raw-posix.c b/block/raw-posix.c index 633d5bc..33f9983 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -56,6 +56,7 @@ #include <linux/cdrom.h> #include <linux/fd.h> #include <linux/fs.h> +#include <linux/hdreg.h> #ifndef FS_NOCOW_FL #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ #endif @@ -90,6 +91,10 @@ #include <xfs/xfs.h> #endif +#ifdef __s390__ +#include <asm/dasd.h> +#endif + //#define DEBUG_FLOPPY //#define DEBUG_BLOCK @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size) return 0; } +/* + * Set physical block size via ioctl. On success return 0. Otherwise -errno. + */ +static int probe_physical_blocksize(int fd, unsigned int *blk_size) +{ +#ifdef BLKPBSZGET + if (ioctl(fd, BLKPBSZGET, blk_size) < 0) { + return -errno; + } +#endif + + return 0; +} + static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) { BDRVRawState *s = bs->opaque; @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) bs->bl.opt_mem_alignment = s->buf_align; } +static int check_for_dasd(int fd) +{ +#ifdef BIODASDINFO2 + struct dasd_information2_t info = {0}; + + return ioctl(fd, BIODASDINFO2, &info); +#endif + return -1; +} + +/* + * Try to get the device blocksize. On success 0. On failure return -errno. + * Currently only implemented for DASD drives. + */ +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) +{ + BDRVRawState *s = bs->opaque; + int ret; + + /* If DASD, get blocksizes */ + if (check_for_dasd(s->fd) < 0) { + return -1; + } + ret = probe_logical_blocksize(s->fd, &bsz->log); + if (ret < 0) { + return ret; + } + return probe_physical_blocksize(s->fd, &bsz->phys); +} + +/* + * Try to get the device geometry. On success 0. On failure return -errno. + * Currently only implemented for DASD drives. + */ +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo) +{ + BDRVRawState *s = bs->opaque; + struct hd_geometry ioctl_geo = {0}; + uint32_t blksize; + + /* If DASD, get it's geometry */ + if (check_for_dasd(s->fd) < 0) { + return -1; + } + if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) { + return -errno; + } + /* HDIO_GETGEO may return success even though geo contains zeros + (e.g. certain multipath setups) */ + if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) { + return -1; + } + /* Do not return a geometry for partition */ + if (ioctl_geo.start != 0) { + return -1; + } + geo->heads = ioctl_geo.heads; + geo->sectors = ioctl_geo.sectors; + if (bs->total_sectors) { + if (!probe_physical_blocksize(s->fd, &blksize)) { + geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE) + / (geo->heads * geo->sectors); + return 0; + } + } + geo->cylinders = ioctl_geo.cylinders; + + return 0; +} + static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb) { int ret; @@ -2127,6 +2216,8 @@ static BlockDriver bdrv_host_device = { .bdrv_get_info = raw_get_info, .bdrv_get_allocated_file_size = raw_get_allocated_file_size, + .bdrv_probe_blocksizes = hdev_probe_blocksizes, + .bdrv_probe_geometry = hdev_probe_geometry, .bdrv_detach_aio_context = raw_detach_aio_context, .bdrv_attach_aio_context = raw_attach_aio_context, diff --git a/block/raw_bsd.c b/block/raw_bsd.c index 401b967..cfd5249 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) return 1; } +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) +{ + bdrv_probe_blocksizes(bs->file, bsz); + + return 0; +} + +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) +{ + return bdrv_probe_geometry(bs->file, geo); +} + static BlockDriver bdrv_raw = { .format_name = "raw", .bdrv_probe = &raw_probe, @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { .has_variable_length = true, .bdrv_get_info = &raw_get_info, .bdrv_refresh_limits = &raw_refresh_limits, + .bdrv_probe_blocksizes = &raw_probe_blocksizes, + .bdrv_probe_geometry = &raw_probe_geometry, .bdrv_is_inserted = &raw_is_inserted, .bdrv_media_changed = &raw_media_changed, .bdrv_eject = &raw_eject,
This patch introduces driver methods of defining disk blocksizes (physical and logical) and hard drive geometry. The method is only implemented for "host_device". For "raw" devices driver calls child's method. For the time being geometry detection will only work for DASD devices. In order to check that a local check_for_dasd function was introduced, which calls BIODASDINFO2 ioctl and returns its rc. Blocksizes detection fuction will probe sizes for DASD devices and set default for other devices. Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> --- block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ block/raw_bsd.c | 14 +++++++++ 2 files changed, 105 insertions(+)