Re: [PATCH 12/19] bcache: update bucket_in_use periodically

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

--
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
> 

[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux