From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39526 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PyqUI-0003Md-Ob for qemu-devel@nongnu.org; Sun, 13 Mar 2011 14:57:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PyqUH-0007m5-FS for qemu-devel@nongnu.org; Sun, 13 Mar 2011 14:57:58 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:34731) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PyqUG-0007lK-Tw for qemu-devel@nongnu.org; Sun, 13 Mar 2011 14:57:57 -0400 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [202.81.31.247]) by e23smtp06.au.ibm.com (8.14.4/8.13.1) with ESMTP id p2DIvfNU004453 for ; Mon, 14 Mar 2011 05:57:41 +1100 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2DIvj0Y2129940 for ; Mon, 14 Mar 2011 05:57:46 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2DIvjxB003087 for ; Mon, 14 Mar 2011 05:57:45 +1100 From: "Aneesh Kumar K. V" Subject: Re: [Qemu-devel] [PATCH -V3 2/8] hw/9pfs: Add file descriptor reclaim support In-Reply-To: References: <1299347533-17047-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1299347533-17047-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com> Date: Mon, 14 Mar 2011 00:27:39 +0530 Message-ID: <87d3lurgpo.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Sun, 13 Mar 2011 16:08:29 +0000, Stefan Hajnoczi wr= ote: > On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V > wrote: > > @@ -107,7 +108,12 @@ static int v9fs_do_closedir(V9fsState *s, DIR *dir) > > > > =C2=A0static int v9fs_do_open(V9fsState *s, V9fsString *path, int flags) > > =C2=A0{ > > - =C2=A0 =C2=A0return s->ops->open(&s->ctx, path->data, flags); > > + =C2=A0 =C2=A0int fd; > > + =C2=A0 =C2=A0fd =3D s->ops->open(&s->ctx, path->data, flags); > > + =C2=A0 =C2=A0if (fd > P9_FD_RECLAIM_THRES) { > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0v9fs_reclaim_fd(s); > > + =C2=A0 =C2=A0} >=20 > I think the threshold should depend on the file descriptor ulimit. > The hardcoded constant doesn't work if the ulimit is set to 1000 or > less (it would cause other users in QEMU to hit EMFILE errors). Yes. That is suppose to be a follow up patch. I had that set to 100 for all the early testing. >=20 > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (f->fsmap.fid_type =3D=3D= P9_FID_FILE) { > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* FIXME!! sho= uld we remember the open flags ?*/ > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (f->fsmap.f= s.fd =3D=3D -1) { > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= f->fsmap.fs.fd =3D v9fs_do_open(s, &f->fsmap.path, O_RDWR); > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >=20 > Please address the FIXME. I think the case where O_RDWR breaks is if > QEMU has permissions to open the file for read only. The the client > is able to open the file for read but when the file descriptor is > resurrected we'll get EPERM here. The FIXME is fixed in the follow up patch (patch 5) >=20 > > @@ -516,7 +600,10 @@ static int free_fid(V9fsState *s, int32_t fid) > > =C2=A0 =C2=A0 *fidpp =3D fidp->next; > > > > =C2=A0 =C2=A0 if (fidp->fsmap.fid_type =3D=3D P9_FID_FILE) { > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0v9fs_do_close(s, fidp->fsmap.fs.fd); > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* I we reclaimed the fd no need to close = */ >=20 > s/I // >=20 > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (fidp->fsmap.fs.fd !=3D -1) { > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0v9fs_do_close(s, fidp->fsmap= .fs.fd); > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > > =C2=A0 =C2=A0 } else if (fidp->fsmap.fid_type =3D=3D P9_FID_DIR) { > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 v9fs_do_closedir(s, fidp->fsmap.fs.dir); > > =C2=A0 =C2=A0 } else if (fidp->fsmap.fid_type =3D=3D P9_FID_XATTR) { > > @@ -2719,7 +2806,11 @@ static void v9fs_remove(V9fsState *s, V9fsPDU *p= du) > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D -EINVAL; > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out; > > =C2=A0 =C2=A0 } > > - > > + =C2=A0 =C2=A0/* > > + =C2=A0 =C2=A0 * IF the file is unlinked, we cannot reopen > > + =C2=A0 =C2=A0 * the file later. So don't reclaim fd > > + =C2=A0 =C2=A0 */ > > + =C2=A0 =C2=A0v9fs_mark_fids_unreclaim(s, &vs->fidp->fsmap.path); >=20 > This poses a problem for the case where guest and host are both > accessing the file system. If the fd is reclaimed and the host > deletes the file, then the guest cannot access its open file anymore. >=20 > The same issue also affects rename and has not been covered by this patch. >=20 Currently virtFS don't handle the host rename/unlink. That we walk a name and get the fid and then use the fid to open the file. In between if the file get removed/renamed we will get an EINVAL. All that will go away once we switch to handle based open. -aneesh