From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R77oB-0002jf-Ie for qemu-devel@nongnu.org; Fri, 23 Sep 2011 11:37:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R77oA-00060O-C3 for qemu-devel@nongnu.org; Fri, 23 Sep 2011 11:36:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3935) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R77oA-00060J-3L for qemu-devel@nongnu.org; Fri, 23 Sep 2011 11:36:58 -0400 From: Markus Armbruster References: <1316715584-25557-1-git-send-email-lcapitulino@redhat.com> <1316715584-25557-2-git-send-email-lcapitulino@redhat.com> <20110923110100.1f72f0c6@doriath> Date: Fri, 23 Sep 2011 17:36:53 +0200 In-Reply-To: <20110923110100.1f72f0c6@doriath> (Luiz Capitulino's message of "Fri, 23 Sep 2011 11:01:00 -0300") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, zwu.kernel@gmail.com, qemu-devel@nongnu.org Luiz Capitulino writes: > On Fri, 23 Sep 2011 09:51:24 +0200 > Markus Armbruster wrote: > >> Luiz Capitulino writes: >> >> > This commit adds support to the BlockDriverState type to keep track >> > of devices' I/O status. >> > >> > There are three possible status: BDRV_IOS_OK (no error), BDRV_IOS_ENOSPC >> > (no space error) and BDRV_IOS_FAILED (any other error). The distinction >> > between no space and other errors is important because a management >> > application may want to watch for no space in order to extend the >> > space assigned to the VM and put it to run again. >> > >> > Qemu devices supporting the I/O status feature have to enable it >> > explicitly by calling bdrv_iostatus_enable() _and_ have to be >> > configured to stop the VM on errors (ie. werror=stop|enospc or >> > rerror=stop). >> > >> > In case of multiple errors being triggered in sequence only the first >> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the >> > 'cont' command is issued. >> > >> > Next commits will add support to some devices and extend the >> > query-block/info block commands to return the I/O status information. >> > >> > Signed-off-by: Luiz Capitulino >> > --- >> > block.c | 32 ++++++++++++++++++++++++++++++++ >> > block.h | 9 +++++++++ >> > block_int.h | 1 + >> > monitor.c | 6 ++++++ >> > 4 files changed, 48 insertions(+), 0 deletions(-) >> > >> > diff --git a/block.c b/block.c >> > index e3fe97f..fbd90b4 100644 >> > --- a/block.c >> > +++ b/block.c >> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char *device_name) >> > if (device_name[0] != '\0') { >> > QTAILQ_INSERT_TAIL(&bdrv_states, bs, list); >> > } >> > + bs->iostatus = BDRV_IOS_INVAL; >> > return bs; >> > } >> > >> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs) >> > return bs->in_use; >> > } >> > >> > +void bdrv_iostatus_enable(BlockDriverState *bs) >> > +{ >> > + assert(bs->iostatus == BDRV_IOS_INVAL); >> > + bs->iostatus = BDRV_IOS_OK; >> > +} >> >> bdrv_new() creates the BDS with I/O status tracking disabled. Devices >> that do tracking declare that by calling this function during >> initialization. Enables tracking if the BDS has suitable on_write_error >> and on_read_error. >> >> If a device gets hot unplugged, tracking remains enabled. If the BDS >> then gets reused with a device that doesn't do tracking, I/O status >> becomes incorrect. Can't happen right now, because we automatically >> delete the BDS on hot unplug, but it's a trap. >> >> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev(). > > Ok, and in bdrv_close() as Zhi Yong said. Disabling in bdrv_close() makes the status go away on eject. Makes some sense, in a way. Also complicates the simple rule "status persists from stop to cont". Hmm, consider the following race: 1. Management app sends eject command 2. qemu gets read error on the same drive, VM stops, I/O status set, QEVENT_BLOCK_IO_ERROR sent to management app. 3. qemu receives eject command, bdrv_close() resets I/O status 4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out which drive caused it. If 2 happens before 3, I/O status is lost. >> > + >> > +/* The I/O status is only enabled if the drive explicitly >> > + * enables it _and_ the VM is configured to stop on errors */ >> > +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs) >> > +{ >> > + return (bs->iostatus != BDRV_IOS_INVAL && >> > + (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC || >> > + bs->on_write_error == BLOCK_ERR_STOP_ANY || >> > + bs->on_read_error == BLOCK_ERR_STOP_ANY)); >> > +} >> > + >> > +void bdrv_iostatus_reset(BlockDriverState *bs) >> > +{ >> > + if (bdrv_iostatus_is_enabled(bs)) { >> > + bs->iostatus = BDRV_IOS_OK; >> > + } >> > +} >> > + >> > +void bdrv_iostatus_set_err(BlockDriverState *bs, int error) >> > +{ >> > + if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) { >> > + bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC : >> > + BDRV_IOS_FAILED; >> > + } >> > +} >> > + >> >> abs(error) feels... unusual. >> >> If you want to guard against callers passing wrongly signed values, why >> not simply assert(error >= 0)? > > Yes. Actually, I thought that there were existing devices that could pass > a positive value (while others passed a negative one), but by taking a look > at the code now it seems that all supported devices pass a negative value, > so your suggestion makes sense. > >> >> > void >> > bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t bytes, >> > enum BlockAcctType type) >> > diff --git a/block.h b/block.h >> > index 16bfa0a..de74af0 100644 >> > --- a/block.h >> > +++ b/block.h >> > @@ -77,6 +77,15 @@ typedef enum { >> > BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP >> > } BlockMonEventAction; >> > >> > +typedef enum { >> > + BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC, >> > + BDRV_IOS_MAX >> > +} BlockIOStatus; >> >> Why is this in block.h? > > I followed BlockErrorAction, you suggest block_int.h? I guess I'd put it in block_int.h, just because I can. No biggie, though. [...]