From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vg9iX-0005L7-93 for qemu-devel@nongnu.org; Tue, 12 Nov 2013 03:53:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vg9iR-0000ZT-El for qemu-devel@nongnu.org; Tue, 12 Nov 2013 03:53:01 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:46755 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vg9iR-0000Z5-3u for qemu-devel@nongnu.org; Tue, 12 Nov 2013 03:52:55 -0500 Message-ID: <5281EC6F.4060106@kamp.de> Date: Tue, 12 Nov 2013 09:53:03 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1382609227-23989-1-git-send-email-pl@kamp.de> <1382609227-23989-11-git-send-email-pl@kamp.de> <20131111132020.GD3046@dhcp-200-207.str.redhat.com> In-Reply-To: <20131111132020.GD3046@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv7 10/17] block: honour BlockLimits in bdrv_co_discard List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, ronniesahlberg@gmail.com, qemu-devel@nongnu.org, stefanha@redhat.com On 11.11.2013 14:20, Kevin Wolf wrote: > Am 24.10.2013 um 12:06 hat Peter Lieven geschrieben: >> Reviewed-by: Eric Blake >> Signed-off-by: Peter Lieven >> --- >> block.c | 37 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 36 insertions(+), 1 deletion(-) >> >> diff --git a/block.c b/block.c >> index 0c0b0ac..b28dd42 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4234,6 +4234,11 @@ 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) >> { >> @@ -4255,7 +4260,37 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, >> } >> >> if (bs->drv->bdrv_co_discard) { >> - return bs->drv->bdrv_co_discard(bs, sector_num, nb_sectors); >> + int max_discard = bs->bl.max_discard ? >> + bs->bl.max_discard : MAX_DISCARD_DEFAULT; >> + >> + while (nb_sectors > 0) { >> + int ret; >> + int num = nb_sectors; >> + >> + /* align request */ >> + if (bs->bl.discard_alignment && >> + num >= bs->bl.discard_alignment && >> + sector_num % bs->bl.discard_alignment) { >> + if (num > bs->bl.discard_alignment) { >> + num = bs->bl.discard_alignment; >> + } >> + num -= sector_num % bs->bl.discard_alignment; >> + } >> + >> + /* limit request size */ >> + if (num > max_discard) { >> + num = max_discard; >> + } >> + >> + ret = bs->drv->bdrv_co_discard(bs, sector_num, num); >> + if (ret) { >> + return ret; >> + } >> + >> + sector_num += num; >> + nb_sectors -= num; >> + } >> + return 0; >> } else if (bs->drv->bdrv_aio_discard) { >> BlockDriverAIOCB *acb; >> CoroutineIOCompletion co = { > You're only optimising drivers which have a .bdrv_co_discard() > implementation, but not those with .bdrv_aio_discard(). Not very nice, > and it would have been easy to avoid this by putting the loop around the > whole if statement instead of inside the 'then' branch. Good point. I wonder noone noticed before ;-) Do you want me to respin or is ok to send a follow up patch? Stefan has it already in block-next. This patch doesn't make the situation worse and we need follow up patches for all the drivers to supply alignment information anyway. Peter