From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42879) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNJ1x-0008Uu-28 for qemu-devel@nongnu.org; Mon, 16 Feb 2015 05:35:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNJ1t-0000uQ-04 for qemu-devel@nongnu.org; Mon, 16 Feb 2015 05:35:57 -0500 Received: from mx2.parallels.com ([199.115.105.18]:42918) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNJ1s-0000G9-Pj for qemu-devel@nongnu.org; Mon, 16 Feb 2015 05:35:52 -0500 Message-ID: <54E1C7A3.3040001@openvz.org> Date: Mon, 16 Feb 2015 13:34:11 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1423244272-24887-1-git-send-email-den@openvz.org> <1423244272-24887-2-git-send-email-den@openvz.org> <20150216103230.GG4079@noname.str.redhat.com> In-Reply-To: <20150216103230.GG4079@noname.str.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] block, raw-posix: replace 512/4096 constants with proper macros/values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Paolo Bonzini , qemu-devel@nongnu.org On 16/02/15 13:32, Kevin Wolf wrote: > Am 06.02.2015 um 18:37 hat Denis V. Lunev geschrieben: >> Signed-off-by: Denis V. Lunev >> CC: Paolo Bonzini >> CC: Kevin Wolf >> --- >> 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 this makes sense. OK