From owner-freebsd-stable@FreeBSD.ORG Thu Mar 21 01:14:45 2013 Return-Path: Delivered-To: freebsd-stable@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id CDBFE77B; Thu, 21 Mar 2013 01:14:45 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-jnhn.mail.uoguelph.ca (esa-jnhn.mail.uoguelph.ca [131.104.91.44]) by mx1.freebsd.org (Postfix) with ESMTP id 30676EE3; Thu, 21 Mar 2013 01:14:44 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqAEADZeSlGDaFvO/2dsb2JhbAA7CIgdvR2Ba3SCJAEBBAEjBFIFFg4KAgINGQIjNgYTiAIDCQawDIhqDYlbgSOLJIEVgQA0B4ItgRMDlQCBYItohRqDJiCBbA X-IronPort-AV: E=Sophos;i="4.84,881,1355115600"; d="scan'208";a="22185724" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-jnhn.mail.uoguelph.ca with ESMTP; 20 Mar 2013 21:14:37 -0400 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 8D4B2B4032; Wed, 20 Mar 2013 21:14:37 -0400 (EDT) Date: Wed, 20 Mar 2013 21:14:37 -0400 (EDT) From: Rick Macklem To: Konstantin Belousov Message-ID: <223607151.4129243.1363828477555.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130320173100.GF3794@kib.kiev.ua> Subject: Re: Core Dump / panic sleeping thread MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.203] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) Cc: Jeremy Chadwick , Michael Landin Hostbaek , freebsd-stable@FreeBSD.org, John Baldwin , Andriy Gapon X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Mar 2013 01:14:45 -0000 Konstantin Belousov wrote: > 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 > > > > 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? > > > > > > 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 > > > ? > > > > > > 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; > > > > > > /* > > > * If v_type == 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 = vp->v_mount->mnt_stat.f_fsid.val[0]; > > > np->n_attrstamp = time_second; > > > + setnsize = 0; > > > if (vap->va_size != np->n_size) { > > > if (vap->va_type == 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 = vap->va_size; > > > np->n_flag |= NSIZECHANGED; > > > } > > > - vnode_pager_setsize(vp, np->n_size); > > > } else { > > > np->n_size = vap->va_size; > > > } > > > + if (vap->va_type == VREG || vap->va_type == VDIR) { > > > + setnsize = 1; > > > + nsize = 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. > > > > 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. > > > > > + } > > > } > > > /* > > > * 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. > And that could result in an attribute cache miss, since setting n_attrstamp = 0 invalidates the cached attributes. I agree that your patch is preferred. I'll admit I was mostly lazy (and a little afraid of moving the vnode_pager_setsize()) when I did the patch. > > > > 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: > Well, read/write sharing of files over NFS is pretty rare, so I suspect a truncation of a file by another client (or locally in the NFS server) is a rare event. As such, not invalidating the buffers here doesn't seem like a big issue? (The client uses np->n_size to determine EOF.) Also, I think close-to-open consistency will typically throw away the buffers on the next open when it sees the mtime changed. (Yes, there won't necessarily be another open, but...) As you point out, I don't think the vnode will be locked when nfscl_loadattrcache() is called for read/writes being done by the nfsiod threads and will only be a shared vnode lock for other reads. I think your patch without the below addition is just fine, rick > 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); > }