From 815e1c13cd185a8d3507c7d200b6f53a78b6a364 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Tue, 1 Sep 2015 00:28:18 -0700 Subject: [PATCH] hammer2 - stabilization - Fix bugs found by blogbench * Make sure that any inode without an associated vnode gets put on the sideq if it is dirtied, otherwise it might not ever get flushed. * Fix a SMP race in hammer2_xop_start_except(). For asynchronous completions, the xop can become invalid the instant we release our spinlock if another thread manages to process, retire, and free it. --- sys/vfs/hammer2/hammer2_inode.c | 28 ++++++++++++++++++++++++++- sys/vfs/hammer2/hammer2_strategy.c | 1 + sys/vfs/hammer2/hammer2_thread.c | 31 +++++++++--------------------- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/sys/vfs/hammer2/hammer2_inode.c b/sys/vfs/hammer2/hammer2_inode.c index 5fde737c6f..b0d86b5104 100644 --- a/sys/vfs/hammer2/hammer2_inode.c +++ b/sys/vfs/hammer2/hammer2_inode.c @@ -1282,6 +1282,10 @@ hammer2_inode_common_parent(hammer2_inode_t *fdip, hammer2_inode_t *tdip) * Mark an inode as being modified, meaning that the caller will modify * ip->meta. * + * If a vnode is present we set the vnode dirty and the nominal filesystem + * sync will also handle synchronizing the inode meta-data. If no vnode + * is present we must ensure that the inode is on pmp->sideq. + * * NOTE: No mtid (modify_tid) is passed into this routine. The caller is * only modifying the in-memory inode. A modify_tid is synchronized * later when the inode gets flushed. @@ -1289,9 +1293,31 @@ hammer2_inode_common_parent(hammer2_inode_t *fdip, hammer2_inode_t *tdip) void hammer2_inode_modify(hammer2_inode_t *ip) { + hammer2_inode_sideq_t *ipul; + hammer2_pfs_t *pmp; + atomic_set_int(&ip->flags, HAMMER2_INODE_MODIFIED); - if (ip->vp) + if (ip->vp) { vsetisdirty(ip->vp); + } else if ((pmp = ip->pmp) != NULL) { + if ((ip->flags & HAMMER2_INODE_ONSIDEQ) == 0) { + ipul = kmalloc(sizeof(*ipul), pmp->minode, + M_WAITOK | M_ZERO); + ipul->ip = ip; + hammer2_inode_ref(ip); + hammer2_spin_ex(&pmp->list_spin); + if ((ip->flags & HAMMER2_INODE_ONSIDEQ) == 0) { + atomic_set_int(&ip->flags, + HAMMER2_INODE_ONSIDEQ); + TAILQ_INSERT_TAIL(&pmp->sideq, ipul, entry); + hammer2_spin_unex(&pmp->list_spin); + } else { + hammer2_spin_unex(&pmp->list_spin); + hammer2_inode_drop(ip); + kfree(ipul, pmp->minode); + } + } + } } /* diff --git a/sys/vfs/hammer2/hammer2_strategy.c b/sys/vfs/hammer2/hammer2_strategy.c index b707d7aa8c..1759c96fd6 100644 --- a/sys/vfs/hammer2/hammer2_strategy.c +++ b/sys/vfs/hammer2/hammer2_strategy.c @@ -264,6 +264,7 @@ hammer2_strategy_read(struct vop_strategy_args *ap) xop->lbase = lbase; hammer2_mtx_init(&xop->lock, "h2bio"); hammer2_xop_start(&xop->head, hammer2_strategy_xop_read); + /* asynchronous completion */ return(0); } diff --git a/sys/vfs/hammer2/hammer2_thread.c b/sys/vfs/hammer2/hammer2_thread.c index a5c4862bca..6667fe7ea2 100644 --- a/sys/vfs/hammer2/hammer2_thread.c +++ b/sys/vfs/hammer2/hammer2_thread.c @@ -289,6 +289,7 @@ hammer2_xop_start_except(hammer2_xop_head_t *xop, hammer2_xop_func_t func, int g; #endif int i; + int nchains; pmp = xop->ip1->pmp; if (pmp->has_xop_threads == 0) @@ -307,9 +308,14 @@ hammer2_xop_start_except(hammer2_xop_head_t *xop, hammer2_xop_func_t func, * finish early and unlock the related inodes, some targets may get * behind. The sequencer ensures that ops on the same inode execute * in the same order. + * + * The instant xop is queued another thread can pick it off. In the + * case of asynchronous ops, another thread might even finish and + * deallocate it. */ hammer2_spin_ex(&pmp->xop_spin); - for (i = 0; i < xop->ip1->cluster.nchains; ++i) { + nchains = xop->ip1->cluster.nchains; + for (i = 0; i < nchains; ++i) { if (i != notidx) { atomic_set_int(&xop->run_mask, 1U << i); atomic_set_int(&xop->chk_mask, 1U << i); @@ -317,34 +323,15 @@ hammer2_xop_start_except(hammer2_xop_head_t *xop, hammer2_xop_func_t func, } } hammer2_spin_unex(&pmp->xop_spin); + /* xop can become invalid at this point */ /* * Try to wakeup just one xop thread for each cluster node. */ - for (i = 0; i < xop->ip1->cluster.nchains; ++i) { + for (i = 0; i < nchains; ++i) { if (i != notidx) wakeup_one(&pmp->xopq[i]); } -#if 0 - /* - * Dispatch to concurrent threads. - */ - for (i = 0; i < xop->ip1->cluster.nchains; ++i) { - thr = &xgrp->thrs[i]; - if (thr->td && i != notidx) { - lockmgr(&thr->lk, LK_EXCLUSIVE); - if (thr->td && - (thr->flags & HAMMER2_THREAD_STOP) == 0) { - atomic_set_int(&xop->run_mask, 1U << i); - atomic_set_int(&xop->chk_mask, 1U << i); - TAILQ_INSERT_TAIL(&thr->xopq, xop, - collect[i].entry); - } - lockmgr(&thr->lk, LK_RELEASE); - wakeup(&thr->flags); - } - } -#endif } void -- 2.41.0