From patchwork Tue Aug 16 19:18:55 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Supriya Kannery X-Patchwork-Id: 110217 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 5D5F0B6F65 for ; Wed, 17 Aug 2011 05:07:32 +1000 (EST) Received: from localhost ([::1]:38031 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QtOyw-00065N-V4 for incoming@patchwork.ozlabs.org; Tue, 16 Aug 2011 15:07:22 -0400 Received: from eggs.gnu.org ([140.186.70.92]:44025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QtOyk-0005jv-6E for qemu-devel@nongnu.org; Tue, 16 Aug 2011 15:07:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QtOyi-00010U-Qq for qemu-devel@nongnu.org; Tue, 16 Aug 2011 15:07:10 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:46523) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QtOyi-00010E-6A for qemu-devel@nongnu.org; Tue, 16 Aug 2011 15:07:08 -0400 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [202.81.31.245]) by e23smtp06.au.ibm.com (8.14.4/8.13.1) with ESMTP id p7GJ6D3Z022568 for ; Wed, 17 Aug 2011 05:06:13 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p7GJ76Fm475194 for ; Wed, 17 Aug 2011 05:07:06 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p7GJ76B6011194 for ; Wed, 17 Aug 2011 05:07:06 +1000 Received: from [9.77.82.197] ([9.77.82.197]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p7GJ72mx011165; Wed, 17 Aug 2011 05:07:03 +1000 Message-ID: <4E4AC29F.7010601@linux.vnet.ibm.com> Date: Wed, 17 Aug 2011 00:48:55 +0530 From: Supriya Kannery User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090513 Fedora/3.0-2.3.beta2.fc11 Thunderbird/3.0b2 MIME-Version: 1.0 To: supriya kannery References: <4E3BB203.5060703@redhat.com> <20110805131247.GC6201@redhat.com> <20110805142812.GB17323@lst.de> <4E3C0FAA.2080303@redhat.com> <4E3C1117.8000009@codemonkey.ws> <4E3F89FC.8090109@linux.vnet.ibm.com> <4E3F9A7E.1090906@redhat.com> <4E40FC59.7010104@in.ibm.com> <4E410306.4010002@redhat.com> <4E40FEBA.60001@in.ibm.com> In-Reply-To: <4E40FEBA.60001@in.ibm.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-Received-From: 202.81.31.148 Cc: Kevin Wolf , Stefan Hajnoczi , Christoph Hellwig , qemu-devel , Paolo Bonzini Subject: Re: [Qemu-devel] [RFC] Safely reopening image files by stashing fds X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: supriyak@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 08/09/2011 03:02 PM, supriya kannery wrote: > Kevin Wolf wrote: >> Am 09.08.2011 11:22, schrieb supriya kannery: >>> Kevin Wolf wrote: >> >> What I meant is that in the end, with a generic bdrv_reopen(), we can >> have raw-posix only call dup() and fcntl() instead of doing a >> close()/open() sequence if it can satisfy the new flags this way. But >> this would be an implementation detail and not be visible in the >> interface. >> >> Kevin > > ok > - thanks, Supriya > Though I started the RFC patch with defining BDRVReopenState, ended up in enhancing struct BlockDriver with .bdrv_reopen. bdrv_reopen mplementation specific to respective driver is assigned to this function pointer. Please find the implementation of O_DIRECT flag change, done in raw-posix.c. Similar implementation can be done for vmdk (with bdrv_reopen_commit and bdrv_reopen_abort as internal functions in vmdk.c for opening split files), sheepdog, nbd etc.. Request for comments on this approach. Note: Following patch is for demonstrating the approach using raw-posix as sample driver. This patch is not complete. - thanks, Supriya --- block.c | 33 +++++++++++++++++++++++---------- block/raw-posix.c | 34 ++++++++++++++++++++++++++++++++++ block_int.h | 1 + 3 files changed, 58 insertions(+), 10 deletions(-) int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors); Index: qemu/block/raw-posix.c =================================================================== --- qemu.orig/block/raw-posix.c +++ qemu/block/raw-posix.c @@ -276,6 +276,39 @@ static int raw_open(BlockDriverState *bs return raw_open_common(bs, filename, flags, 0); } +static int raw_reopen(BlockDriverState *bs, int flags) +{ + BDRVRawState *s = bs->opaque; + int new_fd; + int ret = 0; + + /* Handle request for toggling O_DIRECT */ + if ((bs->open_flags | BDRV_O_NOCACHE) ^ + (flags | BDRV_O_NOCACHE)) + { + if ((new_fd = dup(s->fd)) <= 0) { + return -1; + } + + if ((ret = fcntl_setfl(new_fd, flags)) < 0) { + close(new_fd); + return ret; + } + + /* Set new flags, so replace old fd with new one */ + close(s->fd); + s->fd = new_fd; + s->open_flags = flags; + bs->open_flags = flags; + } + + /* + * TBD: handle O_DSYNC and other flags for which image + * file has to be reopened + */ + return 0; +} + /* XXX: use host sector size if necessary with: #ifdef DIOCGSECTORSIZE { @@ -886,6 +919,7 @@ static BlockDriver bdrv_file = { .instance_size = sizeof(BDRVRawState), .bdrv_probe = NULL, /* no probe for protocols */ .bdrv_file_open = raw_open, + .bdrv_reopen = raw_reopen, .bdrv_read = raw_read, .bdrv_write = raw_write, .bdrv_close = raw_close, Index: qemu/block.c =================================================================== --- qemu.orig/block.c +++ qemu/block.c @@ -687,20 +687,33 @@ int bdrv_reopen(BlockDriverState *bs, in qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name); return ret; } - open_flags = bs->open_flags; - bdrv_close(bs); - ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); - if (ret < 0) { - /* Reopen failed. Try to open with original flags */ - qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); - ret = bdrv_open(bs, bs->filename, open_flags, drv); + /* Use driver specific reopen() if available */ + if (drv->bdrv_reopen) { + if ((ret = drv->bdrv_reopen(bs, bdrv_flags)) < 0) { + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); + return ret; + } + } else { + /* Use reopen procedure common for drivers */ + open_flags = bs->open_flags; + bdrv_close(bs); + + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); if (ret < 0) { - /* Reopen failed with orig and modified flags */ - abort(); + /* + * Reopen failed. Try to open with original flags + */ + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); + ret = bdrv_open(bs, bs->filename, open_flags, drv); + if (ret < 0) { + /* + * Reopen with orig and modified flags failed + */ + abort(); + } } } - return ret; } Index: qemu/block_int.h =================================================================== --- qemu.orig/block_int.h +++ qemu/block_int.h @@ -54,6 +54,7 @@ struct BlockDriver { int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); int (*bdrv_probe_device)(const char *filename); int (*bdrv_open)(BlockDriverState *bs, int flags); + int (*bdrv_reopen)(BlockDriverState *bs, int flags); int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);