Re: [PATCH 07/11] [dm-thin] Commit every second to prevent too much of a position building up.

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Feb 07 2012 at 11:53am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Feb 02 2012 at 11:39am -0500,
> Joe Thornber <ejt@redhat.com> wrote:
> 
> > ---
> >  drivers/md/dm-thin.c |   28 ++++++++++++++++++++++++++--
> >  1 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> > index 6ef03a2..19de11a 100644
> > --- a/drivers/md/dm-thin.c
> > +++ b/drivers/md/dm-thin.c
> > @@ -23,6 +23,7 @@
> >  #define DEFERRED_SET_SIZE 64
> >  #define MAPPING_POOL_SIZE 1024
> >  #define PRISON_CELLS 1024
> > +#define COMMIT_PERIOD HZ
> >  
> >  /*
> >   * The block size of the device holding pool data must be
> > @@ -498,8 +499,10 @@ struct pool {
> >  
> >  	struct workqueue_struct *wq;
> >  	struct work_struct worker;
> > +	struct delayed_work waker;
> >  
> >  	unsigned ref_count;
> > +	unsigned long last_commit_jiffies;
> >  
> >  	spinlock_t lock;
> >  	struct bio_list deferred_bios;
> > @@ -1256,6 +1259,12 @@ static void process_bio(struct thin_c *tc, struct bio *bio)
> >  	}
> >  }
> >  
> > +static int need_commit_due_to_time(struct pool *pool)
> > +{
> > +	return jiffies < pool->last_commit_jiffies ||
> > +	       jiffies > pool->last_commit_jiffies + COMMIT_PERIOD;
> > +}
> > +
> >  static void process_deferred_bios(struct pool *pool)
> >  {
> >  	unsigned long flags;
> > @@ -1297,7 +1306,7 @@ static void process_deferred_bios(struct pool *pool)
> >  	bio_list_init(&pool->deferred_flush_bios);
> >  	spin_unlock_irqrestore(&pool->lock, flags);
> >  
> > -	if (bio_list_empty(&bios))
> > +	if (bio_list_empty(&bios) && !need_commit_due_to_time(pool))
> >  		return;
> 
> Shouldn't this be:
> 
> 	if (bio_list_empty(&bios) || !need_commit_due_to_time(pool))
> 		return;
> 
> ?

Hmm, looking closer at the code it is clear that if we'd use || then the
pool->deferred_flush_bios would get dropped in the floor by the
!need_commit_due_to_time(pool) early return.

So ignore that ;)

> Also, should we make the commit interval tunable (akin to ext[34]'s
> 'commit' mount option)?  Or did you defer doing so until it proves
> worthwhile?

But this question still stands.