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 > > > <kostik...@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? > > > > 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.
> > 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); }
pgpXjtJ_eVr_v.pgp
Description: PGP signature