Re: [PATCH] jbd2: remove __jbd2_journal_clean_checkpoint_list to solve scalability issue

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

 



On Fri, Sep 12, 2014 at 11:40:54AM +0200, Jan Kara wrote:
> On Thu 11-09-14 23:45:39, Yuanhan Liu wrote:
> > On Thu, Sep 11, 2014 at 03:51:47PM +0200, Jan Kara wrote:
> > > On Thu 11-09-14 18:09:53, Yuanhan Liu wrote:
> > > > Firstly, if I read the code correctly(I have a feeling I missed something),
> > > > __jbd2_journal_clean_checkpoint_list is not necessary. jbd2_log_do_checkpoint
> > > > will remove them for the checkpointed transaction. In another word,
> > > > __jbd2_journal_clean_checkpoint_list can't drop an transaction if it's not
> > > > checkpointed.
> > >   Yes, that's correct. __jbd2_journal_clean_checkpoint_list() is there only
> > > to free up buffers that were already checkpointed.
> > 
> > Yes, and that might be something I missed: if a transaction is
> > checkpointed, those buffers attached to the transaction should also be
> > released, right? If that's ture, what __jbd2_journal_clean_checkpoint_list()
> > is for?
>   They aren't released unless memory reclaim wants to free the
> corresponding pagecache page. So they need to be detached from the
> transaction somehow once they are written so that the transaction can be
> eventually freed which frees the space in the journal. Currently we do this
> detaching either in __jbd2_journal_clean_checkpoint_list() or in
> jbd2_log_do_checkpoint(). However the latter function gets called only when
> we really run out of space in the journal and thus everything in the
> filesystem waits for some space to be reclaimed. So relying on that function
> isn't good for performance either...
> 
> One possibility is to remove buffer from transaction on IO completion.

Yes, that's the same thing I thought of. b_end_io hook is the first
thing I thought of, and I tried it. Badly, it was never being invoked.

I then realised that it should be written by page cache(writeback or
page reclaim as you mentioned), and that's the stuff I missed before.
So, my patch was wrong and sorry for that.

> However that's likely going to bounce j_list_lock between CPUs badly. So

I checked __jbd2_journal_remove_checkpoint(jh) again, which is the
entrance to free a journal buffer. It has two main parts:

	__buffer_unlink(jh)

	drop_transaction() if all jh are written

The two are all protected by j_list_lock. IMO, we can split it, say
let __buffer_unlink(jh) be protected by a per-transaction spin lock,
and let drop_transaction() be protected by by j_list_lock as it was.

As drop_transaction() doesn't happen frequently as __buffer_unlink(jh),
it should alleviate the lock contention a bit.

Thoughts?


	--yliu

> I'd rather somehow batch the updates of the checkpointing lists...
> 
> 								Honza
> 
> > > > Secondly, I noticed severe scalability limit caused by this function with fsmark:
> > > >    $ fs_mark  -d  /fs/ram0/1  -D  2  -N  2560  -n  1000000  -L  1  -S  1  -s  4096
> > > > 
> > > > It mainly writes tons of small files, each with 4K, and calls fsync after
> > > > each write. It's one thread only, and it's tested on a 32G memory disk(brd).
> > > > 
> > > > "perf top" shows that journal_clean_one_cp_list() is very hot, like 40%.
> > > > However, that function is quite cheap, actually. But __jbd2_journal_clean_checkpoint_list()
> > > > will repeatedly call it for each transaction on the j_checkpoint_transactions.
> > > > The list becomes longer and longer in linear with time. It soon grows to
> > > > a list with 3,000 and more transactions. What's worse, we will iterate all
> > > > those transactions on this list every time before committing a transaction,
> > > > aka, fsync a file in our case.
> > >   OK, this is kind of a pathological workload (generating lots of tiny
> > > transactions) but it's not insane. I'd also note that we should eventually
> > > reach a steady state once the journal fills up and we will be forced to
> > > checkpoint transactions from the journal to make space for new ones.
> > > However at that point we will have few thousands of transactions in the
> > > journal and I agree it takes a while to scan them in
> > > __jbd2_journal_clean_checkpoint_list().
> > > 
> > > I'm not yet convinced that just dropping
> > > __jbd2_journal_clean_checkpoint_list() is the right thing to do.
> > 
> > Yes, I feel the same. BTW, I should add a RFC tag before the subject.
> > This patch is mainly for informing you guys there might be a scalability
> > issue with current JBD2 code.
> > 
> > 	--yliu
> > 
> > > Some
> > > periodic cleaning of checkpointed entries would seem reasonable so that we
> > > don't save all that work for the moment we run out of space in the journal.
> > > I'll think how to do that in a more efficient way...
> > > 
> > > 								Honza
> > > 
> > > > So, that's roughly the reason why journal_clean_one_cp_list() is hot. However,
> > > > I'm wondering why the j_checkpoint_transactions list keeps growing. Is there
> > > > a bug? I did more investigates, and I guess I found the root cause.
> > > > 
> > > > In this case, we invoke fsync after each write, so, it basically means one
> > > > transaction for one file. One transaction normally contains few blocks of meta
> > > > data, like bitmap, inode and so on. All files in same group shares one bitmap
> > > > block. But one inode table block could contains 16 files. Hence, it's possible
> > > > that 2 different file points to same meta blocks.
> > > > 
> > > > For example, if file A and B uses same meta blocks, and if we write A and B in
> > > > following style:
> > > >         write(A);
> > > >         fsync(A);
> > > > 
> > > >         write(B);
> > > >         fsync(B);
> > > > 
> > > > then, when we are about to commit transation for B, and assume transaction of A
> > > > is not checkpointted yet, we can safely drop transaction A and replace it with
> > > > transaction B. Hence, the j_checkpoint_transactions grows by 1 only.
> > > > 
> > > > And then assume A is the last inode in one inode block, hence, B will use another
> > > > inode table block. Thus transaction A and B is different. Hence, both A and B are
> > > > inserted to the j_checkpoint_transactions; the list grows by 2.
> > > > 
> > > > Here I also got the proves; I added a trace point in jbd2_log_do_checkpoint(),
> > > > and here are some of them:
> > > > 
> > > >          fs_mark-2646  [000] ....     5.830990: jbd2_start_checkpoint: tid=20
> > > >          fs_mark-2646  [000] ....     5.832391: jbd2_start_checkpoint: tid=36
> > > >          fs_mark-2646  [000] ....     5.833794: jbd2_start_checkpoint: tid=52
> > > >          fs_mark-2646  [000] ....     5.835153: jbd2_start_checkpoint: tid=68
> > > >          fs_mark-2646  [000] ....     5.836517: jbd2_start_checkpoint: tid=84
> > > >          fs_mark-2646  [000] ....     5.837982: jbd2_start_checkpoint: tid=100
> > > >          fs_mark-2646  [000] ....     5.839464: jbd2_start_checkpoint: tid=116
> > > >          fs_mark-2646  [000] ....     5.840820: jbd2_start_checkpoint: tid=132
> > > > 
> > > > As you can see, the tid jumps by 16 each time, and other transactions are just
> > > > replaced by the way I described above.
> > > > 
> > > > Step by step like above, that list grows. And that's why journal_clean_one_cp_list() is hot.
> > > > 
> > > > It removes the scalability issue when I removed this function, and the fsmark
> > > > result(Files/sec) jumps from 9000 to 16000, which is an increase about 80%.
> > > > 
> > > > Thoughts?
> > > > 
> > > > CC: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  fs/jbd2/checkpoint.c | 115 ---------------------------------------------------
> > > >  fs/jbd2/commit.c     |   9 ----
> > > >  include/linux/jbd2.h |   1 -
> > > >  3 files changed, 125 deletions(-)
> > > > 
> > > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> > > > index 9ffb19c..31fce78 100644
> > > > --- a/fs/jbd2/checkpoint.c
> > > > +++ b/fs/jbd2/checkpoint.c
> > > > @@ -83,26 +83,6 @@ static inline void __buffer_relink_io(struct journal_head *jh)
> > > >  }
> > > >  
> > > >  /*
> > > > - * Try to release a checkpointed buffer from its transaction.
> > > > - * Returns 1 if we released it and 2 if we also released the
> > > > - * whole transaction.
> > > > - *
> > > > - * Requires j_list_lock
> > > > - */
> > > > -static int __try_to_free_cp_buf(struct journal_head *jh)
> > > > -{
> > > > -	int ret = 0;
> > > > -	struct buffer_head *bh = jh2bh(jh);
> > > > -
> > > > -	if (jh->b_transaction == NULL && !buffer_locked(bh) &&
> > > > -	    !buffer_dirty(bh) && !buffer_write_io_error(bh)) {
> > > > -		JBUFFER_TRACE(jh, "remove from checkpoint list");
> > > > -		ret = __jbd2_journal_remove_checkpoint(jh) + 1;
> > > > -	}
> > > > -	return ret;
> > > > -}
> > > > -
> > > > -/*
> > > >   * __jbd2_log_wait_for_space: wait until there is space in the journal.
> > > >   *
> > > >   * Called under j-state_lock *only*.  It will be unlocked if we have to wait
> > > > @@ -412,101 +392,6 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
> > > >  
> > > >  /* Checkpoint list management */
> > > >  
> > > > -/*
> > > > - * journal_clean_one_cp_list
> > > > - *
> > > > - * Find all the written-back checkpoint buffers in the given list and
> > > > - * release them.
> > > > - *
> > > > - * Called with the journal locked.
> > > > - * Called with j_list_lock held.
> > > > - * Returns number of buffers reaped (for debug)
> > > > - */
> > > > -
> > > > -static int journal_clean_one_cp_list(struct journal_head *jh, int *released)
> > > > -{
> > > > -	struct journal_head *last_jh;
> > > > -	struct journal_head *next_jh = jh;
> > > > -	int ret, freed = 0;
> > > > -
> > > > -	*released = 0;
> > > > -	if (!jh)
> > > > -		return 0;
> > > > -
> > > > -	last_jh = jh->b_cpprev;
> > > > -	do {
> > > > -		jh = next_jh;
> > > > -		next_jh = jh->b_cpnext;
> > > > -		ret = __try_to_free_cp_buf(jh);
> > > > -		if (ret) {
> > > > -			freed++;
> > > > -			if (ret == 2) {
> > > > -				*released = 1;
> > > > -				return freed;
> > > > -			}
> > > > -		}
> > > > -		/*
> > > > -		 * This function only frees up some memory
> > > > -		 * if possible so we dont have an obligation
> > > > -		 * to finish processing. Bail out if preemption
> > > > -		 * requested:
> > > > -		 */
> > > > -		if (need_resched())
> > > > -			return freed;
> > > > -	} while (jh != last_jh);
> > > > -
> > > > -	return freed;
> > > > -}
> > > > -
> > > > -/*
> > > > - * journal_clean_checkpoint_list
> > > > - *
> > > > - * Find all the written-back checkpoint buffers in the journal and release them.
> > > > - *
> > > > - * Called with the journal locked.
> > > > - * Called with j_list_lock held.
> > > > - * Returns number of buffers reaped (for debug)
> > > > - */
> > > > -
> > > > -int __jbd2_journal_clean_checkpoint_list(journal_t *journal)
> > > > -{
> > > > -	transaction_t *transaction, *last_transaction, *next_transaction;
> > > > -	int ret = 0;
> > > > -	int released;
> > > > -
> > > > -	transaction = journal->j_checkpoint_transactions;
> > > > -	if (!transaction)
> > > > -		goto out;
> > > > -
> > > > -	last_transaction = transaction->t_cpprev;
> > > > -	next_transaction = transaction;
> > > > -	do {
> > > > -		transaction = next_transaction;
> > > > -		next_transaction = transaction->t_cpnext;
> > > > -		ret += journal_clean_one_cp_list(transaction->
> > > > -				t_checkpoint_list, &released);
> > > > -		/*
> > > > -		 * This function only frees up some memory if possible so we
> > > > -		 * dont have an obligation to finish processing. Bail out if
> > > > -		 * preemption requested:
> > > > -		 */
> > > > -		if (need_resched())
> > > > -			goto out;
> > > > -		if (released)
> > > > -			continue;
> > > > -		/*
> > > > -		 * It is essential that we are as careful as in the case of
> > > > -		 * t_checkpoint_list with removing the buffer from the list as
> > > > -		 * we can possibly see not yet submitted buffers on io_list
> > > > -		 */
> > > > -		ret += journal_clean_one_cp_list(transaction->
> > > > -				t_checkpoint_io_list, &released);
> > > > -		if (need_resched())
> > > > -			goto out;
> > > > -	} while (transaction != last_transaction);
> > > > -out:
> > > > -	return ret;
> > > > -}
> > > >  
> > > >  /*
> > > >   * journal_remove_checkpoint: called after a buffer has been committed
> > > > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > > > index b73e021..1ebdd70 100644
> > > > --- a/fs/jbd2/commit.c
> > > > +++ b/fs/jbd2/commit.c
> > > > @@ -504,15 +504,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> > > >  		jbd2_journal_refile_buffer(journal, jh);
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * Now try to drop any written-back buffers from the journal's
> > > > -	 * checkpoint lists.  We do this *before* commit because it potentially
> > > > -	 * frees some memory
> > > > -	 */
> > > > -	spin_lock(&journal->j_list_lock);
> > > > -	__jbd2_journal_clean_checkpoint_list(journal);
> > > > -	spin_unlock(&journal->j_list_lock);
> > > > -
> > > >  	jbd_debug(3, "JBD2: commit phase 1\n");
> > > >  
> > > >  	/*
> > > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > > > index 0dae71e..c41ab38 100644
> > > > --- a/include/linux/jbd2.h
> > > > +++ b/include/linux/jbd2.h
> > > > @@ -1042,7 +1042,6 @@ void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block);
> > > >  extern void jbd2_journal_commit_transaction(journal_t *);
> > > >  
> > > >  /* Checkpoint list management */
> > > > -int __jbd2_journal_clean_checkpoint_list(journal_t *journal);
> > > >  int __jbd2_journal_remove_checkpoint(struct journal_head *);
> > > >  void __jbd2_journal_insert_checkpoint(struct journal_head *, transaction_t *);
> > > >  
> > > > -- 
> > > > 1.9.0
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > -- 
> > > Jan Kara <jack@xxxxxxx>
> > > SUSE Labs, CR
> -- 
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux