Date: Wed, 20 Mar 2013 19:31:00 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: Jeremy Chadwick <jdc@koitsu.org>, Michael Landin Hostbaek <mich@FreeBSD.org>, freebsd-stable@FreeBSD.org, John Baldwin <jhb@FreeBSD.org>, Andriy Gapon <avg@FreeBSD.org> Subject: Re: Core Dump / panic sleeping thread Message-ID: <20130320173100.GF3794@kib.kiev.ua> In-Reply-To: <1758615869.4102301.1363793876607.JavaMail.root@erie.cs.uoguelph.ca> References: <20130320132222.GC3794@kib.kiev.ua> <1758615869.4102301.1363793876607.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
--N+0yS2UTPp5bxBJO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Mar 20, 2013 at 11:37:56AM -0400, Rick Macklem wrote:
> Konstantin Belousov wrote:
> > On Wed, Mar 20, 2013 at 12:13:05PM +0100, Michael Landin Hostbaek
> > wrote:
> > >
> > > On Mar 20, 2013, at 10:49 AM, Konstantin Belousov
> > > <kostikbel@gmail.com> wrote:
> > > >
> > > > I do not like it. As I said in the previous response to Andrey,
> > > > I think that moving the vnode_pager_setsize() after the unlock is
> > > > better, since it reduces races with other thread seeing half-done
> > > > attribute update or making attribute change simultaneously.
> > >
> > > OK - so should I wait for another patch - or?
> >=20
> > I think the following is what I mean. As an additional note, why nfs
> > client does not trim the buffers when server reported node size change
> > ?
> >=20
> > diff --git a/sys/fs/nfsclient/nfs_clport.c
> > b/sys/fs/nfsclient/nfs_clport.c
> > index a07a67f..4fe2e35 100644
> > --- a/sys/fs/nfsclient/nfs_clport.c
> > +++ b/sys/fs/nfsclient/nfs_clport.c
> > @@ -361,6 +361,8 @@ nfscl_loadattrcache(struct vnode **vpp, struct
> > nfsvattr *nap, void *nvaper,
> > struct nfsnode *np;
> > struct nfsmount *nmp;
> > struct timespec mtime_save;
> > + u_quad_t nsize;
> > + int setnsize;
> >=20
> > /*
> > * If v_type =3D=3D VNON it is a new node, so fill in the v_type,
> > @@ -418,6 +420,7 @@ nfscl_loadattrcache(struct vnode **vpp, struct
> > nfsvattr *nap, void *nvaper,
> > } else
> > vap->va_fsid =3D vp->v_mount->mnt_stat.f_fsid.val[0];
> > np->n_attrstamp =3D time_second;
> > + setnsize =3D 0;
> > if (vap->va_size !=3D np->n_size) {
> > if (vap->va_type =3D=3D VREG) {
> > if (dontshrink && vap->va_size < np->n_size) {
> > @@ -444,10 +447,13 @@ nfscl_loadattrcache(struct vnode **vpp, struct
> > nfsvattr *nap, void *nvaper,
> > np->n_size =3D vap->va_size;
> > np->n_flag |=3D NSIZECHANGED;
> > }
> > - vnode_pager_setsize(vp, np->n_size);
> > } else {
> > np->n_size =3D vap->va_size;
> > }
> > + if (vap->va_type =3D=3D VREG || vap->va_type =3D=3D VDIR) {
> > + setnsize =3D 1;
> > + nsize =3D vap->va_size;
> I might have used np->n_size here, since that is what is given
> as the argument for the pre-patched version, but since
> np->n_size should equal vap->va_size (it is set the same for
> all cases in the code at this point), it doesn't really matter.
>=20
> I have no idea what the implications of doing vnode_pager_setsize()
> for VDIR is, but Kostik would be much more conversant that I on this,
> so if he thinks it's ok, that's fine with me.
>=20
> > + }
> > }
> > /*
> > * The following checks are added to prevent a race between (say)
> > @@ -480,6 +486,8 @@ nfscl_loadattrcache(struct vnode **vpp, struct
> > nfsvattr *nap, void *nvaper,
> > KDTRACE_NFS_ATTRCACHE_LOAD_DONE(vp, vap, 0);
> > #endif
> > NFSUNLOCKNODE(np);
> > + if (setnsize)
> > + vnode_pager_setsize(vp, nsize);
> > return (0);
> > }
> Yes, I think Kostik's version of the patch is good. I had thought
> of doing it that way, but want for the "minimal change" version.
> I agree that avoiding unlocking/relocking the mutex is a good idea,
> although I didn't see anything after the relock that I thought
> might be an issue if something changed while unlocked.
If the parallel calls to nfscl_loadattrcache() are possible, then
IMHO at least the n_attrstamp could be cleared needlessly.
>=20
> Kostik, thanks for posting this version, rick
> ps: Michael, I'd suggest you try this patch instead of mine.
Still, my patch has the issue I noted for the head as well: the buffers
are not destroyed if the size of the vnode is decreased. I would be
inclined to suggest the following change on top of my patch, but I am
sure that it does not work, since vnode is generally not locked in
the nfs_loadattrcache(), I think:
diff --git a/sys/fs/nfsclient/nfs_clport.c b/sys/fs/nfsclient/nfs_clport.c
index 4fe2e35..3a08424 100644
--- a/sys/fs/nfsclient/nfs_clport.c
+++ b/sys/fs/nfsclient/nfs_clport.c
@@ -487,7 +487,7 @@ nfscl_loadattrcache(struct vnode **vpp, struct nfsvattr=
*nap, void *nvaper,
#endif
NFSUNLOCKNODE(np);
if (setnsize)
- vnode_pager_setsize(vp, nsize);
+ vtruncbuf(vp, NOCRED, nsize, vp->v_bufobj.bo_bsize);
return (0);
}
=20
--N+0yS2UTPp5bxBJO
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iQIcBAEBAgAGBQJRSfJTAAoJEJDCuSvBvK1BDnIP/Rw0+zVoNBr+l/53ZxZoZGqP
i6FjxyZXKExvllWiTeRu952gA5oCXnD8Lw3UEaKbHbtu2OG8fnQ82Z+lGMkhWtZp
5ZzkXP5aZOLc7aNRgQ9EggBIC1QgjAXPWM1mDsCtQw3nwUxucN0VIW1Lej3uTFER
/mDsF5ISHsd0gILchlyRumAreJvcxTuAFPESUdjsQHoecy5dKStVtmdqg1Pw4hor
NLlrCJrO49/vtPp4J2QtRv1mOyH6bjq8e3EfUNqpldoMcgJxCIch7xH2nhNPoZ4K
Ozl8KTe8yLNpfatmqU0K3QM67EcJRUeE/z9HOa5qR4gWjYsDzG84SdKfHt/BBHLg
8QfGeLxWSPQREgDUiK9E29Owy5RvziSWF68Jed6DKVjBvHfrkIIZL9vXM/jxDx6F
IrA/MEqKtww0aLY2iTPB+Yv9w7Q+TpvOOEuznaA+lXKA+np7wvtC7YzsZqgTuLng
DHd6zcDY8XHKfvKjBcepE4lfFEXTyEmXMx6bqAh8Na6PWZWg+RmdA0w8RUM28rMd
NMWJvnPlPcPv80jAKEtqwEYcdbHojDlRkR9HdOhgkcTd+MMgbA2e4lZIRIuZ8TD3
lvfF98qSDLFMhPjtyEvYV96DiCbkamkvNj8MMDOkUZW/sO3G7eNVkBeEK6rAaTSp
kl5dzygqpNqSGePyF2ND
=4441
-----END PGP SIGNATURE-----
--N+0yS2UTPp5bxBJO--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130320173100.GF3794>