hammer2 - Major restructuring, part 2/several
authorMatthew Dillon <dillon@apollo.backplane.com>
Sun, 28 Apr 2013 23:24:56 +0000 (16:24 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Sun, 28 Apr 2013 23:28:25 +0000 (16:28 -0700)
* Stabilization pass on hammer2_inode_connect() and
  hammer2_hardlink_consolidate().  Adjust the API to accomodate
  requirements (primarily that the caller is responsible for holding
  the inode locked and for replacing ip->chain).

* Add hammer2_inode_repoint() to handle the guts of replacing ip->chain.

* Flush code shouldn't be messing the chain->bref when deleting the chain
  as the chain may be used in a duplication operation after the call.

* basic rm/mv/ln operations now work (note: we still need to code the
  reparenting of sub-chains based on chain->duplink, and hardlinks are
  still buggy when parent directories get renamed).

sys/vfs/hammer2/hammer2.h
sys/vfs/hammer2/hammer2_flush.c
sys/vfs/hammer2/hammer2_inode.c
sys/vfs/hammer2/hammer2_subr.c
sys/vfs/hammer2/hammer2_vnops.c

index 86ea530..fed1ad6 100644 (file)
@@ -436,6 +436,7 @@ void hammer2_inode_put(hammer2_inode_t *ip);
 void hammer2_inode_free(hammer2_inode_t *ip);
 void hammer2_inode_ref(hammer2_inode_t *ip);
 void hammer2_inode_drop(hammer2_inode_t *ip);
+void hammer2_inode_repoint(hammer2_inode_t *ip, hammer2_chain_t **chainp);
 int hammer2_inode_calc_alloc(hammer2_key_t filesize);
 
 hammer2_inode_t *hammer2_inode_create(hammer2_trans_t *trans,
@@ -447,7 +448,6 @@ hammer2_chain_t *hammer2_inode_duplicate(hammer2_trans_t *trans,
                        hammer2_chain_t *ochain,
                         hammer2_inode_t *dip, int *errorp);
 int hammer2_inode_connect(hammer2_trans_t *trans,
-                       hammer2_inode_t *ip,
                        hammer2_inode_t *dip,
                        hammer2_chain_t **chainp,
                        const uint8_t *name, size_t name_len);
index ea750a1..83bec77 100644 (file)
@@ -782,6 +782,14 @@ hammer2_chain_flush_scan2(hammer2_chain_t *child, void *data)
         * blockref updates do not touch modify_tid.  Instead, mirroring
         * operations always reconcile the entire array during their
         * mirror_tid based recursion.
+        *
+        * WARNING! Deleted chains may still be used by the filesystem
+        *          in a later duplication, for example in a rename()
+        *          operation.  Also any topological movement of the
+        *          related blocks.
+        *
+        *          We adjust the parent's bref pointer to the child but
+        *          we do not modify the contents of the child.
         */
        if (action == HC_DELETE) {
                if (base) {
@@ -805,11 +813,6 @@ hammer2_chain_flush_scan2(hammer2_chain_t *child, void *data)
        }
 
 cleanup:
-       /*
-        * Deletions should also zero-out the child's bref for safety.
-        */
-       if (action == HC_DELETE)
-               bzero(&child->bref, sizeof(child->bref));
 
        /*
         * Cleanup the child's MOVED flag and unlock the child.
index feff9e6..3266ba6 100644 (file)
@@ -584,6 +584,9 @@ retry:
  * Connect the target inode to the media topology at (dip, name, len).
  * This function creates a directory entry and replace (*chainp).
  *
+ * The caller usually holds the related inode exclusive locked through this
+ * call and is also responsible for replacing ip->chain after we return.
+ *
  * If (*chainp) was marked DELETED then it represents a terminus inode
  * with no other nlinks, we can simply duplicate the chain (in-memory
  * chain structures cannot be moved within the in-memory topology, only
@@ -602,14 +605,15 @@ retry:
  * might reside in some parent directory.  It will not be the
  * OBJTYPE_HARDLINK pointer.
  *
- * WARNING!  This function will also replace ip->chain.  The related inode
- *          must be locked exclusively or would wind up racing other
- *          modifying operations on the same inode which then wind up
- *          modifying under the old chain instead of the new chain.
+ * WARNING!  The caller is likely holding ip/ip->chain locked exclusively.
+ *          Replacing ip->chain here would create confusion so we leave
+ *          it to the caller to do that.
+ *
+ *          (The caller is expected to hold the related inode exclusively)
  */
 int
 hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip,
-                     hammer2_inode_t *ip, hammer2_chain_t **chainp,
+                     hammer2_chain_t **chainp,
                      const uint8_t *name, size_t name_len)
 {
        hammer2_inode_data_t *ipdata;
@@ -633,7 +637,7 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip,
        parent = hammer2_chain_lookup_init(dip->chain, 0);
 
        lhc = hammer2_dirhash(name, name_len);
-       hlink = ((ochain->flags & HAMMER2_CHAIN_DELETED) != 0);
+       hlink = ((ochain->flags & HAMMER2_CHAIN_DELETED) == 0);
        kprintf("reconnect hlink=%d name=%*.*s\n",
                hlink, (int)name_len, (int)name_len, name);
 
@@ -703,13 +707,10 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip,
        parent = NULL;
 
        /*
-        * ochain still active.
-        *
-        * Handle the error case
+        * nchain should be NULL on error, leave ochain (== *chainp) alone.
         */
        if (error) {
                KKASSERT(nchain == NULL);
-               hammer2_chain_unlock(ochain);
                return (error);
        }
 
@@ -726,6 +727,8 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip,
                /*
                 * Create the HARDLINK pointer.  oip represents the hardlink
                 * target in this situation.
+                *
+                * We will return ochain (the hardlink target).
                 */
                hammer2_chain_modify(trans, nchain, 0);
                KKASSERT(name_len < HAMMER2_INODE_MAXNAME);
@@ -779,21 +782,57 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip,
                }
                ipdata->nlinks = 1;
        }
+
+       /*
+        * We are replacing ochain with nchain, unlock ochain.  In the
+        * case where ochain is left unchanged the code above sets
+        * nchain to ochain and ochain to NULL, resulting in a NOP here.
+        */
        if (ochain)
                hammer2_chain_unlock(ochain);
        *chainp = nchain;
 
+       return (0);
+}
+
+/*
+ * Caller must hold exactly ONE exclusive lock on the inode.  *nchainp
+ * must be exclusive locked (its own exclusive lock even if it is the
+ * same as ip->chain).
+ *
+ * This function replaces ip->chain.  The exclusive lock on the passed
+ * nchain is inherited by the inode and the caller becomes responsible
+ * for unlocking it when the caller unlocks the inode.
+ *
+ * ochain was locked by the caller indirectly via the inode lock.  Since
+ * ip->chain is being repointed, we become responsible for cleaning up
+ * that lock.
+ *
+ * Return *nchainp = NULL as a safety.
+ */
+void
+hammer2_inode_repoint(hammer2_inode_t *ip, hammer2_chain_t **nchainp)
+{
+       hammer2_chain_t *nchain = *nchainp;
+       hammer2_chain_t *ochain;
+
        /*
-        * Replace ip->chain if necessary.  XXX inode sub-topology replacement.
+        * Repoint ip->chain if necessary.
+        *
+        * (Inode must be locked exclusively by parent)
         */
-       if (ip->chain != nchain) {
-               hammer2_chain_ref(nchain);                      /* ip->chain */
-               if (ip->chain)
-                       hammer2_chain_drop(ip->chain);          /* ip->chain */
+       ochain = ip->chain;
+       if (ochain != nchain) {
+               hammer2_chain_ref(nchain);              /* for ip->chain */
                ip->chain = nchain;
+               if (ochain) {
+                       hammer2_chain_unlock(ochain);
+                       hammer2_chain_drop(ochain);     /* for ip->chain */
+               }
+       } else {
+               hammer2_chain_unlock(nchain);
        }
-
-       return (0);
+       *nchainp = NULL;
 }
 
 /*
@@ -1019,11 +1058,17 @@ hammer2_inode_calc_alloc(hammer2_key_t filesize)
 }
 
 /*
- * Given an unlocked ip consolidate for hardlink creation, adding (nlinks)
- * to the file's link count and potentially relocating the file to a
- * directory common to ip->pip and tdip.
+ * Given an exclusively locked inode we consolidate its chain for hardlink
+ * creation, adding (nlinks) to the file's link count and potentially
+ * relocating the file to a directory common to ip->pip and tdip.
+ *
+ * Returns a locked chain in (*chainp) (the chain's lock is in addition to
+ * any lock it might already have due to the inode being locked).  *chainp
+ * is set unconditionally and its previous contents can be garbage.
  *
- * If the file has to be relocated ip->chain will also be adjusted.
+ * The caller is responsible for replacing ip->chain, not us.  For certain
+ * operations such as renames the caller may do additional manipulation
+ * of the chain before replacing ip->chain.
  */
 int
 hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
@@ -1043,7 +1088,6 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
         * Extra lock on chain so it can be returned locked.
         */
        hmp = tdip->hmp;
-       hammer2_inode_lock_ex(ip);
 
        chain = ip->chain;
        error = hammer2_chain_lock(chain, HAMMER2_RESOLVE_ALWAYS);
@@ -1051,18 +1095,15 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
 
        if (nlinks == 0 &&                      /* no hardlink needed */
            (chain->data->ipdata.name_key & HAMMER2_DIRHASH_VISIBLE)) {
-               hammer2_inode_unlock_ex(ip);
                *chainp = chain;
                return (0);
        }
        if (hammer2_hardlink_enable < 0) {      /* fake hardlinks */
-               hammer2_inode_unlock_ex(ip);
                *chainp = chain;
                return (0);
        }
 
        if (hammer2_hardlink_enable == 0) {     /* disallow hardlinks */
-               hammer2_inode_unlock_ex(ip);
                hammer2_chain_unlock(chain);
                *chainp = NULL;
                return (ENOTSUP);
@@ -1166,16 +1207,14 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
                }
 
                /*
-                * Replace ip->chain with nchain (ip is still locked).
+                * Return the new chain.
                 */
-               hammer2_chain_ref(nchain);              /* ip->chain */
-               if (ip->chain)
-                       hammer2_chain_drop(ip->chain);  /* ip->chain */
-               ip->chain = nchain;
-
                hammer2_chain_unlock(chain);
                *chainp = nchain;
        } else {
+               /*
+                * Return an error
+                */
                hammer2_chain_unlock(chain);
                *chainp = NULL;
        }
@@ -1184,7 +1223,6 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip,
         * Cleanup, chain/nchain already dealt with.
         */
 done:
-       hammer2_inode_unlock_ex(ip);
        hammer2_inode_drop(cdip);
 
        return (error);
index c3c9cd3..a725389 100644 (file)
@@ -55,6 +55,9 @@
  *
  * NOTE: We don't combine the inode/chain lock because putting away an
  *       inode would otherwise confuse multiple lock holders of the inode.
+ *
+ * WARNING! hammer2_inode_repoint() expects exactly one exclusive lock
+ *         on ip->chain.
  */
 void
 hammer2_inode_lock_ex(hammer2_inode_t *ip)
index cc1d6e1..76796b0 100644 (file)
@@ -1765,7 +1765,7 @@ hammer2_vop_nlink(struct vop_nlink_args *ap)
         * returned chain is locked.
         */
        ip = VTOI(ap->a_vp);
-       hammer2_inode_ref(ip);
+       hammer2_inode_lock_ex(ip);
        error = hammer2_hardlink_consolidate(&trans, ip, &chain, dip, 1);
        if (error)
                goto done;
@@ -1774,17 +1774,18 @@ hammer2_vop_nlink(struct vop_nlink_args *ap)
         * Create a directory entry connected to the specified chain.
         * This function unlocks and NULL's chain on return.
         */
-       error = hammer2_inode_connect(&trans, dip, ip, &chain, name, name_len);
-       if (chain) {
-               hammer2_chain_unlock(chain);
-               chain = NULL;
-       }
+       error = hammer2_inode_connect(&trans, dip, &chain, name, name_len);
        if (error == 0) {
+               hammer2_inode_repoint(ip, &chain);
                cache_setunresolved(ap->a_nch);
                cache_setvp(ap->a_nch, ap->a_vp);
        }
 done:
-       hammer2_inode_drop(ip);
+       if (chain) {
+               hammer2_chain_unlock(chain);
+               chain = NULL;
+       }
+       hammer2_inode_unlock_ex(ip);
        hammer2_trans_done(&trans);
 
        return error;
@@ -2031,7 +2032,7 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
        hammer2_trans_init(&trans, hmp);
 
        /*
-        * ip is the inode being removed.  If this is a hardlink then
+        * ip is the inode being renamed.  If this is a hardlink then
         * ip represents the actual file and not the hardlink marker.
         */
        ip = VTOI(fncp->nc_vp);
@@ -2066,6 +2067,7 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
         *
         * The returned chain will be locked.
         */
+       hammer2_inode_lock_ex(ip);
        error = hammer2_hardlink_consolidate(&trans, ip, &chain, tdip, 0);
        if (error)
                goto done;
@@ -2094,18 +2096,16 @@ hammer2_vop_nrename(struct vop_nrename_args *ap)
         *          op (that might have to lock a vnode).
         */
        error = hammer2_inode_connect(&trans, tdip,
-                                     ip, &chain,
-                                     tname, tname_len);
+                                     &chain, tname, tname_len);
        if (error == 0) {
-               if (chain) {
-                       hammer2_chain_unlock(chain);
-                       chain = NULL;
-               }
+               KKASSERT(chain != NULL);
+               hammer2_inode_repoint(ip, &chain);
                cache_rename(ap->a_fnch, ap->a_tnch);
        }
 done:
        if (chain)
                hammer2_chain_unlock(chain);
+       hammer2_inode_unlock_ex(ip);
        hammer2_inode_drop(ip);
        hammer2_trans_done(&trans);