Re: [PATCH] Re: zero-filed page on VOP_PUTPAGES

[Emmanuel Dreyfus <manu%netbsd.org@localhost>]

On Fri, Aug 26, 2011 at 08:33:47AM +0000, YAMAMOTO Takashi wrote:
> if we go this route, it's cleaner to use a puffs internal lock

I did it that way, and it seems to work fine. 

-- 
Emmanuel Dreyfus
manu%netbsd.org@localhost

? sys/fs/puffs/puffs_vnops.c-debug
? sys/fs/puffs/puffs_vnops.c.ok1
? sys/fs/puffs/puffs_vnops.c.ok2
Index: sys/fs/puffs/puffs_node.c
===================================================================
RCS file: /cvsroot/src/sys/fs/puffs/puffs_node.c,v
retrieving revision 1.13.10.1
diff -u -p -d -r1.13.10.1 puffs_node.c
--- sys/fs/puffs/puffs_node.c   3 Oct 2009 23:11:27 -0000       1.13.10.1
+++ sys/fs/puffs/puffs_node.c   26 Aug 2011 15:18:57 -0000
@@ -155,6 +155,7 @@ puffs_getvnode(struct mount *mp, puffs_c
                }
                KASSERT(pnc != NULL);
        }
+       mutex_init(&pnode->pn_sizemtx, MUTEX_DEFAULT, IPL_NONE);
        mutex_exit(&pmp->pmp_lock);
 
        vp->v_data = pnode;
Index: sys/fs/puffs/puffs_sys.h
===================================================================
RCS file: /cvsroot/src/sys/fs/puffs/puffs_sys.h,v
retrieving revision 1.70.20.2
diff -u -p -d -r1.70.20.2 puffs_sys.h
--- sys/fs/puffs/puffs_sys.h    18 Jun 2011 16:19:39 -0000      1.70.20.2
+++ sys/fs/puffs/puffs_sys.h    26 Aug 2011 15:18:57 -0000
@@ -205,6 +205,8 @@ struct puffs_node {
        voff_t          pn_serversize;
        struct lockf *  pn_lockf;
 
+       kmutex_t        pn_sizemtx;     /* size modification mutex      */
+
        LIST_ENTRY(puffs_node) pn_hashent;
 };
 
Index: sys/fs/puffs/puffs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/puffs/puffs_vnops.c,v
retrieving revision 1.129.4.9
diff -u -p -d -r1.129.4.9 puffs_vnops.c
--- sys/fs/puffs/puffs_vnops.c  17 Jul 2011 15:36:03 -0000      1.129.4.9
+++ sys/fs/puffs/puffs_vnops.c  26 Aug 2011 15:18:58 -0000
@@ -856,6 +856,16 @@ puffs_vnop_getattr(void *v)
        struct puffs_node *pn = VPTOPP(vp);
        int error = 0;
 
+       /*
+        * A lock is required so that we do not race with 
+        * setattr, write and fsync when changing vp->v_size.
+        * This is critical, since setting a stall smaler value
+        * triggers a file truncate in uvm_vnp_setsize(), which
+        * most of the time means data corruption (a chunk of
+        * data is replaced by zeroes).
+        */
+       mutex_enter(&pn->pn_sizemtx);
+
        REFPN(pn);
        vap = ap->a_vap;
 
@@ -906,6 +916,9 @@ puffs_vnop_getattr(void *v)
  out:
        puffs_releasenode(pn);
        PUFFS_MSG_RELEASE(getattr);
+       
+       mutex_exit(&pn->pn_sizemtx);
+
        return error;
 }
 
@@ -919,6 +932,8 @@ dosetattr(struct vnode *vp, struct vattr
        struct puffs_node *pn = vp->v_data;
        int error = 0;
 
+       KASSERT(!(flags & SETATTR_CHSIZE) || mutex_owned(&pn->pn_sizemtx));
+
        if ((vp->v_mount->mnt_flag & MNT_RDONLY) &&
            (vap->va_uid != (uid_t)VNOVAL || vap->va_gid != (gid_t)VNOVAL
            || vap->va_atime.tv_sec != VNOVAL || vap->va_mtime.tv_sec != VNOVAL
@@ -989,8 +1004,14 @@ puffs_vnop_setattr(void *v)
                struct vattr *a_vap;
                kauth_cred_t a_cred;
        } */ *ap = v;
+       struct puffs_node *pn = ap->a_vp->v_data;
+       int error;
 
-       return dosetattr(ap->a_vp, ap->a_vap, ap->a_cred, SETATTR_CHSIZE);
+       mutex_enter(&pn->pn_sizemtx);
+       error = dosetattr(ap->a_vp, ap->a_vap, ap->a_cred, SETATTR_CHSIZE);
+       mutex_exit(&pn->pn_sizemtx);
+
+       return error;
 }
 
 static __inline int
@@ -1040,6 +1061,7 @@ puffs_vnop_inactive(void *v)
        int error;
 
        pnode = vp->v_data;
+       mutex_enter(&pnode->pn_sizemtx);
 
        if (doinact(pmp, pnode->pn_stat & PNODE_DOINACT)) {
                flushvncache(vp, 0, 0, false);
@@ -1059,6 +1081,7 @@ puffs_vnop_inactive(void *v)
         */
        *ap->a_recycle = ((pnode->pn_stat & PNODE_NOREFS) != 0);
 
+       mutex_exit(&pnode->pn_sizemtx);
        VOP_UNLOCK(vp, 0);
 
        return 0;
@@ -1342,13 +1365,15 @@ puffs_vnop_fsync(void *v)
        } */ *ap = v;
        PUFFS_MSG_VARS(vn, fsync);
        struct vnode *vp = ap->a_vp;
+       struct puffs_node *pn = VPTOPP(vp);
        struct puffs_mount *pmp = MPTOPUFFSMP(vp->v_mount);
        int error, dofaf;
 
+       mutex_enter(&pn->pn_sizemtx);
        error = flushvncache(vp, ap->a_offlo, ap->a_offhi,
            (ap->a_flags & FSYNC_WAIT) == FSYNC_WAIT);
        if (error)
-               return error;
+               goto out;
 
        /*
         * HELLO!  We exit already here if the user server does not
@@ -1356,8 +1381,9 @@ puffs_vnop_fsync(void *v)
         * has references neither in the kernel or the fs server.
         * Otherwise we continue to issue fsync() forward.
         */
+       error = 0;
        if (!EXISTSOP(pmp, FSYNC))
-               return 0;
+               goto out;
 
        dofaf = (ap->a_flags & FSYNC_WAIT) == 0 || ap->a_flags == FSYNC_LAZY;
        /*
@@ -1390,6 +1416,8 @@ puffs_vnop_fsync(void *v)
 
        error = checkerr(pmp, error, __func__);
 
+out:
+       mutex_exit(&pn->pn_sizemtx);
        return error;
 }
 
@@ -1932,6 +1960,7 @@ puffs_vnop_write(void *v)
        } */ *ap = v;
        PUFFS_MSG_VARS(vn, write);
        struct vnode *vp = ap->a_vp;
+       struct puffs_node *pn = VPTOPP(vp);
        struct puffs_mount *pmp = MPTOPUFFSMP(vp->v_mount);
        struct uio *uio = ap->a_uio;
        size_t tomove, argsize;
@@ -1943,6 +1972,8 @@ puffs_vnop_write(void *v)
        error = uflags = 0;
        write_msg = NULL;
 
+       mutex_enter(&pn->pn_sizemtx);
+
        if (vp->v_type == VREG && PUFFS_USE_PAGECACHE(pmp)) {
                ubcflags = UBC_WRITE | UBC_PARTIALOK;
                if (UBC_WANT_UNMAP(vp))
@@ -2066,6 +2097,7 @@ puffs_vnop_write(void *v)
                puffs_msgmem_release(park_write);
        }
 
+       mutex_exit(&pn->pn_sizemtx);
        return error;
 }