From 11030793fae4226352b67b1c806beae51e88150a Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Mon, 21 May 2012 17:18:35 -0700 Subject: [PATCH] objectcacher: fix infinite loop in flusher_entry The addition of accounting for simultaneous writers in f3043fee3e22600cb4349072287842db129588eb could lead to an infinite loop. This could happen because starting a flush doesn't decrease dirty_waiters, only the number of dirty bytes (they change to tx). If dirty_waiters is more than target_dirty, the loop would continue forever while holding the object cacher lock, since the object cacher lock is required by the write callbacks. The extra while (!flusher_stop) loop is unnecessary, so remove it. This means the flusher thread always releases the lock after starting flushing, so progress can be made. Signed-off-by: Sage Weil Signed-off-by: Josh Durgin --- src/osdc/ObjectCacher.cc | 62 +++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/src/osdc/ObjectCacher.cc b/src/osdc/ObjectCacher.cc index c9825f9220507..ade79fb382f43 100644 --- a/src/osdc/ObjectCacher.cc +++ b/src/osdc/ObjectCacher.cc @@ -1181,40 +1181,38 @@ void ObjectCacher::flusher_entry() ldout(cct, 10) << "flusher start" << dendl; lock.Lock(); while (!flusher_stop) { - while (!flusher_stop) { - loff_t all = get_stat_tx() + get_stat_rx() + get_stat_clean() + get_stat_dirty(); - ldout(cct, 11) << "flusher " - << all << " / " << max_size << ": " - << get_stat_tx() << " tx, " - << get_stat_rx() << " rx, " - << get_stat_clean() << " clean, " - << get_stat_dirty() << " dirty (" - << target_dirty << " target, " - << max_dirty << " max)" - << dendl; - if (get_stat_dirty() + get_stat_dirty_waiting() > target_dirty) { - // flush some dirty pages - ldout(cct, 10) << "flusher " - << get_stat_dirty() << " dirty + " << get_stat_dirty_waiting() - << " dirty_waiting > target " - << target_dirty - << ", flushing some dirty bhs" << dendl; - flush(get_stat_dirty() + get_stat_dirty_waiting() - target_dirty); - } - else { - // check tail of lru for old dirty items - utime_t cutoff = ceph_clock_now(cct); - cutoff -= max_dirty_age; - BufferHead *bh = 0; - while ((bh = (BufferHead*)lru_dirty.lru_get_next_expire()) != 0 && - bh->last_write < cutoff) { - ldout(cct, 10) << "flusher flushing aged dirty bh " << *bh << dendl; - bh_write(bh); - } - break; + loff_t all = get_stat_tx() + get_stat_rx() + get_stat_clean() + get_stat_dirty(); + ldout(cct, 11) << "flusher " + << all << " / " << max_size << ": " + << get_stat_tx() << " tx, " + << get_stat_rx() << " rx, " + << get_stat_clean() << " clean, " + << get_stat_dirty() << " dirty (" + << target_dirty << " target, " + << max_dirty << " max)" + << dendl; + loff_t actual = get_stat_dirty() + get_stat_dirty_waiting(); + if (actual > target_dirty) { + // flush some dirty pages + ldout(cct, 10) << "flusher " + << get_stat_dirty() << " dirty + " << get_stat_dirty_waiting() + << " dirty_waiting > target " + << target_dirty + << ", flushing some dirty bhs" << dendl; + flush(actual - target_dirty); + } else { + // check tail of lru for old dirty items + utime_t cutoff = ceph_clock_now(cct); + cutoff -= max_dirty_age; + BufferHead *bh = 0; + while ((bh = (BufferHead*)lru_dirty.lru_get_next_expire()) != 0 && + bh->last_write < cutoff) { + ldout(cct, 10) << "flusher flushing aged dirty bh " << *bh << dendl; + bh_write(bh); } } - if (flusher_stop) break; + if (flusher_stop) + break; flusher_cond.WaitInterval(cct, lock, utime_t(1,0)); } lock.Unlock(); -- 2.39.5