From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5BSj-00042s-He for qemu-devel@nongnu.org; Mon, 20 Jan 2014 04:48:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W5BSb-0000p5-3s for qemu-devel@nongnu.org; Mon, 20 Jan 2014 04:48:09 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:41921 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5BSa-0000oW-P5 for qemu-devel@nongnu.org; Mon, 20 Jan 2014 04:48:01 -0500 Message-ID: <52DCF13E.5070508@kamp.de> Date: Mon, 20 Jan 2014 10:49:50 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1389968119-24771-1-git-send-email-kwolf@redhat.com> <1389968119-24771-2-git-send-email-kwolf@redhat.com> <20140117222653.GA7739@irqsave.net> <20140120093103.GB3267@dhcp-200-207.str.redhat.com> In-Reply-To: <20140120093103.GB3267@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 01/29] block: Move initialisation of BlockLimits to bdrv_refresh_limits() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , =?ISO-8859-1?Q?Beno=EEt_Canet?= Cc: pbonzini@redhat.com, xiawenc@linux.vnet.ibm.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com On 20.01.2014 10:31, Kevin Wolf wrote: > Am 17.01.2014 um 23:39 hat Benoît Canet geschrieben: >> Le Friday 17 Jan 2014 à 15:14:51 (+0100), Kevin Wolf a écrit : >>> This function separates filling the BlockLimits from bdrv_open(), which >>> allows it to call it from other operations which may change the limits >>> (e.g. modifications to the backing file chain or bdrv_reopen) >>> >>> Signed-off-by: Kevin Wolf >>> Reviewed-by: Max Reitz >>> --- >>> block.c | 19 +++++++++++++++++++ >>> block/iscsi.c | 46 +++++++++++++++++++++++++++++----------------- >>> block/qcow2.c | 11 ++++++++++- >>> block/qed.c | 11 ++++++++++- >>> block/vmdk.c | 22 ++++++++++++++++++---- >>> include/block/block_int.h | 2 ++ >>> 6 files changed, 88 insertions(+), 23 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 64e7d22..99e69da 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -479,6 +479,19 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options, >>> return ret; >>> } >>> >>> +static int bdrv_refresh_limits(BlockDriverState *bs) >>> +{ >>> + BlockDriver *drv = bs->drv; >>> + >>> + memset(&bs->bl, 0, sizeof(bs->bl)); >>> + >>> + if (drv && drv->bdrv_refresh_limits) { >>> + return drv->bdrv_refresh_limits(bs); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> /* >>> * Create a uniquely-named empty temporary file. >>> * Return 0 upon success, otherwise a negative errno value. >>> @@ -833,6 +846,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, >>> goto free_and_fail; >>> } >>> >>> + bdrv_refresh_limits(bs); >>> + >>> #ifndef _WIN32 >>> if (bs->is_temporary) { >>> assert(bs->filename[0] != '\0'); >>> @@ -1018,6 +1033,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) >>> } >>> pstrcpy(bs->backing_file, sizeof(bs->backing_file), >>> bs->backing_hd->file->filename); >>> + >>> + /* Recalculate the BlockLimits with the backing file */ >>> + bdrv_refresh_limits(bs); >>> + >>> return 0; >>> } >>> >>> diff --git a/block/iscsi.c b/block/iscsi.c >>> index c0ea0c4..3202dc5 100644 >>> --- a/block/iscsi.c >>> +++ b/block/iscsi.c >>> @@ -1265,23 +1265,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, >>> sizeof(struct scsi_inquiry_block_limits)); >>> scsi_free_scsi_task(task); >>> task = NULL; >>> - >>> - if (iscsilun->bl.max_unmap < 0xffffffff) { >>> - bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, >>> - iscsilun); >>> - } >>> - bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> - iscsilun); >>> - >>> - if (iscsilun->bl.max_ws_len < 0xffffffff) { >>> - bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, >>> - iscsilun); >>> - } >>> - bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> - iscsilun); >>> - >>> - bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len, >>> - iscsilun); >>> } >>> >>> #if defined(LIBISCSI_FEATURE_NOP_COUNTER) >>> @@ -1326,6 +1309,34 @@ static void iscsi_close(BlockDriverState *bs) >>> memset(iscsilun, 0, sizeof(IscsiLun)); >>> } >>> >>> +static int iscsi_refresh_limits(BlockDriverState *bs) >>> +{ >>> + IscsiLun *iscsilun = bs->opaque; >>> + >>> + /* We don't actually refresh here, but just return data queried in >>> + * iscsi_open(): iscsi targets don't change their limits. */ >>> + if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) { >>> + if (iscsilun->bl.max_unmap < 0xffffffff) { >>> + bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, >>> + iscsilun); >>> + } >>> + bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> + iscsilun); >>> + >>> + if (iscsilun->bl.max_ws_len < 0xffffffff) { >>> + bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, >>> + iscsilun); >>> + } >>> + bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> + iscsilun); >>> + >>> + bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len, >>> + iscsilun); >> Why is bl.opt_transfer_length filled only inside the test for unmap and write same ? >> Is there a relationship ? > Good question. The only thing I can say is that I'm moving existing code > here, so this aspect doesnt' change. I suspect this needs a fix on top > of this series. Peter? Yes, the opt_transfer_length is independend. It got there because, if either lbp.lbpu or lbp.lbpws is set, we can be sure that the Block Limits page is supported. I will change this when Kevins series is merged. Peter