On Fri, 27 Oct 2017, Eric Wheeler wrote:
> On Thu, 13 Jul 2017, Coly Li wrote:
>
> > On 2017/7/13 下午12:13, Eric Wheeler wrote:
> > > On Tue, 11 Jul 2017, Coly Li wrote:
> > >
> > >> On 2017/7/11 下午1:39, tang.junhui@xxxxxxxxxx wrote:
> > >>> Compared to bucket depletion, resulting in hanging dead,
> > >>> It is worthy to consumes a little time to update the bucket_in_use.
> > >>> If you have any better solution, please show to us,
> > >>> We should solve it as soon as possible, not wait for it forever.
> > >>>
> > >>
> > >> I also test this patch on a cache device with 4x3.8TB size, all buckets
> > >> iteration takes around 40-50ms. If the iteration needs to hold
> > >> bucket_lock of cache set, it is very probably to introduce a huge I/O
> > >> latency in period of every 30 seconds.
> > >>
> > >> For database people, this is not good news.
> > >
> > >
> > > Hi Tang,
> > >
> > > I'm waiting to queue this patch pending your response to Coly.
> > >
> > > Please send a v2 when you're ready.
> >
> >
> > Eric,
> >
> > I guess Tang is working on the I/O hang issue during back ground garbage
> > collection running. From discussion from other email thread, it seems a
> > regular I/O request gets hung for 10+ second in some cases. Maybe that
> > issue is more urgent than this one.
> >
> > From my personal opinion, updating bucket_in_use is for acting garbage
> > collection. If number of bucket in use is not updated in time, garbage
> > collection won't start due to old bucket_in_use still beyond
> > CUTOFF_WRITEBACK_SYNC.
> >
> > We may maintain an atomic counter per-cache set for dirty buckets, and
> > update it at some locations when allocating or reclaiming bucket. This
> > counter is unnecessary to be very accurate, just accurate enough for
> > should_writeback() working correctly.
> >
> > I am also looking at it for a better solution as well.
>
> Hi Coli & Tang,
>
> Have either of you had a chance to come up with a solution to this?
Nevermind, I just saw the patch sent on 10/24. Thanks for your work on
that! I'll see if we can try it out.
--
Eric Wheeler
>
> --
> Eric Wheeler
>
> >
> > Coly
> >
> >
> > >>
> > >> Coly
> > >>
> > >>
> > >>>
> > >>>
> > >>>
> > >>> 发件人: Coly Li <i@xxxxxxx>
> > >>> 收件人: linux-block@xxxxxxxxxxxxxxx, Tang Junhui
> > >>> <tang.junhui@xxxxxxxxxx>,
> > >>> 抄送: bcache@xxxxxxxxxxxxxxxxxx, linux-bcache@xxxxxxxxxxxxxxx,
> > >>> hch@xxxxxxxxxxxxx, axboe@xxxxxxxxx
> > >>> 日期: 2017/07/11 13:06
> > >>> 主题: Re: [PATCH 12/19] bcache: update bucket_in_use periodically
> > >>> 发件人: linux-bcache-owner@xxxxxxxxxxxxxxx
> > >>> ------------------------------------------------------------------------
> > >>>
> > >>>
> > >>>
> > >>> On 2017/7/1 上午4:43, bcache@xxxxxxxxxxxxxxxxxx wrote:
> > >>>> From: Tang Junhui <tang.junhui@xxxxxxxxxx>
> > >>>>
> > >>>> bucket_in_use is updated in gc thread which triggered by invalidating or
> > >>>> writing sectors_to_gc dirty data, It's been too long, Therefore, when we
> > >>>> use it to compare with the threshold, it is often not timely, which leads
> > >>>> to inaccurate judgment and often results in bucket depletion.
> > >>>>
> > >>>> Signed-off-by: Tang Junhui <tang.junhui@xxxxxxxxxx>
> > >>>> ---
> > >>>> drivers/md/bcache/btree.c | 29 +++++++++++++++++++++++++++--
> > >>>> 1 file changed, 27 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > >>>> index 866dcf7..77aa20b 100644
> > >>>> --- a/drivers/md/bcache/btree.c
> > >>>> +++ b/drivers/md/bcache/btree.c
> > >>>> @@ -90,6 +90,8 @@
> > >>>> #define MAX_NEED_GC 64
> > >>>> #define MAX_SAVE_PRIO 72
> > >>>>
> > >>>> +#define GC_THREAD_TIMEOUT_MS (30 * 1000)
> > >>>> +
> > >>>> #define PTR_DIRTY_BIT (((uint64_t) 1
> > >>> << 36))
> > >>>>
> > >>>> #define PTR_HASH(c, k)
> > >>> \
> > >>>> @@ -1760,6 +1762,23 @@ static void bch_btree_gc(struct cache_set *c)
> > >>>> bch_moving_gc(c);
> > >>>> }
> > >>>>
> > >>>> +void bch_update_bucket_in_use(struct cache_set *c)
> > >>>> +{
> > >>>> + struct cache *ca;
> > >>>> + struct bucket *b;
> > >>>> + unsigned i;
> > >>>> + size_t available = 0;
> > >>>> +
> > >>>> + for_each_cache(ca, c, i) {
> > >>>> + for_each_bucket(b, ca) {
> > >>>> + if (!GC_MARK(b) ||
> > >>> GC_MARK(b) == GC_MARK_RECLAIMABLE)
> > >>>> +
> > >>> available++;
> > >>>> + }
> > >>>> + }
> > >>>> +
> > >>>
> > >>> bucket_lock of cache set should be held before accessing buckets.
> > >>>
> > >>>
> > >>>> + c->gc_stats.in_use = (c->nbuckets - available) * 100
> > >>> / c->nbuckets;
> > >>>> +}
> > >>>> +
> > >>>> static bool gc_should_run(struct cache_set *c)
> > >>>> {
> > >>>> struct cache *ca;
> > >>>> @@ -1778,10 +1797,16 @@ static bool gc_should_run(struct cache_set *c)
> > >>>> static int bch_gc_thread(void *arg)
> > >>>> {
> > >>>> struct cache_set *c = arg;
> > >>>> + long ret;
> > >>>> + unsigned long timeout =
> > >>> msecs_to_jiffies(GC_THREAD_TIMEOUT_MS);
> > >>>>
> > >>>> while (1) {
> > >>>> - wait_event_interruptible(c->gc_wait,
> > >>>> -
> > >>> kthread_should_stop() || gc_should_run(c));
> > >>>> + ret =
> > >>> wait_event_interruptible_timeout(c->gc_wait,
> > >>>> +
> > >>> kthread_should_stop() || gc_should_run(c), timeout);
> > >>>> + if (!ret) {
> > >>>> +
> > >>> bch_update_bucket_in_use(c);
> > >>>> + continue;
> > >>>
> > >>> A continue here will ignore status returned from kthread_should_stop(),
> > >>> which might not be expected behavior.
> > >>>
> > >>>
> > >>>> + }
> > >>>>
> > >>>> if (kthread_should_stop())
> > >>>> break;
> > >>>>
> > >>>
> > >>> Iterating all buckets from the cache set requires bucket_lock to be
> > >>> held. Waiting for bucket_lock may take quite a long time for either
> > >>> bucket allocating code or bch_gc_thread(). What I concern is, this patch
> > >>> may introduce bucket allocation delay in period of GC_THREAD_TIMEOUT_MS.
> > >>>
> > >>> We need to find out a way to avoid such a performance regression.
> > >>>
> > >>> --
> > >>> Coly Li
> > >>> --
> > >>> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> > >>> the body of a message to majordomo@xxxxxxxxxxxxxxx
> > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >>>
> > >>>
> > >>
> > >>
> > >> --
> > >> Coly Li
> >
> >
> > --
> > Coly Li
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
