hammer2 - Fix lost flush
authorMatthew Dillon <dillon@apollo.backplane.com>
Sun, 20 May 2012 18:12:58 +0000 (11:12 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Sun, 20 May 2012 18:12:58 +0000 (11:12 -0700)
* hammer2 allows the buffer cache buffers related to MODIFIED but unlocked
  chains to be retired by the OS.  In this situation hammer2 does not want
  to bdwrite() the buffer again unless additional modifications are made,
  even though the MODIFIED bit in the chain remains set throughout the
  entire sequence.

* Fix a case where these additional modifications were not properly flagging
  for the buffer cache buffer to be retired with a bdwrite(), causing data
  loss.  This is related to the DIRTYBP chain flag.

* Make further adjustments to the DIRTYBP chain flag.

* Also fix a case where the MOVED bit might not get properly set when a
  block is resized.  The problem was masked by the fact that a resize
  only occurs on data blocks and only during a write(), so the related
  buffer was being marked MODIFIED anyway.  However, the resize code still
  needed to be corrected.

* Add some debugging to 'hammer2 stat' to make it easier to poke around
  related kernel structures.

sbin/hammer2/cmd_stat.c
sys/vfs/hammer2/hammer2_chain.c
sys/vfs/hammer2/hammer2_inode.c
sys/vfs/hammer2/hammer2_ioctl.c
sys/vfs/hammer2/hammer2_ioctl.h

index 306418b..53e03ce 100644 (file)
@@ -58,7 +58,7 @@ cmd_stat(int ac, const char **av)
        }
        if (w < 16)
                w = 16;
-       printf("%-*.*s ncp data-use  inode-use\n", w, w, "PATH");
+       printf("%-*.*s ncp data-use  inode-use kaddr\n", w, w, "PATH");
        for (i = 0; i < ac; ++i) {
                if ((fd = open(av[i], O_RDONLY)) < 0) {
                        fprintf(stderr, "%s: %s\n", av[i], strerror(errno));
@@ -74,6 +74,7 @@ cmd_stat(int ac, const char **av)
                printf("%3d ", ino.ip_data.ncopies);
                printf("%9s ", sizetostr(ino.ip_data.data_count));
                printf("%9s ", sizetostr(ino.ip_data.inode_count));
+               printf("%p ", ino.kdata);
                if (ino.ip_data.data_quota || ino.ip_data.inode_quota) {
                        printf(" quota ");
                        printf("%12s", sizetostr(ino.ip_data.data_quota));
index 808172c..2f8cb21 100644 (file)
@@ -386,12 +386,12 @@ hammer2_chain_lock(hammer2_mount_t *hmp, hammer2_chain_t *chain, int how)
 
        /*
         * Zero the data area if the chain is in the INITIAL-create state.
+        * Mark the buffer for bdwrite().
         */
        bdata = (char *)chain->bp->b_data + boff;
        if (chain->flags & HAMMER2_CHAIN_INITIAL) {
                bzero(bdata, chain->bytes);
-               chain->bp->b_flags |= B_CACHE;
-               bdirty(chain->bp);
+               atomic_set_int(&chain->flags, HAMMER2_CHAIN_DIRTYBP);
        }
 
        /*
@@ -454,6 +454,8 @@ hammer2_chain_unlock(hammer2_mount_t *hmp, hammer2_chain_t *chain)
 
        /*
         * Undo a recursive lock
+        *
+        * XXX shared locks not handled properly
         */
        if (lockcountnb(&chain->lk) > 1) {
                KKASSERT(chain->refs > 1);
@@ -464,9 +466,14 @@ hammer2_chain_unlock(hammer2_mount_t *hmp, hammer2_chain_t *chain)
 
        /*
         * Shortcut the case if the data is embedded or not resolved.
+        *
         * Do NOT null-out pointers to embedded data (e.g. inode).
+        *
+        * The DIRTYBP flag is non-applicable in this situation and can
+        * be cleared to keep the flags state clean.
         */
        if (chain->bp == NULL) {
+               atomic_clear_int(&chain->flags, HAMMER2_CHAIN_DIRTYBP);
                lockmgr(&chain->lk, LK_RELEASE);
                hammer2_chain_drop(hmp, chain);
                return;
@@ -521,6 +528,12 @@ hammer2_chain_unlock(hammer2_mount_t *hmp, hammer2_chain_t *chain)
        if (chain->bref.type == HAMMER2_BREF_TYPE_DATA)
                chain->bp->b_flags |= B_RELBUF;
 
+       /*
+        * The DIRTYBP flag tracks whether we have to bdwrite() the buffer
+        * or not.  The flag will get re-set when chain_modify() is called,
+        * even if MODIFIED is already set, allowing the OS to retire the
+        * buffer independent of a hammer2 flus.
+        */
        chain->data = NULL;
        if (chain->flags & HAMMER2_CHAIN_DIRTYBP) {
                atomic_clear_int(&chain->flags, HAMMER2_CHAIN_DIRTYBP);
@@ -663,9 +676,8 @@ hammer2_chain_resize(hammer2_inode_t *ip, hammer2_chain_t *chain,
 
                /*
                 * NOTE: The INITIAL state of the chain is left intact.
-                *
-                * NOTE: Because of the reallocation we have to set DIRTYBP
-                *       if INITIAL is not set.
+                *       We depend on hammer2_chain_modify() to do the
+                *       right thing.
                 *
                 * NOTE: We set B_NOCACHE to throw away the previous bp and
                 *       any VM backing store, even if it was dirty.
@@ -676,8 +688,16 @@ hammer2_chain_resize(hammer2_inode_t *ip, hammer2_chain_t *chain,
                brelse(chain->bp);
                chain->bp = nbp;
                chain->data = (void *)bdata;
-               if ((chain->flags & HAMMER2_CHAIN_INITIAL) == 0)
-                       atomic_set_int(&chain->flags, HAMMER2_CHAIN_DIRTYBP);
+               hammer2_chain_modify(hmp, chain, 0);
+       }
+
+       /*
+        * Make sure the chain is marked MOVED and SUBMOD is set in the
+        * parent(s) so the adjustments are picked up by flush.
+        */
+       if ((chain->flags & HAMMER2_CHAIN_MOVED) == 0) {
+               hammer2_chain_ref(hmp, chain);
+               atomic_set_int(&chain->flags, HAMMER2_CHAIN_MOVED);
        }
        hammer2_chain_parent_setsubmod(hmp, chain);
 }
@@ -713,12 +733,19 @@ hammer2_chain_modify(hammer2_mount_t *hmp, hammer2_chain_t *chain, int flags)
 
        /*
         * If the chain is already marked MODIFIED we can just return.
+        *
+        * However, it is possible that a prior lock/modify sequence
+        * retired the buffer.  During this lock/modify sequence MODIFIED
+        * may still be set but the buffer could wind up clean.  Since
+        * the caller is going to modify the buffer further we have to
+        * be sure that DIRTYBP is set again.
         */
        if (chain->flags & HAMMER2_CHAIN_MODIFIED) {
                if ((flags & HAMMER2_MODIFY_OPTDATA) == 0 &&
                    chain->bp == NULL) {
                        goto skip1;
                }
+               atomic_set_int(&chain->flags, HAMMER2_CHAIN_DIRTYBP);
                return;
        }
 
index 45f541f..48d065b 100644 (file)
@@ -674,6 +674,11 @@ hammer2_unlink_file(hammer2_inode_t *dip,
         * If this is a directory the directory must be empty.  However, if
         * isdir < 0 we are doing a rename and the directory does not have
         * to be empty.
+        *
+        * NOTE: We check the full key range here which covers both visible
+        *       and invisible entries.  Theoretically there should be no
+        *       invisible (hardlink target) entries if there are no visible
+        *       entries.
         */
        if (type == HAMMER2_OBJTYPE_DIRECTORY && isdir >= 0) {
                dparent = chain;
index e31ef5c..ffe1b0a 100644 (file)
@@ -403,6 +403,7 @@ hammer2_ioctl_inode_get(hammer2_inode_t *ip, void *data)
 
        hammer2_inode_lock_sh(ip);
        ino->ip_data = ip->ip_data;
+       ino->kdata = ip;
        hammer2_inode_unlock_sh(ip);
        return (0);
 }
index 175e211..8c0f3e6 100644 (file)
@@ -99,6 +99,7 @@ typedef struct hammer2_ioc_pfs hammer2_ioc_pfs_t;
  */
 struct hammer2_ioc_inode {
        uint32_t                flags;
+       void                    *kdata;
        hammer2_inode_data_t    ip_data;
 };