Re: [Qemu-devel] [PATCH 1/2] block, raw-posix: replace 512/4096 constants with proper macros/values

[Kevin Wolf <kwolf@redhat.com>]

Am 06.02.2015 um 18:37 hat Denis V. Lunev geschrieben:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c           | 10 +++++-----
>  block/raw-posix.c | 16 ++++++++--------
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d45e4dd..e98d651 100644
> --- a/block.c
> +++ b/block.c
> @@ -225,8 +225,8 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
>  size_t bdrv_opt_mem_align(BlockDriverState *bs)
>  {
>      if (!bs || !bs->drv) {
> -        /* 4k should be on the safe side */
> -        return 4096;
> +        /* page size should be on the safe side */
> +        return getpagesize();

Can we make this MAX(4096, getpagesize())? The 4k aren't chosen because
of buffer alignment in RAM, but because of a disk sector size of 4k is
the highest common value.

>      }
>  
>      return bs->bl.opt_mem_alignment;
> @@ -543,7 +543,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>          bs->bl.max_transfer_length = bs->file->bl.max_transfer_length;
>          bs->bl.opt_mem_alignment = bs->file->bl.opt_mem_alignment;
>      } else {
> -        bs->bl.opt_mem_alignment = 512;
> +        bs->bl.opt_mem_alignment = BDRV_SECTOR_SIZE;

I wouldn't make this conversion. The value happens to be the same, but
BDRV_SECTOR_SIZE is just the arbitrary unit that the block layer uses
internally for things like sector_num/nb_sectors.

The 512 in this place, however, is the minimum alignment that hardware
could require, and logically independent of BDRV_SECTOR_SIZE.

Similar considerations apply to the other conversions of 512 in this
patch. They would all be better left as literal 512 (unless you want to
introduce new constants for their specific purpose).

>      }
>  
>      if (bs->backing_hd) {
> @@ -965,8 +965,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>      }
>  
>      bs->open_flags = flags;
> -    bs->guest_block_size = 512;
> -    bs->request_alignment = 512;
> +    bs->guest_block_size = BDRV_SECTOR_SIZE;
> +    bs->request_alignment = BDRV_SECTOR_SIZE;
>      bs->zero_beyond_eof = true;
>      open_flags = bdrv_open_flags(bs, flags);
>      bs->read_only = !(open_flags & BDRV_O_RDWR);
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 853ffa6..9848f83 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -122,7 +122,6 @@
>     reopen it to see if the disk has been changed */
>  #define FD_OPEN_TIMEOUT (1000000000)
>  
> -#define MAX_BLOCKSIZE	4096
>  
>  typedef struct BDRVRawState {
>      int fd;
> @@ -222,6 +221,7 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>      BDRVRawState *s = bs->opaque;
>      char *buf;
>      unsigned int sector_size;
> +    size_t page_size = getpagesize();
>  
>      /* For /dev/sg devices the alignment is not really used.
>         With buffered I/O, we don't have any restrictions. */
> @@ -264,9 +264,9 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>      /* If we could not get the sizes so far, we can only guess them */
>      if (!s->buf_align) {
>          size_t align;
> -        buf = qemu_memalign(MAX_BLOCKSIZE, 2 * MAX_BLOCKSIZE);
> -        for (align = 512; align <= MAX_BLOCKSIZE; align <<= 1) {
> -            if (pread(fd, buf + align, MAX_BLOCKSIZE, 0) >= 0) {
> +        buf = qemu_memalign(page_size, 2 * page_size);
> +        for (align = BDRV_SECTOR_SIZE; align <= page_size; align <<= 1) {
> +            if (pread(fd, buf + align, page_size, 0) >= 0) {
>                  s->buf_align = align;
>                  break;
>              }

Here, I'd prefer MAX(getpagesize(), MAX_BLOCKSIZE) as well.

Kevin