From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59829) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xz1kZ-0005uv-WB for qemu-devel@nongnu.org; Thu, 11 Dec 2014 06:17:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xz1kQ-0000g0-RT for qemu-devel@nongnu.org; Thu, 11 Dec 2014 06:17:39 -0500 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:46688) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xz1kQ-0000fl-FU for qemu-devel@nongnu.org; Thu, 11 Dec 2014 06:17:30 -0500 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Dec 2014 11:17:26 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 0330D1B08040 for ; Thu, 11 Dec 2014 11:17:45 +0000 (GMT) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sBBBHNnl62521540 for ; Thu, 11 Dec 2014 11:17:23 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sBBBHMZ0000839 for ; Thu, 11 Dec 2014 04:17:22 -0700 Message-ID: <54897D41.5090106@linux.vnet.ibm.com> Date: Thu, 11 Dec 2014 14:17:21 +0300 From: Ekaterina Tumanova MIME-Version: 1.0 References: <1417802181-20834-1-git-send-email-tumanova@linux.vnet.ibm.com> <1417802181-20834-4-git-send-email-tumanova@linux.vnet.ibm.com> <20141210141458.5adf276b@oc7435384737.ibm.com> In-Reply-To: <20141210141458.5adf276b@oc7435384737.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/5] block: Add driver methods to probe blocksizes and geometry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: kwolf@redhat.com, dahi@linux.vnet.ibm.com, Public KVM Mailing List , armbru@redhat.com, mihajlov@linux.vnet.ibm.com, borntraeger@de.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com On 12/10/2014 04:14 PM, Thomas Huth wrote: > On Fri, 5 Dec 2014 18:56:19 +0100 > Ekaterina Tumanova 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 >> --- >> 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 >> #include >> #include >> +#include >> #ifndef FS_NOCOW_FL >> #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> #endif >> @@ -90,6 +91,10 @@ >> #include >> #endif >> >> +#ifdef __s390__ >> +#include >> +#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.