hammer2 - Refactor file unlink w/open descriptor (2)
authorMatthew Dillon <dillon@apollo.backplane.com>
Thu, 12 Dec 2013 03:44:38 +0000 (19:44 -0800)
committerMatthew Dillon <dillon@apollo.backplane.com>
Thu, 12 Dec 2013 03:44:38 +0000 (19:44 -0800)
* Stabilization pass

* Fix various deadlocks.  When dealing with hardlinks lock the common
  parent directory before locking the source/target directories.
  Move cache_rename() outside of internal H2 locks.

* Fix an issue with nlinks counting.

* Remove some debugging.

sys/vfs/hammer2/hammer2.h
sys/vfs/hammer2/hammer2_chain.c
sys/vfs/hammer2/hammer2_inode.c
sys/vfs/hammer2/hammer2_vnops.c

index 1e0c7a0..bbacbc7 100644 (file)
@@ -734,8 +734,9 @@ hammer2_inode_t *hammer2_inode_create(hammer2_trans_t *trans,
                        struct vattr *vap, struct ucred *cred,
                        const uint8_t *name, size_t name_len,
                        hammer2_chain_t **chainp, int *errorp);
-int hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
-                       hammer2_inode_t *dip, hammer2_chain_t **chainp,
+int hammer2_inode_connect(hammer2_trans_t *trans,
+                       hammer2_chain_t **chainp, int hlink,
+                       hammer2_inode_t *dip, hammer2_chain_t **dchainp,
                        const uint8_t *name, size_t name_len,
                        hammer2_key_t key);
 hammer2_inode_t *hammer2_inode_common_parent(hammer2_inode_t *fdip,
@@ -745,9 +746,10 @@ void hammer2_inode_fsync(hammer2_trans_t *trans, hammer2_inode_t *ip,
 int hammer2_unlink_file(hammer2_trans_t *trans, hammer2_inode_t *dip,
                        const uint8_t *name, size_t name_len, int isdir,
                        int *hlinkp, struct nchandle *nch);
-int hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
-                       hammer2_chain_t **chainp,
-                       hammer2_inode_t *tdip, int linkcnt);
+int hammer2_hardlink_consolidate(hammer2_trans_t *trans,
+                       hammer2_inode_t *ip, hammer2_chain_t **chainp,
+                       hammer2_inode_t *cdip, hammer2_chain_t **cdchainp,
+                       int nlinks);
 int hammer2_hardlink_deconsolidate(hammer2_trans_t *trans, hammer2_inode_t *dip,
                        hammer2_chain_t **chainp, hammer2_chain_t **ochainp);
 int hammer2_hardlink_find(hammer2_inode_t *dip,
index 8ea0e9c..5ad7198 100644 (file)
@@ -4608,7 +4608,9 @@ hammer2_chain_memory_wait(hammer2_pfsmount_t *pmp)
        long waiting;
        long count;
        long limit;
+#if 0
        static int zzticks;
+#endif
 
        /*
         * Atomic check condition and wait.  Also do an early speedup of
@@ -4625,10 +4627,12 @@ hammer2_chain_memory_wait(hammer2_pfsmount_t *pmp)
                if (limit < 1000)
                        limit = 1000;
 
+#if 0
                if ((int)(ticks - zzticks) > hz) {
                        zzticks = ticks;
                        kprintf("count %ld %ld\n", count, limit);
                }
+#endif
 
                /*
                 * Block if there are too many dirty chains present, wait
index a476063..06f74b6 100644 (file)
@@ -103,7 +103,8 @@ hammer2_inode_lock_ex(hammer2_inode_t *ip)
                        break;
                hammer2_chain_unlock(chain);
        }
-       if (chain->data->ipdata.type == HAMMER2_OBJTYPE_HARDLINK) {
+       if (chain->data->ipdata.type == HAMMER2_OBJTYPE_HARDLINK &&
+           (chain->flags & HAMMER2_CHAIN_DELETED) == 0) {
                error = hammer2_hardlink_find(ip->pip, &chain, &ochain);
                hammer2_chain_drop(ochain);
                KKASSERT(error == 0);
@@ -763,10 +764,10 @@ hammer2_chain_refactor(hammer2_chain_t **chainp)
 static
 void
 hammer2_hardlink_shiftup(hammer2_trans_t *trans, hammer2_chain_t **chainp,
-                       hammer2_inode_t *dip, int nlinks, int *errorp)
+                       hammer2_inode_t *dip, hammer2_chain_t **dchainp,
+                       int nlinks, int *errorp)
 {
        hammer2_inode_data_t *nipdata;
-       hammer2_chain_t *parent;
        hammer2_chain_t *chain;
        hammer2_chain_t *xchain;
        hammer2_key_t key_dummy;
@@ -790,13 +791,11 @@ hammer2_hardlink_shiftup(hammer2_trans_t *trans, hammer2_chain_t **chainp,
         */
 retry:
        *errorp = 0;
-       parent = hammer2_inode_lock_ex(dip);
-       /*parent = hammer2_chain_lookup_init(dip->chain, 0);*/
-       xchain = hammer2_chain_lookup(&parent, &key_dummy,
+       xchain = hammer2_chain_lookup(dchainp, &key_dummy,
                                      lhc, lhc, &cache_index, 0);
        if (xchain) {
-               kprintf("X3 chain %p parent %p dip %p dip->chain %p\n",
-                       xchain, parent, dip, dip->chain);
+               kprintf("X3 chain %p dip %p dchain %p dip->chain %p\n",
+                       xchain, dip, *dchainp, dip->chain);
                hammer2_chain_unlock(xchain);
                xchain = NULL;
                *errorp = ENOSPC;
@@ -815,7 +814,7 @@ retry:
        if (*errorp == 0) {
                KKASSERT(xchain == NULL);
 #if 0
-               *errorp = hammer2_chain_create(trans, &parent, &xchain,
+               *errorp = hammer2_chain_create(trans, dchainp, &xchain,
                                               lhc, 0,
                                               HAMMER2_BREF_TYPE_INODE,/* n/a */
                                               HAMMER2_INODE_BYTES);   /* n/a */
@@ -828,11 +827,9 @@ retry:
         * Cleanup and handle retries.
         */
        if (*errorp == EAGAIN) {
-               hammer2_chain_ref(parent);
-               /* hammer2_chain_lookup_done(parent); */
-               hammer2_inode_unlock_ex(dip, parent);
-               hammer2_chain_wait(parent);
-               hammer2_chain_drop(parent);
+               kprintf("R");
+               hammer2_chain_wait(*dchainp);
+               hammer2_chain_drop(*dchainp);
                goto retry;
        }
 
@@ -842,8 +839,6 @@ retry:
        if (*errorp) {
                panic("error2");
                KKASSERT(xchain == NULL);
-               hammer2_inode_unlock_ex(dip, parent);
-               /*hammer2_chain_lookup_done(parent);*/
                return;
        }
 
@@ -859,20 +854,7 @@ retry:
        bref = chain->bref;
        bref.key = lhc;                 /* invisible dir entry key */
        bref.keybits = 0;
-#if 0
-       hammer2_chain_delete(trans, xchain, 0);
-#endif
-       hammer2_chain_duplicate(trans, &parent, &chain, &bref, 0, 2);
-#if 0
-       hammer2_chain_refactor(&xchain);
-       /*hammer2_chain_delete(trans, xchain, 0);*/
-#endif
-
-       hammer2_inode_unlock_ex(dip, parent);
-       /*hammer2_chain_lookup_done(parent);*/
-#if 0
-       hammer2_chain_unlock(xchain);   /* no longer needed */
-#endif
+       hammer2_chain_duplicate(trans, dchainp, &chain, &bref, 0, 2);
 
        /*
         * chain is now 'live' again.. adjust the filename.
@@ -904,14 +886,14 @@ retry:
  * by the caller in this case (e.g. rename).
  */
 int
-hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
-                     hammer2_inode_t *dip, hammer2_chain_t **chainp,
+hammer2_inode_connect(hammer2_trans_t *trans,
+                     hammer2_chain_t **chainp, int hlink,
+                     hammer2_inode_t *dip, hammer2_chain_t **dchainp,
                      const uint8_t *name, size_t name_len,
                      hammer2_key_t lhc)
 {
        hammer2_inode_data_t *ipdata;
        hammer2_chain_t *nchain;
-       hammer2_chain_t *parent;
        hammer2_chain_t *ochain;
        hammer2_key_t key_dummy;
        int cache_index = -1;
@@ -926,8 +908,6 @@ hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
         *          dip->chain cache.
         */
        ochain = *chainp;
-       parent = hammer2_inode_lock_ex(dip);
-       /*parent = hammer2_chain_lookup_init(dip->chain, 0);*/
 
        /*
         * If name is non-NULL we calculate lhc, else we use the passed-in
@@ -943,7 +923,7 @@ hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
                 */
                error = 0;
                while (error == 0) {
-                       nchain = hammer2_chain_lookup(&parent, &key_dummy,
+                       nchain = hammer2_chain_lookup(dchainp, &key_dummy,
                                                      lhc, lhc,
                                                      &cache_index, 0);
                        if (nchain == NULL)
@@ -969,7 +949,7 @@ hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
                         * create.
                         */
                        KKASSERT(nchain == NULL);
-                       error = hammer2_chain_create(trans, &parent, &nchain,
+                       error = hammer2_chain_create(trans, dchainp, &nchain,
                                                     lhc, 0,
                                                     HAMMER2_BREF_TYPE_INODE,
                                                     HAMMER2_INODE_BYTES);
@@ -994,7 +974,7 @@ hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
                        ochain = NULL;
                        hammer2_chain_duplicate(trans, NULL, &nchain, NULL,
                                                0, 3);
-                       error = hammer2_chain_create(trans, &parent, &nchain,
+                       error = hammer2_chain_create(trans, dchainp, &nchain,
                                                     lhc, 0,
                                                     HAMMER2_BREF_TYPE_INODE,
                                                     HAMMER2_INODE_BYTES);
@@ -1005,9 +985,6 @@ hammer2_inode_connect(hammer2_trans_t *trans, int hlink,
         * Unlock stuff.
         */
        KKASSERT(error != EAGAIN);
-       hammer2_inode_unlock_ex(dip, parent);
-       /*hammer2_chain_lookup_done(parent);*/
-       parent = NULL;
 
        /*
         * nchain should be NULL on error, leave ochain (== *chainp) alone.
@@ -1219,8 +1196,8 @@ hammer2_unlink_file(hammer2_trans_t *trans, hammer2_inode_t *dip,
        }
 
        /*
-        * Hardlink must be resolved.  We can't hold parent locked while we
-        * do this or we could deadlock.
+        * Hardlink must be resolved.  We can't hold the parent locked
+        * while we do this or we could deadlock.
         *
         * On success chain will be adjusted to point at the hardlink target
         * and ochain will point to the hardlink pointer in the original
@@ -1296,11 +1273,15 @@ hammer2_unlink_file(hammer2_trans_t *trans, hammer2_inode_t *dip,
         *       (which does not represent a ref for the open-test), and to
         *       force finalization of the vnode if/when the last ref gets
         *       dropped.
+        *
+        * NOTE! Files are unlinked by rename and then relinked.  nch will be
+        *       passed as NULL in this situation.  hammer2_inode_connect()
+        *       will bump nlinks.
         */
+       KKASSERT(chain != NULL);
        hammer2_chain_modify(trans, &chain, 0);
        ipdata = &chain->data->ipdata;
        --ipdata->nlinks;
-       kprintf("file %s nlinks %ld\n", ipdata->filename, ipdata->nlinks);
        if ((int64_t)ipdata->nlinks < 0)        /* XXX debugging */
                ipdata->nlinks = 0;
        if (ipdata->nlinks == 0) {
@@ -1430,6 +1411,7 @@ hammer2_inode_move_to_hidden(hammer2_trans_t *trans, hammer2_chain_t **chainp,
                             hammer2_tid_t inum)
 {
        hammer2_chain_t *chain;
+       hammer2_chain_t *dchain;
        hammer2_pfsmount_t *pmp;
        int error;
 
@@ -1439,9 +1421,11 @@ hammer2_inode_move_to_hidden(hammer2_trans_t *trans, hammer2_chain_t **chainp,
        KKASSERT(pmp->ihidden != NULL);
 
        hammer2_chain_delete(trans, chain, 0);
-        error = hammer2_inode_connect(trans, 0,
-                                      pmp->ihidden, chainp,
+       dchain = hammer2_inode_lock_ex(pmp->ihidden);
+        error = hammer2_inode_connect(trans, chainp, 0,
+                                      pmp->ihidden, &dchain,
                                      NULL, 0, inum);
+       hammer2_inode_unlock_ex(pmp->ihidden, dchain);
        KKASSERT(error == 0);
 }
 
@@ -1456,13 +1440,12 @@ hammer2_inode_move_to_hidden(hammer2_trans_t *trans, hammer2_chain_t **chainp,
  * NOTE!  This function will also replace ip->chain.
  */
 int
-hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
-                            hammer2_chain_t **chainp,
-                            hammer2_inode_t *tdip, int nlinks)
+hammer2_hardlink_consolidate(hammer2_trans_t *trans,
+                            hammer2_inode_t *ip, hammer2_chain_t **chainp,
+                            hammer2_inode_t *cdip, hammer2_chain_t **cdchainp,
+                            int nlinks)
 {
        hammer2_inode_data_t *ipdata;
-       hammer2_inode_t *fdip;
-       hammer2_inode_t *cdip;
        hammer2_chain_t *chain;
        hammer2_chain_t *nchain;
        int error;
@@ -1483,21 +1466,11 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
        }
 
        /*
-        * cdip will be returned with a ref, but not locked.
-        */
-       fdip = ip->pip;
-       cdip = hammer2_inode_common_parent(fdip, tdip);
-
-       /*
         * If no change in the hardlink's target directory is required and
         * this is already a hardlink target, all we need to do is adjust
         * the link count.
-        *
-        * XXX The common parent is a big wiggly due to duplication from
-        *     renames.  Compare the core (RBTREE) pointer instead of the
-        *     ip's.
         */
-       if (cdip == fdip &&
+       if (cdip == ip->pip &&
            (chain->data->ipdata.name_key & HAMMER2_DIRHASH_VISIBLE) == 0) {
                if (nlinks) {
                        hammer2_chain_modify(trans, &chain, 0);
@@ -1573,7 +1546,7 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
         *          to the older/original version.
         */
        KKASSERT(chain->flags & HAMMER2_CHAIN_DELETED);
-       hammer2_hardlink_shiftup(trans, &chain, cdip, nlinks, &error);
+       hammer2_hardlink_shiftup(trans, &chain, cdip, cdchainp, nlinks, &error);
 
        if (error == 0)
                hammer2_inode_repoint(ip, cdip, chain);
index bbf3d8c..38cfb1c 100644 (file)
@@ -336,7 +336,8 @@ hammer2_vop_reclaim(struct vop_reclaim_args *ap)
        vp->v_data = NULL;
        ip->vp = NULL;
        if (chain->flags & HAMMER2_CHAIN_UNLINKED) {
-               kprintf("unlink on reclaim: %s\n", chain->data->ipdata.filename);
+               kprintf("unlink on reclaim: %s\n",
+                       chain->data->ipdata.filename);
                hammer2_trans_init(&trans, ip->pmp, NULL,
                                   HAMMER2_TRANS_BUFCACHE);
                hammer2_chain_delete(&trans, chain, 0);
@@ -1486,17 +1487,22 @@ static
 int
 hammer2_vop_nlink(struct vop_nlink_args *ap)
 {
-       hammer2_inode_t *dip;   /* target directory to create link in */
+       hammer2_inode_t *fdip;  /* target directory to create link in */
+       hammer2_inode_t *tdip;  /* target directory to create link in */
+       hammer2_inode_t *cdip;  /* common parent directory */
        hammer2_inode_t *ip;    /* inode we are hardlinking to */
        hammer2_chain_t *chain;
+       hammer2_chain_t *fdchain;
+       hammer2_chain_t *tdchain;
+       hammer2_chain_t *cdchain;
        hammer2_trans_t trans;
        struct namecache *ncp;
        const uint8_t *name;
        size_t name_len;
        int error;
 
-       dip = VTOI(ap->a_dvp);
-       if (dip->pmp->ronly)
+       tdip = VTOI(ap->a_dvp);
+       if (tdip->pmp->ronly)
                return (EROFS);
 
        ncp = ap->a_nch->ncp;
@@ -1510,7 +1516,7 @@ hammer2_vop_nlink(struct vop_nlink_args *ap)
         * of the hardlinked target, NOT the location of the hardlink pointer.
         *
         * Bump nlinks and potentially also create or move the hardlink
-        * target in the parent directory common to (ip) and (dip).  The
+        * target in the parent directory common to (ip) and (tdip).  The
         * consolidation code can modify ip->chain and ip->pip.  The
         * returned chain is locked.
         */
@@ -1518,8 +1524,18 @@ hammer2_vop_nlink(struct vop_nlink_args *ap)
        hammer2_chain_memory_wait(ip->pmp);
        hammer2_trans_init(&trans, ip->pmp, NULL, HAMMER2_TRANS_NEWINODE);
 
+       /*
+        * The common parent directory must be locked first to avoid deadlocks.
+        * Also note that fdip and/or tdip might match cdip.
+        */
+       fdip = ip->pip;
+       cdip = hammer2_inode_common_parent(fdip, tdip);
+       cdchain = hammer2_inode_lock_ex(cdip);
+       fdchain = hammer2_inode_lock_ex(fdip);
+       tdchain = hammer2_inode_lock_ex(tdip);
        chain = hammer2_inode_lock_ex(ip);
-       error = hammer2_hardlink_consolidate(&trans, ip, &chain, dip, 1);
+       error = hammer2_hardlink_consolidate(&trans, ip, &chain,
+                                            cdip, &cdchain, 1);
        if (error)
                goto done;
 
@@ -1535,8 +1551,8 @@ hammer2_vop_nlink(struct vop_nlink_args *ap)
         * WARNING! chain can get moved by the connect (indirectly due to
         *          potential indirect block creation).
         */
-       error = hammer2_inode_connect(&trans, 1,
-                                     dip, &chain,
+       error = hammer2_inode_connect(&trans, &chain, 1,
+                                     tdip, &tdchain,
                                      name, name_len, 0);
        if (error == 0) {
                cache_setunresolved(ap->a_nch);
@@ -1544,6 +1560,9 @@ hammer2_vop_nlink(struct vop_nlink_args *ap)
        }
 done:
        hammer2_inode_unlock_ex(ip, chain);
+       hammer2_inode_unlock_ex(tdip, tdchain);
+       hammer2_inode_unlock_ex(fdip, fdchain);
+       hammer2_inode_unlock_ex(cdip, cdchain);
        hammer2_trans_done(&trans);
 
        return error;
@@ -1760,10 +1779,8 @@ hammer2_vop_nremove(struct vop_nremove_args *ap)
        error = hammer2_unlink_file(&trans, dip, name, name_len,
                                    0, NULL, ap->a_nch);
        hammer2_trans_done(&trans);
-       if (error == 0) {
-               kprintf("cache_unlink\n");
+       if (error == 0)
                cache_unlink(ap->a_nch);
-       }
        return (error);
 }
 
@@ -1794,9 +1811,8 @@ hammer2_vop_nrmdir(struct vop_nrmdir_args *ap)
        error = hammer2_unlink_file(&trans, dip, name, name_len,
                                    1, NULL, ap->a_nch);
        hammer2_trans_done(&trans);
-       if (error == 0) {
+       if (error == 0)
                cache_unlink(ap->a_nch);
-       }
        return (error);
 }
 
@@ -1809,10 +1825,14 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
 {
        struct namecache *fncp;
        struct namecache *tncp;
+       hammer2_inode_t *cdip;
        hammer2_inode_t *fdip;
        hammer2_inode_t *tdip;
        hammer2_inode_t *ip;
        hammer2_chain_t *chain;
+       hammer2_chain_t *fdchain;
+       hammer2_chain_t *tdchain;
+       hammer2_chain_t *cdchain;
        hammer2_trans_t trans;
        const uint8_t *fname;
        size_t fname_len;
@@ -1850,6 +1870,27 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
        ip = VTOI(fncp->nc_vp);
        chain = NULL;
 
+
+       /*
+        * The common parent directory must be locked first to avoid deadlocks.
+        * Also note that fdip and/or tdip might match cdip.
+        *
+        * WARNING! fdip may not match ip->pip.  That is, if the source file
+        *          is already a hardlink then what we are renaming is the
+        *          hardlink pointer, not the hardlink itself.  The hardlink
+        *          directory (ip->pip) will already be at a common parent
+        *          of fdrip.
+        *
+        *          Be sure to use ip->pip when finding the common parent
+        *          against tdip or we might accidently move the hardlink
+        *          target into a subdirectory that makes it inaccessible to
+        *          other pointers.
+        */
+       cdip = hammer2_inode_common_parent(ip->pip, tdip);
+       cdchain = hammer2_inode_lock_ex(cdip);
+       fdchain = hammer2_inode_lock_ex(fdip);
+       tdchain = hammer2_inode_lock_ex(tdip);
+
        /*
         * Keep a tight grip on the inode so the temporary unlinking from
         * the source location prior to linking to the target location
@@ -1872,8 +1913,8 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
 
        /*
         * When renaming a hardlinked file we may have to re-consolidate
-        * the location of the hardlink target.  Since the element is simply
-        * being moved, nlinks is not modified in this case.
+        * the location of the hardlink target.  Also adjust nlinks by +1
+        * to counter-act the unlink below.
         *
         * If ip represents a regular file the consolidation code essentially
         * does nothing other than return the same locked chain that was
@@ -1886,7 +1927,8 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
         *           on any modification to the inode, including connects.
         */
        chain = hammer2_inode_lock_ex(ip);
-       error = hammer2_hardlink_consolidate(&trans, ip, &chain, tdip, 0);
+       error = hammer2_hardlink_consolidate(&trans, ip, &chain,
+                                            cdip, &cdchain, 1);
        if (error)
                goto done;
 
@@ -1924,20 +1966,29 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
         *          op (that might have to lock a vnode).
         */
        hammer2_chain_refactor(&chain);
-       error = hammer2_inode_connect(&trans, hlink,
-                                     tdip, &chain,
+       error = hammer2_inode_connect(&trans, &chain, hlink,
+                                     tdip, &tdchain,
                                      tname, tname_len, 0);
        chain->inode_reason = 5;
        if (error == 0) {
                KKASSERT(chain != NULL);
                hammer2_inode_repoint(ip, (hlink ? ip->pip : tdip), chain);
-               cache_rename(ap->a_fnch, ap->a_tnch);
        }
 done:
        hammer2_inode_unlock_ex(ip, chain);
+       hammer2_inode_unlock_ex(tdip, tdchain);
+       hammer2_inode_unlock_ex(fdip, fdchain);
+       hammer2_inode_unlock_ex(cdip, cdchain);
        hammer2_inode_drop(ip);
        hammer2_trans_done(&trans);
 
+       /*
+        * Issue the namecache update after unlocking all the internal
+        * hammer structures, otherwise we might deadlock.
+        */
+       if (error == 0)
+               cache_rename(ap->a_fnch, ap->a_tnch);
+
        return (error);
 }