|
From: | Denis V. Lunev |
Subject: | Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files |
Date: | Mon, 2 Feb 2015 17:38:41 +0300 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 02/02/15 17:20, Peter Lieven wrote:
Am 02.02.2015 um 15:16 schrieb Kevin Wolf:Am 02.02.2015 um 15:12 hat Peter Lieven geschrieben:Am 02.02.2015 um 15:04 schrieb Kevin Wolf:Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:Well, splitting it up doesn't make it any faster. I think we can assumeAm 02.02.2015 um 14:23 schrieb Kevin Wolf:Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:fallocate() works fine and could handle properly with arbitrary size requests. There is no sense to reduce the amount of space to fallocate. The bigger is the size, the better is the performance as the amount ofPeter, do you remember why INT_MAX isn't actually the default? I think the most reasonable behaviour would be that a limitation is only used ifjournal updates is reduced.The patch changes behavior for both generic filesystem and XFS codepaths, which are different in handle_aiocb_write_zeroes. The implementation of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the samethus the change is fine for both ways. Signed-off-by: Denis V. Lunev <address@hidden> Reviewed-by: Max Reitz <address@hidden> CC: Kevin Wolf <address@hidden> CC: Stefan Hajnoczi <address@hidden> CC: Peter Lieven <address@hidden> CC: Fam Zheng <address@hidden> --- block/raw-posix.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/block/raw-posix.c b/block/raw-posix.c index 7b42f37..933c778 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c@@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)} } +static void raw_probe_max_write_zeroes(BlockDriverState *bs) +{ + BDRVRawState *s = bs->opaque; + struct stat st; + + if (fstat(s->fd, &st) < 0) { + return; /* no problem, keep default value */ + } + if (!S_ISREG(st.st_mode) || !s->discard_zeroes) { + return; + } + bs->bl.max_write_zeroes = INT_MAX; +}a block driver requests it, and otherwise unlimited is assumed.The default (0) actually means unlimited or undefined. We introduced that limit of 16MB in bdrv_co_write_zeroes to create only reasonable sized requests because there is no guarantee that write zeroes is a fast operation. We should set INT_MAX only if we know that write zeroes of an arbitrary size is always fast.that drv->bdrv_co_write_zeroes() wants to know the full request size unless the driver has explicitly set bs->bl.max_write_zeroes.You mean sth like this:Yes, I think that's what I meant.I can't find the original discussion why we added this limit. It was actually the default before we introduced BlockLimits. And, it was also the default in the unsupported path of write zeroes which created big memory allocations. This might be the reason whywe introduced a limit. Peter
my $0.02 here is that even if the patch below adds regression (though I can not imagine how at the moment after some checking), we should fix bogus driver. Personally I do not like such unnatural limitations. Den
Kevindiff --git a/block.c b/block.c index 61412e9..8272ef9 100644 --- a/block.c +++ b/block.c@@ -3192,10 +3192,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,BDRV_REQ_COPY_ON_READ); } -/* if no limit is specified in the BlockLimits use a default - * of 32768 512-byte sectors (16 MiB) per request. - */ -#define MAX_WRITE_ZEROES_DEFAULT 32768 +#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)@@ -3206,7 +3203,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,int ret = 0; int max_write_zeroes = bs->bl.max_write_zeroes ?- bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;+ bs->bl.max_write_zeroes : INT_MAX; while (nb_sectors > 0 && !ret) { int num = nb_sectors;@@ -3242,7 +3239,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,if (ret == -ENOTSUP) {/* Fall back to bounce buffer if write zeroes is unsupported */ int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,- MAX_WRITE_ZEROES_DEFAULT); + MAX_WRITE_ZEROES_BOUNCE_BUFFER); num = MIN(num, max_xfer_len); iov.iov_len = num * BDRV_SECTOR_SIZE; if (iov.iov_base == NULL) {@@ -5099,11 +5096,6 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque) rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);} -/* if no limit is specified in the BlockLimits use a default - * of 32768 512-byte sectors (16 MiB) per request. - */ -#define MAX_DISCARD_DEFAULT 32768 -int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,int nb_sectors) {@@ -5128,7 +5120,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,return 0; }- max_discard = bs->bl.max_discard ? bs->bl.max_discard : MAX_DISCARD_DEFAULT;+ max_discard = bs->bl.max_discard ? bs->bl.max_discard : INT_MAX; while (nb_sectors > 0) { int ret; int num = nb_sectors; Peter
[Prev in Thread] | Current Thread | [Next in Thread] |