Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> Move the IOCTL calls that detect logical blocksize from raw_probe_alignment
> into separate function (probe_logical_blocksize).
> Introduce function which detect physical blocksize via IOCTL
> (probe_physical_blocksize).
> Both functions will be used in the next patch.
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>

>From what I can tell this should be a no-op for raw_probe_alignment.

probe_physical_blocksize looks also good. When this patch is applied stand-alone,
gcc will complain about a defined but unused function, though.

So we might want to move this function into patch 3 or just add an __attribute__((unused))
here (and remove that in patch 3). Or just leave it as is.

Otherwise
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e100ae2..45f1d79 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
> 
> -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd)
>  {
> -    BDRVRawState *s = bs->opaque;
> -    char *buf;
> -    unsigned int sector_size;
> -
> -    /* For /dev/sg devices the alignment is not really used.
> -       With buffered I/O, we don't have any restrictions. */
> -    if (bs->sg || !s->needs_alignment) {
> -        bs->request_alignment = 1;
> -        s->buf_align = 1;
> -        return;
> -    }
> +    unsigned int sector_size = 0;
> 
>      /* Try a few ioctls to get the right size */
> -    bs->request_alignment = 0;
> -    s->buf_align = 0;
> -
>  #ifdef BLKSSZGET
>      if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DKIOCGETBLOCKSIZE
>      if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DIOCGSECTORSIZE
>      if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef CONFIG_XFS
>      if (s->is_xfs) {
>          struct dioattr da;
>          if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
> -            bs->request_alignment = da.d_miniosz;
> +            sector_size = da.d_miniosz;
>              /* The kernel returns wrong information for d_mem */
>              /* s->buf_align = da.d_mem; */
> +            return sector_size;
>          }
>      }
>  #endif
> 
> +    return 0;
> +}
> +
> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)
> +{
> +    unsigned int blk_size = 0;
> +#ifdef BLKPBSZGET
> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
> +        return blk_size;
> +    }
> +#endif
> +
> +    return 0;
> +}
> +
> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    char *buf;
> +
> +    /* For /dev/sg devices the alignment is not really used.
> +       With buffered I/O, we don't have any restrictions. */
> +    if (bs->sg || !s->needs_alignment) {
> +        bs->request_alignment = 1;
> +        s->buf_align = 1;
> +        return;
> +    }
> +
> +    s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
> +
>      /* If we could not get the sizes so far, we can only guess them */
>      if (!s->buf_align) {
>          size_t align;
>