* [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit @ 2014-09-02 17:18 Jan Kara 2014-09-02 21:59 ` Theodore Ts'o 0 siblings, 1 reply; 9+ messages in thread From: Jan Kara @ 2014-09-02 17:18 UTC (permalink / raw) To: Ted Tso; +Cc: linux-ext4, Jan Kara When we discover written out buffer in transaction checkpoint list we don't have to recheck validity of a transaction. Either this is the last buffer in a transaction - and then we are done - or this isn't and then we can just take another buffer from the checkpoint list without dropping j_list_lock. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/jbd2/checkpoint.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index 993a187527f3..3722e2e53638 100644 --- a/fs/jbd2/checkpoint.c +++ b/fs/jbd2/checkpoint.c @@ -343,12 +343,15 @@ restart: if (!buffer_dirty(bh)) { if (unlikely(buffer_write_io_error(bh)) && !result) result = -EIO; - get_bh(bh); BUFFER_TRACE(bh, "remove from checkpoint"); - __jbd2_journal_remove_checkpoint(jh); - spin_unlock(&journal->j_list_lock); - __brelse(bh); - goto retry; + if (__jbd2_journal_remove_checkpoint(jh)) { + /* + * This was the last buffer attached to the + * transaction. We are done. + */ + goto out; + } + continue; } /* * Important: we are about to write the buffer, and -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit 2014-09-02 17:18 [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit Jan Kara @ 2014-09-02 21:59 ` Theodore Ts'o 2014-09-02 22:46 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Theodore Ts'o 2014-09-03 16:03 ` [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit Jan Kara 0 siblings, 2 replies; 9+ messages in thread From: Theodore Ts'o @ 2014-09-02 21:59 UTC (permalink / raw) To: Jan Kara; +Cc: linux-ext4 On Tue, Sep 02, 2014 at 07:18:30PM +0200, Jan Kara wrote: > When we discover written out buffer in transaction checkpoint list we > don't have to recheck validity of a transaction. Either this is the last > buffer in a transaction - and then we are done - or this isn't and then > we can just take another buffer from the checkpoint list without > dropping j_list_lock. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/jbd2/checkpoint.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 993a187527f3..3722e2e53638 100644 > --- a/fs/jbd2/checkpoint.c > +++ b/fs/jbd2/checkpoint.c > @@ -343,12 +343,15 @@ restart: > if (!buffer_dirty(bh)) { > if (unlikely(buffer_write_io_error(bh)) && !result) > result = -EIO; > - get_bh(bh); > BUFFER_TRACE(bh, "remove from checkpoint"); > - __jbd2_journal_remove_checkpoint(jh); > - spin_unlock(&journal->j_list_lock); > - __brelse(bh); Currently, all of the places which call __jbd2_jouranl_remove_checkpoint(jh) are doing so with an elevated b_count. For example, see __try_to_free_cp_buf(). After doing a lot of desk checking, I can't see any reason for holding the elevanted b_count, so I think it should be to remove it, but then we can simplify the other uses __try_to_free_cp_buf(). For example, in the loop that I folded from __wait_cp_io, we could drop the done variable and change: done = __jbd2_journal_remove_checkpoint(jh); __brelse(bh); to this: __brelse(bh); if (__jbd2_journal_remove_checkpoint(jh)) break; How much testing have you done of this optimization? I'm tempted to try nuking all of the elevated b_counts around the call to __jbd2_journal_remove_checkpoint(), and then doing a test to see if anything blows up. Cheers, - Ted ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() 2014-09-02 21:59 ` Theodore Ts'o @ 2014-09-02 22:46 ` Theodore Ts'o 2014-09-02 22:46 ` [PATCH 2/2] jbd2: optimize jbd2_log_do_checkpoint() a bit Theodore Ts'o ` (3 more replies) 2014-09-03 16:03 ` [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit Jan Kara 1 sibling, 4 replies; 9+ messages in thread From: Theodore Ts'o @ 2014-09-02 22:46 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Theodore Ts'o The __jbd2_journal_remove_checkpoint() doesn't require an elevated b_count; indeed, until the jh structure gets released by the call to jbd2_journal_put_journal_head(), the bh's b_count is elevated by virtue of the existence of the jh structure. Suggested-by: Jan Kara <jack@suse.cz> Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/jbd2/checkpoint.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index 22fcd50..cb6e17c 100644 --- a/fs/jbd2/checkpoint.c +++ b/fs/jbd2/checkpoint.c @@ -100,11 +100,8 @@ static int __try_to_free_cp_buf(struct journal_head *jh) * Get our reference so that bh cannot be freed before * we unlock it */ - get_bh(bh); JBUFFER_TRACE(jh, "remove from checkpoint list"); ret = __jbd2_journal_remove_checkpoint(jh) + 1; - BUFFER_TRACE(bh, "release"); - __brelse(bh); } return ret; } @@ -216,7 +213,7 @@ int jbd2_log_do_checkpoint(journal_t *journal) struct buffer_head *bh; transaction_t *transaction; tid_t this_tid; - int result, batch_count = 0, done = 0; + int result, batch_count = 0; jbd_debug(1, "Start checkpoint\n"); @@ -291,11 +288,9 @@ restart: if (!buffer_dirty(bh)) { if (unlikely(buffer_write_io_error(bh)) && !result) result = -EIO; - get_bh(bh); BUFFER_TRACE(bh, "remove from checkpoint"); __jbd2_journal_remove_checkpoint(jh); spin_unlock(&journal->j_list_lock); - __brelse(bh); goto retry; } /* @@ -338,7 +333,7 @@ restart2: transaction->t_tid != this_tid) goto out; - while (!done && transaction->t_checkpoint_io_list) { + while (transaction->t_checkpoint_io_list) { jh = transaction->t_checkpoint_io_list; bh = jh2bh(jh); get_bh(bh); @@ -359,8 +354,9 @@ restart2: * know that it has been written out and so we can * drop it from the list */ - done = __jbd2_journal_remove_checkpoint(jh); __brelse(bh); + if (__jbd2_journal_remove_checkpoint(jh)) + break; } out: spin_unlock(&journal->j_list_lock); -- 2.1.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] jbd2: optimize jbd2_log_do_checkpoint() a bit 2014-09-02 22:46 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Theodore Ts'o @ 2014-09-02 22:46 ` Theodore Ts'o 2014-09-03 7:54 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Yuanhan Liu ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Theodore Ts'o @ 2014-09-02 22:46 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Jan Kara, Theodore Ts'o From: Jan Kara <jack@suse.cz> When we discover written out buffer in transaction checkpoint list we don't have to recheck validity of a transaction. Either this is the last buffer in a transaction - and then we are done - or this isn't and then we can just take another buffer from the checkpoint list without dropping j_list_lock. Signed-off-by: Jan Kara <jack@suse.cz> Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/jbd2/checkpoint.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index cb6e17c..7713f94 100644 --- a/fs/jbd2/checkpoint.c +++ b/fs/jbd2/checkpoint.c @@ -289,9 +289,10 @@ restart: if (unlikely(buffer_write_io_error(bh)) && !result) result = -EIO; BUFFER_TRACE(bh, "remove from checkpoint"); - __jbd2_journal_remove_checkpoint(jh); - spin_unlock(&journal->j_list_lock); - goto retry; + if (__jbd2_journal_remove_checkpoint(jh)) + /* The transaction was released; we're done */ + goto out; + continue; } /* * Important: we are about to write the buffer, and -- 2.1.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() 2014-09-02 22:46 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Theodore Ts'o 2014-09-02 22:46 ` [PATCH 2/2] jbd2: optimize jbd2_log_do_checkpoint() a bit Theodore Ts'o @ 2014-09-03 7:54 ` Yuanhan Liu 2014-09-03 16:08 ` Jan Kara [not found] ` <CAGZGoEfjAZuOAbp-AqA-kiL2aTwiRXDO_Di=LPDovRBDNSn5dw@mail.gmail.com> 3 siblings, 0 replies; 9+ messages in thread From: Yuanhan Liu @ 2014-09-03 7:54 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Ext4 Developers List <resend due to malformed email> On Wed, Sep 3, 2014 at 6:46 AM, Theodore Ts'o <tytso@mit.edu> wrote: > The __jbd2_journal_remove_checkpoint() doesn't require an elevated > b_count; indeed, until the jh structure gets released by the call to > jbd2_journal_put_journal_head(), the bh's b_count is elevated by > virtue of the existence of the jh structure. > > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > fs/jbd2/checkpoint.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 22fcd50..cb6e17c 100644 > --- a/fs/jbd2/checkpoint.c > +++ b/fs/jbd2/checkpoint.c > @@ -100,11 +100,8 @@ static int __try_to_free_cp_buf(struct journal_head *jh) > * Get our reference so that bh cannot be freed before > * we unlock it > */ I guess you need drop those comments as well. --yliu > - get_bh(bh); > JBUFFER_TRACE(jh, "remove from checkpoint list"); > ret = __jbd2_journal_remove_checkpoint(jh) + 1; > - BUFFER_TRACE(bh, "release"); > - __brelse(bh); > } > return ret; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() 2014-09-02 22:46 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Theodore Ts'o 2014-09-02 22:46 ` [PATCH 2/2] jbd2: optimize jbd2_log_do_checkpoint() a bit Theodore Ts'o 2014-09-03 7:54 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Yuanhan Liu @ 2014-09-03 16:08 ` Jan Kara 2014-09-03 18:38 ` [PATCH -v2] " Theodore Ts'o [not found] ` <CAGZGoEfjAZuOAbp-AqA-kiL2aTwiRXDO_Di=LPDovRBDNSn5dw@mail.gmail.com> 3 siblings, 1 reply; 9+ messages in thread From: Jan Kara @ 2014-09-03 16:08 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Ext4 Developers List On Tue 02-09-14 18:46:39, Ted Tso wrote: > The __jbd2_journal_remove_checkpoint() doesn't require an elevated > b_count; indeed, until the jh structure gets released by the call to > jbd2_journal_put_journal_head(), the bh's b_count is elevated by > virtue of the existence of the jh structure. > > Suggested-by: Jan Kara <jack@suse.cz> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> Looks good so you can add: Reviewed-by: Jan Kara <jack@suse.cz> Just we can do a bit more as I mentioned in my other email: > @@ -359,8 +354,9 @@ restart2: > * know that it has been written out and so we can > * drop it from the list > */ > - done = __jbd2_journal_remove_checkpoint(jh); > __brelse(bh); Here we don't need to grab bh reference unless we are going to call wait_on_buffer(). Which moves get_bh / __brelse out of fast path. > + if (__jbd2_journal_remove_checkpoint(jh)) > + break; > } > out: > spin_unlock(&journal->j_list_lock); Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH -v2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() 2014-09-03 16:08 ` Jan Kara @ 2014-09-03 18:38 ` Theodore Ts'o 0 siblings, 0 replies; 9+ messages in thread From: Theodore Ts'o @ 2014-09-03 18:38 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Theodore Ts'o The __jbd2_journal_remove_checkpoint() doesn't require an elevated b_count; indeed, until the jh structure gets released by the call to jbd2_journal_put_journal_head(), the bh's b_count is elevated by virtue of the existence of the jh structure. Suggested-by: Jan Kara <jack@suse.cz> Reviewed-by: Jan Kara <jack@suse.cz> Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/jbd2/checkpoint.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index 18c7a8d..90d6091 100644 --- a/fs/jbd2/checkpoint.c +++ b/fs/jbd2/checkpoint.c @@ -96,15 +96,8 @@ static int __try_to_free_cp_buf(struct journal_head *jh) if (jh->b_transaction == NULL && !buffer_locked(bh) && !buffer_dirty(bh) && !buffer_write_io_error(bh)) { - /* - * Get our reference so that bh cannot be freed before - * we unlock it - */ - get_bh(bh); JBUFFER_TRACE(jh, "remove from checkpoint list"); ret = __jbd2_journal_remove_checkpoint(jh) + 1; - BUFFER_TRACE(bh, "release"); - __brelse(bh); } return ret; } @@ -216,7 +209,7 @@ int jbd2_log_do_checkpoint(journal_t *journal) struct buffer_head *bh; transaction_t *transaction; tid_t this_tid; - int result, batch_count = 0, done = 0; + int result, batch_count = 0; jbd_debug(1, "Start checkpoint\n"); @@ -291,11 +284,9 @@ restart: if (!buffer_dirty(bh)) { if (unlikely(buffer_write_io_error(bh)) && !result) result = -EIO; - get_bh(bh); BUFFER_TRACE(bh, "remove from checkpoint"); __jbd2_journal_remove_checkpoint(jh); spin_unlock(&journal->j_list_lock); - __brelse(bh); goto retry; } /* @@ -338,12 +329,12 @@ restart2: transaction->t_tid != this_tid) goto out; - while (!done && transaction->t_checkpoint_io_list) { + while (transaction->t_checkpoint_io_list) { jh = transaction->t_checkpoint_io_list; bh = jh2bh(jh); - get_bh(bh); if (buffer_locked(bh)) { spin_unlock(&journal->j_list_lock); + get_bh(bh); wait_on_buffer(bh); /* the journal_head may have gone by now */ BUFFER_TRACE(bh, "brelse"); @@ -359,8 +350,8 @@ restart2: * know that it has been written out and so we can * drop it from the list */ - done = __jbd2_journal_remove_checkpoint(jh); - __brelse(bh); + if (__jbd2_journal_remove_checkpoint(jh)) + break; } out: spin_unlock(&journal->j_list_lock); -- 2.1.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <CAGZGoEfjAZuOAbp-AqA-kiL2aTwiRXDO_Di=LPDovRBDNSn5dw@mail.gmail.com>]
* Re: [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() [not found] ` <CAGZGoEfjAZuOAbp-AqA-kiL2aTwiRXDO_Di=LPDovRBDNSn5dw@mail.gmail.com> @ 2014-09-03 17:30 ` Theodore Ts'o 0 siblings, 0 replies; 9+ messages in thread From: Theodore Ts'o @ 2014-09-03 17:30 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Ext4 Developers List On Wed, Sep 03, 2014 at 03:48:56PM +0800, Yuanhan Liu wrote: > > * Get our reference so that bh cannot be freed before > > * we unlock it > > */ > > I guess you need remove those comments as well. Good catch, thanks. - Ted ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit 2014-09-02 21:59 ` Theodore Ts'o 2014-09-02 22:46 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Theodore Ts'o @ 2014-09-03 16:03 ` Jan Kara 1 sibling, 0 replies; 9+ messages in thread From: Jan Kara @ 2014-09-03 16:03 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Jan Kara, linux-ext4 On Tue 02-09-14 17:59:30, Ted Tso wrote: > On Tue, Sep 02, 2014 at 07:18:30PM +0200, Jan Kara wrote: > > When we discover written out buffer in transaction checkpoint list we > > don't have to recheck validity of a transaction. Either this is the last > > buffer in a transaction - and then we are done - or this isn't and then > > we can just take another buffer from the checkpoint list without > > dropping j_list_lock. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/jbd2/checkpoint.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > > index 993a187527f3..3722e2e53638 100644 > > --- a/fs/jbd2/checkpoint.c > > +++ b/fs/jbd2/checkpoint.c > > @@ -343,12 +343,15 @@ restart: > > if (!buffer_dirty(bh)) { > > if (unlikely(buffer_write_io_error(bh)) && !result) > > result = -EIO; > > - get_bh(bh); > > BUFFER_TRACE(bh, "remove from checkpoint"); > > - __jbd2_journal_remove_checkpoint(jh); > > - spin_unlock(&journal->j_list_lock); > > - __brelse(bh); > > Currently, all of the places which call > __jbd2_jouranl_remove_checkpoint(jh) are doing so with an elevated > b_count. For example, see __try_to_free_cp_buf(). I did a bit of archeology and commit 932bb305ba2a01cd62809644d569f004e77a4355 removed the need to hold buffer reference when calling __jbd2_journal_remove_checkpoint(). So it should be safe to remove that reference handling also from __try_to_free_cp_buf(). > After doing a lot of desk checking, I can't see any reason for holding > the elevanted b_count, so I think it should be to remove it, but then > we can simplify the other uses __try_to_free_cp_buf(). For example, > in the loop that I folded from __wait_cp_io, we could drop the done > variable and change: > > done = __jbd2_journal_remove_checkpoint(jh); > __brelse(bh); > > to this: > > __brelse(bh); > if (__jbd2_journal_remove_checkpoint(jh)) > break; Well, we don't even need to grab bh reference unless we find the buffer is locked and are going to wait for it. And yes, we can get rid of that 'done' variable. > How much testing have you done of this optimization? I'm tempted to > try nuking all of the elevated b_counts around the call to > __jbd2_journal_remove_checkpoint(), and then doing a test to see if > anything blows up. Honestly, I didn't test much but I'm pretty confident we are safe to remove those bh refs ;) Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-09-03 18:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-02 17:18 [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit Jan Kara 2014-09-02 21:59 ` Theodore Ts'o 2014-09-02 22:46 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Theodore Ts'o 2014-09-02 22:46 ` [PATCH 2/2] jbd2: optimize jbd2_log_do_checkpoint() a bit Theodore Ts'o 2014-09-03 7:54 ` [PATCH 1/2] jbd2: don't call get_bh() before calling __jbd2_journal_remove_checkpoint() Yuanhan Liu 2014-09-03 16:08 ` Jan Kara 2014-09-03 18:38 ` [PATCH -v2] " Theodore Ts'o [not found] ` <CAGZGoEfjAZuOAbp-AqA-kiL2aTwiRXDO_Di=LPDovRBDNSn5dw@mail.gmail.com> 2014-09-03 17:30 ` [PATCH 1/2] " Theodore Ts'o 2014-09-03 16:03 ` [PATCH] jbd2: Optimize jbd2_log_do_checkpoint() a bit Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.