From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36334 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pznt9-0002Jl-SS for qemu-devel@nongnu.org; Wed, 16 Mar 2011 06:23:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pznt7-0007Av-Jo for qemu-devel@nongnu.org; Wed, 16 Mar 2011 06:23:34 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:36170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pznt7-0007An-G4 for qemu-devel@nongnu.org; Wed, 16 Mar 2011 06:23:33 -0400 Received: by yxk8 with SMTP id 8so721761yxk.4 for ; Wed, 16 Mar 2011 03:23:33 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20110315103901.GD23922@linux.vnet.ibm.com> References: <20110315103453.GA23922@linux.vnet.ibm.com> <20110315103901.GD23922@linux.vnet.ibm.com> Date: Wed, 16 Mar 2011 10:23:32 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arun@linux.vnet.ibm.com Cc: aliguori@us.ibm.com, jvrao@linux.vnet.ibm.com, qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj wrote: > -static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int er= r) > +static void v9fs_stat_post_lstat(void *opaque) > =A0{ > - =A0 =A0if (err =3D=3D -1) { > - =A0 =A0 =A0 =A0err =3D -errno; > + =A0 =A0V9fsStatState *vs =3D (V9fsStatState *)opaque; No need to cast void* in C. > + =A0 =A0if (vs->err =3D=3D -1) { > + =A0 =A0 =A0 =A0vs->err =3D -(vs->v9fs_errno); How about the thread worker function puts the -errno into a vs->ret field: static void v9fs_stat_do_lstat(V9fsRequest *request) { =A0 =A0V9fsStatState *vs =3D container_of(request, V9fsStatState, request)= ; =A0 =A0vs->ret =3D v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf)= ; if (vs->ret !=3D 0) { =A0 =A0 vs->ret =3D -errno; } } Then v9fs_stat_post_lstat() can use vs->ret directly and does not need to juggle around the two fields, vs->err and vs->v9fs_errno. > =A0 =A0 =A0 =A0 goto out; > =A0 =A0 } > > - =A0 =A0err =3D stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs= ->v9stat); > - =A0 =A0if (err) { > + =A0 =A0vs->err =3D stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->st= buf, &vs->v9stat); This function can block in v9fs_do_readlink(). Needs to be done asynchronously to avoid blocking QEMU. > + =A0 =A0if (vs->err) { > =A0 =A0 =A0 =A0 goto out; > =A0 =A0 } > =A0 =A0 vs->offset +=3D pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9= stat); > - =A0 =A0err =3D vs->offset; > + =A0 =A0vs->err =3D vs->offset; > > =A0out: > - =A0 =A0complete_pdu(s, vs->pdu, err); > + =A0 =A0complete_pdu(vs->s, vs->pdu, vs->err); > =A0 =A0 v9fs_stat_free(&vs->v9stat); > =A0 =A0 qemu_free(vs); > =A0} > > +static void v9fs_stat_do_lstat(V9fsRequest *request) > +{ > + =A0 =A0V9fsStatState *vs =3D container_of(request, V9fsStatState, reque= st); Nice. Could container_of() be used for v9fs_post_lstat() too? I'm suggesting making post op functions take the V9fsRequest* instead of a void* opaque pointer. > + > + =A0 =A0vs->err =3D v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stb= uf); This is not threadsafe since rpath still uses a static buffer in qemu.git. Please ensure that rpath() is thread-safe before pushing this patch. > + =A0 =A0vs->v9fs_errno =3D errno; > +} > + > =A0static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) > =A0{ > =A0 =A0 int32_t fid; > @@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) > =A0 =A0 vs =3D qemu_malloc(sizeof(*vs)); > =A0 =A0 vs->pdu =3D pdu; > =A0 =A0 vs->offset =3D 7; > + =A0 =A0vs->s =3D s; > + =A0 =A0vs->request.func =3D v9fs_stat_do_lstat; > + =A0 =A0vs->request.post_op.func =3D v9fs_stat_post_lstat; > + =A0 =A0vs->request.post_op.arg =3D vs; > > =A0 =A0 memset(&vs->v9stat, 0, sizeof(vs->v9stat)); > > @@ -1498,8 +1511,11 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) > =A0 =A0 =A0 =A0 goto out; > =A0 =A0 } > > + =A0 =A0/* > =A0 =A0 err =3D v9fs_do_lstat(s, &vs->fidp->fsmap.path, &vs->stbuf); > =A0 =A0 v9fs_stat_post_lstat(s, vs, err); > + =A0 =A0*/ Please remove unused code, it quickly becomes out-of-date and confuses read= ers. > + =A0 =A0v9fs_qemu_submit_request(&vs->request); What happens when another PDU is handled next that uses the same fid? The worst case is if the client sends TCLUNK and fid is freed while the worker thread and later the post op still access the memory. There needs to be some kind of guard (like a reference count) to prevent this. Stefan