From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34816) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEi0w-0001AA-EW for qemu-devel@nongnu.org; Thu, 20 Sep 2012 10:46:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEi0r-0007pB-6u for qemu-devel@nongnu.org; Thu, 20 Sep 2012 10:46:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57324) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEi0q-0007os-T0 for qemu-devel@nongnu.org; Thu, 20 Sep 2012 10:45:57 -0400 Message-ID: <505B2C1E.8000808@redhat.com> Date: Thu, 20 Sep 2012 10:45:50 -0400 From: Jeff Cody MIME-Version: 1.0 References: <4feab4ab212834c55707c0690d8efcb4f3662b1c.1347993885.git.jcody@redhat.com> <505B23DC.3090702@redhat.com> In-Reply-To: <505B23DC.3090702@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: stefanha@gmail.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, supriyak@linux.vnet.ibm.com On 09/20/2012 10:10 AM, Kevin Wolf wrote: > Am 18.09.2012 20:53, schrieb Jeff Cody: >> This is derived from the Supriya Kannery's reopen patches. >> >> This contains the raw-posix driver changes for the bdrv_reopen_* >> functions. All changes are staged into a temporary scratch buffer >> during the prepare() stage, and copied over to the live structure >> during commit(). Upon abort(), all changes are abandoned, and the >> live structures are unmodified. >> >> The _prepare() will create an extra fd - either by means of a dup, >> if possible, or opening a new fd if not (for instance, access >> control changes). Upon _commit(), the original fd is closed and >> the new fd is used. Upon _abort(), the duplicate/new fd is closed. >> >> Signed-off-by: Jeff Cody > >> +static int raw_reopen_prepare(BDRVReopenState *state, >> + BlockReopenQueue *queue, Error **errp) >> +{ >> + BDRVRawState *s; >> + BDRVRawReopenState *raw_s; >> + int ret = 0; >> + >> + assert(state != NULL); >> + assert(state->bs != NULL); >> + >> + s = state->bs->opaque; >> + >> + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); >> + raw_s = state->opaque; >> + raw_s->use_aio = s->use_aio; >> + raw_s->aio_ctx = s->aio_ctx; > > You can immediately set s->aio_ctx instead of going through > BDRVRawReopenState with it. It seems to be valid to have it present > while use_aio = 0. If it wasn't valid, you'd have to free the context > when reopening without Linux AIO. > Good catch, thanks. >> + >> + raw_parse_flags(state->flags, &raw_s->open_flags); >> + raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags); > > At least you're consistently omitting the error check. :-) > Thanks, fixed. I guess I am an optimist at heart :) >> + >> + raw_s->fd = -1; >> + >> + int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK; >> +#ifdef O_NOATIME >> + fcntl_flags |= O_NOATIME; >> +#endif >> + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { >> + /* dup the original fd */ >> + /* TODO: use qemu fcntl wrapper */ > > Hm, still not addressed? > No. I mentioned this in the cover letter for v2. I'd rather see the qemu fcntl wrapper happen as a separate series, and then come back and update this, if that is OK. I'm afraid changes to qemu_open or qemu_dup_flags would delay getting this series in. Although Eric is right, I do need to ifdef the F_DUPFD_CLOEXEC. > Kevin >