From 74779181703092463a6f85210dd4fd6e6594ccd6 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Thu, 21 Jul 2016 22:48:10 -0700 Subject: [PATCH] hammer2 - Fix deadlocks, bad assertion, improve flushing. * Fix a deadlock in checkdirempty(). We must release the lock on oparent before following a hardlink. If after re-locking chain->parent != oparent, return EAGAIN to the caller. * When doing a full filesystem flush, pre-flush the vnodes with a normal transaction to try to soak-up all the compression time and avoid stalling user process writes for too long once we get inside the formal flush. * Fix a flush bug. Flushing a deleted chain is allowed if it is an inode. --- sys/vfs/hammer2/hammer2_chain.c | 6 ++---- sys/vfs/hammer2/hammer2_strategy.c | 8 +++++++- sys/vfs/hammer2/hammer2_vfsops.c | 5 +++-- sys/vfs/hammer2/hammer2_xops.c | 39 ++++++++++++++++++++++++++++++++++---- 4 files changed, 47 insertions(+), 11 deletions(-) diff --git a/sys/vfs/hammer2/hammer2_chain.c b/sys/vfs/hammer2/hammer2_chain.c index ae9d3cc2b8..5dcd74b3c3 100644 --- a/sys/vfs/hammer2/hammer2_chain.c +++ b/sys/vfs/hammer2/hammer2_chain.c @@ -4983,14 +4983,12 @@ hammer2_chain_testcheck(hammer2_chain_t *chain, void *bdata) /* * The caller presents a shared-locked (parent, chain) where the chain - * is of type HAMMER2_OBJTYPE_HARDLINK. The caller must hold the ip - * structure representing the inode locked to prevent - * consolidation/deconsolidation races. + * is of type HAMMER2_OBJTYPE_HARDLINK. * * The flags passed in are LOOKUP flags, not RESOLVE flags. Only * HAMMER2_LOOKUP_SHARED is supported. * - * We locate the hardlink in the current or a common parent directory. + * We locate the actual inode chain & parent. * * If we are unable to locate the hardlink, EIO is returned and * (*chainp) is unlocked and dropped. diff --git a/sys/vfs/hammer2/hammer2_strategy.c b/sys/vfs/hammer2/hammer2_strategy.c index f481bc5641..079ce1aaca 100644 --- a/sys/vfs/hammer2/hammer2_strategy.c +++ b/sys/vfs/hammer2/hammer2_strategy.c @@ -689,7 +689,13 @@ retry: &cache_index, HAMMER2_LOOKUP_NODATA); - if (chain && (chain->flags & HAMMER2_CHAIN_DELETED)) { + /* + * The lookup code should not return a DELETED chain to us, unless + * its a short-file embedded in the inode. Then it is possible for + * the lookup to return a deleted inode. + */ + if (chain && (chain->flags & HAMMER2_CHAIN_DELETED) && + chain->bref.type != HAMMER2_BREF_TYPE_INODE) { kprintf("assign physical deleted chain @ " "%016jx (%016jx.%02x) ip %016jx\n", lbase, chain->bref.data_off, chain->bref.type, diff --git a/sys/vfs/hammer2/hammer2_vfsops.c b/sys/vfs/hammer2/hammer2_vfsops.c index 92972553b1..165a87d6a1 100644 --- a/sys/vfs/hammer2/hammer2_vfsops.c +++ b/sys/vfs/hammer2/hammer2_vfsops.c @@ -2053,10 +2053,11 @@ hammer2_vfs_sync(struct mount *mp, int waitfor) if (waitfor & MNT_LAZY) flags |= VMSC_ONEPASS; -#if 0 +#if 1 /* * Preflush the vnodes using a normal transaction before interlocking - * with a flush transaction. + * with a flush transaction. We do this to try to run as much of + * the compression as possible outside the flush transaction. */ hammer2_trans_init(pmp, 0); info.error = 0; diff --git a/sys/vfs/hammer2/hammer2_xops.c b/sys/vfs/hammer2/hammer2_xops.c index 4300085c0c..69619697fa 100644 --- a/sys/vfs/hammer2/hammer2_xops.c +++ b/sys/vfs/hammer2/hammer2_xops.c @@ -62,19 +62,26 @@ /* * Determine if the specified directory is empty. Returns 0 on success. + * + * May return 0, ENOTDIR, or EAGAIN. */ static int -checkdirempty(hammer2_chain_t *parent, hammer2_chain_t *chain, int clindex) +checkdirempty(hammer2_chain_t *oparent, hammer2_chain_t *ochain, int clindex) { + hammer2_chain_t *parent; + hammer2_chain_t *chain; hammer2_key_t key_next; int cache_index = -1; int error; error = 0; - chain = hammer2_chain_lookup_init(chain, 0); + chain = hammer2_chain_lookup_init(ochain, 0); if (chain->data->ipdata.meta.type == HAMMER2_OBJTYPE_HARDLINK) { + if (oparent) + hammer2_chain_unlock(oparent); + parent = NULL; error = hammer2_chain_hardlink_find(&parent, &chain, clindex, 0); @@ -82,6 +89,18 @@ checkdirempty(hammer2_chain_t *parent, hammer2_chain_t *chain, int clindex) hammer2_chain_unlock(parent); hammer2_chain_drop(parent); } + if (oparent) { + hammer2_chain_lock(oparent, HAMMER2_RESOLVE_ALWAYS); + if (ochain->parent != oparent) { + if (chain) { + hammer2_chain_unlock(chain); + hammer2_chain_drop(chain); + } + kprintf("H2EAGAIN\n"); + + return EAGAIN; + } + } } parent = chain; @@ -301,6 +320,7 @@ hammer2_xop_unlink(hammer2_xop_t *arg, int clindex) int cache_index = -1; /* XXX */ int error; +again: /* * Requires exclusive lock */ @@ -368,10 +388,21 @@ hammer2_xop_unlink(hammer2_xop_t *arg, int clindex) * Check directory typing and delete the entry. Note that * nlinks adjustments are made on the real inode by the * frontend, not here. + * + * Unfortunately, checkdirempty() may have to unlock (parent). + * If it no longer matches chain->parent after re-locking, + * EAGAIN is returned. */ if (type == HAMMER2_OBJTYPE_DIRECTORY && - checkdirempty(parent, chain, clindex) != 0) { - error = ENOTEMPTY; + (error = checkdirempty(parent, chain, clindex)) != 0) { + /* error may be EAGAIN or ENOTEMPTY */ + if (error == EAGAIN) { + hammer2_chain_unlock(chain); + hammer2_chain_drop(chain); + hammer2_chain_unlock(parent); + hammer2_chain_drop(parent); + goto again; + } } else if (type == HAMMER2_OBJTYPE_DIRECTORY && xop->isdir == 0) { error = ENOTDIR; -- 2.11.4.GIT