.

wcfs/client/wcfs.cpp

@@ -258,6 +258,15 @@ static global<error> errConnClosed = errors::New("connection closed");
 error _Conn::close() {
     _Conn& wconn = *this;
 
+    // lock virtmem early. TODO more granular virtmem locking (see __pin1 for
+    // details and why virt_lock currently goes first)
+    virt_lock();
+    bool virtUnlocked = false;
+    defer([&]() {
+        if (!virtUnlocked)
+            virt_unlock();
+    });
+
     wconn._atMu.RLock();
     defer([&]() {
         wconn._atMu.RUnlock();
@@ -272,6 +281,7 @@ error _Conn::close() {
             eret = err;
     };
 
+    // mark wconn as closed, so that no new wconn.open might be spawned.
     bool alreadyClosed = false;
     wconn._filehMu.Lock();
     alreadyClosed = (wconn._downErr == errConnClosed);
@@ -280,39 +290,54 @@ error _Conn::close() {
     if (alreadyClosed)
         return nil;
 
-    // close wlink and signal to pinner to stop outside of wconn.filehMu
-    err = wconn._wlink->close();    // XXX ok under atMu?
-    if (err != nil)
-        reterr1(err);
-    wconn._pinCancel();
-    err = wconn._pinWG->wait();     // XXX ok under atMu?
-    if (!errors::Is(err, context::canceled)) // canceled - ok
-        reterr1(err);
-
-    // pinner is stopped - now close all files - both that have no mappings and
-    // that still have opened mappings.
+    // close all files - both that have no mappings and that still have opened
+    // mappings. We have to close files before shutting down pinner, because
+    // wcfs might send pin messages due to file access by other clients. So to
+    // avoid being killed we have to unwatch all files before stopping the
+    // pinner.
     //
-    // NOTE after file is closed mappings could continue to survive, but we can no
-    // longer maintain consistent view. For this reason we change mappings to
-    // something that gives EFAULT on access. XXX implement
+    // NOTE after file is closed, its mappings could continue to survive, but
+    // we can no longer maintain consistent view. For this reason we change
+    // mappings to give EFAULT on access.
+    while (1) {
+        FileH f = nil;
+
+        // pick up any fileh
         wconn._filehMu.Lock();
-    defer([&]() {
+        if (!wconn._filehTab.empty())
+            f = wconn._filehTab.begin()->second;
         wconn._filehMu.Unlock();
-    });
 
-    // XXX f locking
-    for (auto _ : wconn._filehTab) {
-        //zodb::Oid  fid = _.first;
-        FileH        f   = _.second;
-        err = f->_headf->close(); // XXX mark fileh as down so that fileh.close does not say "bad fd"
+        if (f == nil)
+            break; // all closed
+
+        // XXX vvv only for !opening state
+
+        // close f under
+        // - XXX virt_lock
+        // - wconn.atMu.R
+        // - wconn.filehMu unlocked
+        err = f->_closeLocked();
         if (err != nil)
             reterr1(err);
-        f->_headf = nil;
 
-        // XXX stop watching f      XXX ok under mu?
+        // wait for f close to complete, as it might be that f.close was called
+        // simultaneously to wconn.close
+        f->_closedq.recv();
     }
 
-    wconn._filehTab.clear();
+    // close wlink and signal to pinner to stop.
+    // we have to release virt_lock, to avoid deadlocking with pinner.
+    virtUnlocked = true;
+    virt_unlock();
+
+    err = wconn._wlink->close();
+    if (err != nil)
+        reterr1(err);
+    wconn._pinCancel();
+    err = wconn._pinWG->wait();
+    if (!errors::Is(err, context::canceled)) // canceled - ok
+        reterr1(err);
 
     return E(eret);
 }
@@ -682,10 +707,10 @@ pair<FileH, error> _Conn::open(zodb::Oid foid) {
         return make_pair(nil, E(err));
     }
 
-    // XXX ensure f<foid> @ wconn.at exists?
+    // XXX ensure f<foid>@ wconn.at exists?
     // XXX else we get pins to non-exists state from wcfs.
     //
-    // XXX -> better teach wcfs to reject "watch <foid> @at" for @at where foid did not existed.
+    // XXX -> better teach wcfs to reject "watch <foid> @at" for @at where f did not existed.
 
 retry:
     FileH f; bool ok;
@@ -758,6 +783,8 @@ retry:
     if (f->_openErr != nil)
         return make_pair(nil, E(f->_openErr));
 
+    // XXX recheck the wconn was not closed while the open was in progress
+
     retok = true;
     return make_pair(f, nil);
 }
@@ -816,7 +843,7 @@ error _FileH::close() {
     _FileH& fileh = *this;
     Conn    wconn = fileh.wconn;
 
-    // lock virtmem first. TODO more granular virtmem locking (see __pin1 for
+    // lock virtmem early. TODO more granular virtmem locking (see __pin1 for
     // details and why virt_lock currently goes first)
     virt_lock();
     defer([&]() {
@@ -824,10 +851,25 @@ error _FileH::close() {
     });
 
     wconn->_atMu.RLock();
+    defer([&]() {
+        wconn->_atMu.RUnlock();
+    });
+
+    return fileh._closeLocked();
+}
+
+// _closeLocked serves FileH.close and Conn.close.
+//
+// Must be called with the following locks held by caller:
+// - virt_lock
+// - wconn.atMu
+error _FileH::_closeLocked() {
+    _FileH& fileh = *this;
+    Conn    wconn = fileh.wconn;
+
     wconn->_filehMu.Lock();
     defer([&]() {
         wconn->_filehMu.Unlock();
-        wconn->_atMu.RUnlock();
     });
 
     // fileh.close can be called several times. just return nil for second close.

wcfs/client/wcfs.h

@@ -261,9 +261,10 @@ public:
 public:
     error close();
     pair<Mapping, error> mmap(int64_t blk_start, int64_t blk_len, VMA *vma=nil);
+    string String() const;
 
     error _open();
-    string String() const;
+    error _closeLocked();
 };
 
 // Mapping represents one memory mapping of FileH.