[PATCH] Re: zero-filed page on VOP_PUTPAGES

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

Hello again

Here is my latest attempt to fix the race condition on file size between
puffs_vnop_fsync() and puffs_vnop_getattr(). In a nutshell for the one that
missed previous episodes, this is about preventing puffs_vnop_getattr() to 
call uvm_vnp_setsize() with a stall value (which causes a truncate) while 
a PUFFS_VN_SETATTR operation is in progress.

The PUFFS_VN_SETATTR sent by puffs_vnop_fsync/flushvncache/dosetattr
is kept asynchronous to avoid a deadlock between the ioflush thread
and the file server (see "Re: deadlock on flt_noram5" on 
tech-kern%netbsd.org@localhost), but it is not FAF anymore, and has a callback
so that we can keep track of the critical window end.

That code passes my test case for data corruption, and it does not hang
the kernel. 

I have two concerns:
1) pn->pn_inresize is decreased in puffs_parkdone_setattr(), but exclusive
access to that field is not guaranteed by vnode locking at that time. Should
I use atomic_inc_uint() and atomic_dec_uint()?

2) puffs_vnop_fsync/flushvncache/dosetattr calls uvm_vnp_setsize() to 
set the new size regardless of the PUFFS_VN_SETATTR operation success. 
What about if it failed? We could call uvm_vnp_setsize() from 
puffs_parkdone_setattr(), but that will open aanother can of worms with 
stall size and spurious truncate.

-- 
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   24 Aug 2011 08:00:29 -0000
@@ -162,6 +162,8 @@ puffs_getvnode(struct mount *mp, puffs_c
        pnode->pn_vp = vp;
        pnode->pn_serversize = vsize;
 
+       pnode->pn_inresize = 0;
+
        genfs_node_init(vp, &puffs_genfsops);
        *vpp = vp;
 
Index: sys/fs/puffs/puffs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/fs/puffs/puffs_subr.c,v
retrieving revision 1.65
diff -u -p -d -r1.65 puffs_subr.c
--- sys/fs/puffs/puffs_subr.c   1 Mar 2008 14:16:51 -0000       1.65
+++ sys/fs/puffs/puffs_subr.c   24 Aug 2011 08:00:29 -0000
@@ -87,6 +87,17 @@ puffs_credcvt(struct puffs_kcred *pkcr, 
 }
 
 void
+puffs_parkdone_setattr(struct puffs_mount *pmp,
+       struct puffs_req *preq, void *arg)
+{
+       struct puffs_node *pn = arg;
+
+       pn->pn_inresize--;
+
+       return;
+}
+
+void
 puffs_parkdone_asyncbioread(struct puffs_mount *pmp,
        struct puffs_req *preq, void *arg)
 {
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    24 Aug 2011 08:00:29 -0000
@@ -205,6 +205,8 @@ struct puffs_node {
        voff_t          pn_serversize;
        struct lockf *  pn_lockf;
 
+       unsigned int    pn_inresize;
+
        LIST_ENTRY(puffs_node) pn_hashent;
 };
 
@@ -247,6 +249,8 @@ void        puffs_makecn(struct puffs_kcn *, st
                     const struct componentname *, int);
 void   puffs_credcvt(struct puffs_kcred *, kauth_cred_t);
 
+void   puffs_parkdone_setattr(struct puffs_mount *,
+                              struct puffs_req *, void *);
 void   puffs_parkdone_asyncbioread(struct puffs_mount *,
                                    struct puffs_req *, void *);
 void   puffs_parkdone_asyncbiowrite(struct puffs_mount *,
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  24 Aug 2011 08:00:30 -0000
@@ -898,7 +898,8 @@ puffs_vnop_getattr(void *v)
        } else {
                if (rvap->va_size != VNOVAL
                    && vp->v_type != VBLK && vp->v_type != VCHR) {
-                       uvm_vnp_setsize(vp, rvap->va_size);
+                       if (pn->pn_inresize == 0)
+                               uvm_vnp_setsize(vp, rvap->va_size);
                        pn->pn_serversize = rvap->va_size;
                }
        }
@@ -956,8 +957,22 @@ dosetattr(struct vnode *vp, struct vattr
        puffs_credcvt(&setattr_msg->pvnr_cred, cred);
        puffs_msg_setinfo(park_setattr, PUFFSOP_VN,
            PUFFS_VN_SETATTR, VPTOPNC(vp));
+
+       /*
+        * If we are called from puffs_vnop_fsync(), then this
+        * PUFFS_VN_SETATTR operation must be asynchronous, otherwise
+        * the ioflush thread may deadlock with the file server.
+        * 
+        * However, the window between PUFFS_VN_SETATTR request is sent
+        * and its completion is critical for uvm_vnp_setsize() calls   
+        * from puffs_vnop_getattr(), since it could set a stall size
+        * and trigger a spurious truncate. pn->pn_inresize is here to
+        * ensure it will not happen, and we need a a PUFFS_VN_SETATTR
+        * completion callback to decrease it.
+        */
        if (flags & SETATTR_ASYNC)
-               puffs_msg_setfaf(park_setattr);
+               puffs_msg_setcall(park_setattr, puffs_parkdone_setattr, pn);
+       pn->pn_inresize++;
 
        puffs_msg_enqueue(pmp, park_setattr);
        if ((flags & SETATTR_ASYNC) == 0)
@@ -977,6 +992,10 @@ dosetattr(struct vnode *vp, struct vattr
                        uvm_vnp_setsize(vp, vap->va_size);
        }
 
+       /* Decreased from puffs_parkdone_setattr() if SETATTR_ASYNC is set */
+       if ((flags & SETATTR_ASYNC) == 0)
+                pn->pn_inresize--;
+
        return 0;
 }