diff mbox series

[05/10] jbd2: fix fast commit journalling APIs

Message ID 20201031200518.4178786-6-harshadshirwadkar@gmail.com
State Superseded
Headers show
Series Ext4 fast commit fixes | expand

Commit Message

harshad shirwadkar Oct. 31, 2020, 8:05 p.m. UTC 
This patch adds a few misc fixes for jbd2 fast commit functions.

Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
---
 fs/ext4/fast_commit.c |  2 +-
 fs/jbd2/commit.c      |  9 +++++++++
 fs/jbd2/journal.c     | 28 +++++++++++++---------------
 include/linux/jbd2.h  | 10 +++++++---
 4 files changed, 30 insertions(+), 19 deletions(-)

Comments

Jan Kara Nov. 3, 2020, 4:38 p.m. UTC | #1 
On Sat 31-10-20 13:05:13, Harshad Shirwadkar wrote:
> This patch adds a few misc fixes for jbd2 fast commit functions.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

Please no "misc fixes" patches. If you have trouble writing good
explanatory changelog, it's usually a sign you're trying to cram too much
into a single commit :). In this case I'd split it into 3 changes:

1) TODO update.
2) Removal of j_state_lock protection (with comment updates)
3) Fix of journal->j_running_transaction->t_tid handling.

> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 9b4e87a0068b..df1285da7276 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -946,7 +946,9 @@ struct journal_s
>  	 * @j_fc_off:
>  	 *
>  	 * Number of fast commit blocks currently allocated.
> -	 * [j_state_lock].
> +	 * [j_state_lock]. During the commit path, this variable is not

Please remove the [j_state_lock] annotation when the entry isn't really
protected by j_state_lock... Also I'd maybe rephrase the comment like
"Accessed only during fastcommit, currenly only a single process can
perform fastcommit at a time."

> +	 * protected by j_state_lock since only one process performs commit
> +	 * at a time.
>  	 */
>  	unsigned long		j_fc_off;
>  
> @@ -1110,7 +1112,9 @@ struct journal_s
>  
>  	/**
>  	 * @j_fc_wbuf: Array of fast commit bhs for
> -	 * jbd2_journal_commit_transaction.
> +	 * jbd2_journal_commit_transaction. During the commit path, this
> +	 * variable is not protected by j_state_lock since only one process
> +	 * performs commit at a time.
>  	 */
>  	struct buffer_head	**j_fc_wbuf;

Here the bh's aren't really used in jbd2_journal_commit_transaction() are
they? Please fix that when updating the comment. Also I'd find a
reformulation like I suggested for the comment above more comprehensible.

								Honza
harshad shirwadkar Nov. 4, 2020, 9:25 p.m. UTC | #2 
On Tue, Nov 3, 2020 at 8:38 AM Jan Kara <jack@suse.cz> wrote:
>
> On Sat 31-10-20 13:05:13, Harshad Shirwadkar wrote:
> > This patch adds a few misc fixes for jbd2 fast commit functions.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
>
> Please no "misc fixes" patches. If you have trouble writing good
> explanatory changelog, it's usually a sign you're trying to cram too much
> into a single commit :). In this case I'd split it into 3 changes:
>
> 1) TODO update.
> 2) Removal of j_state_lock protection (with comment updates)
> 3) Fix of journal->j_running_transaction->t_tid handling.
Okay thanks for pointing this out. I'll break this commit into logical
patches for the next version.
>
> > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > index 9b4e87a0068b..df1285da7276 100644
> > --- a/include/linux/jbd2.h
> > +++ b/include/linux/jbd2.h
> > @@ -946,7 +946,9 @@ struct journal_s
> >        * @j_fc_off:
> >        *
> >        * Number of fast commit blocks currently allocated.
> > -      * [j_state_lock].
> > +      * [j_state_lock]. During the commit path, this variable is not
>
> Please remove the [j_state_lock] annotation when the entry isn't really
> protected by j_state_lock... Also I'd maybe rephrase the comment like
> "Accessed only during fastcommit, currenly only a single process can
> perform fastcommit at a time."
Ack
>
> > +      * protected by j_state_lock since only one process performs commit
> > +      * at a time.
> >        */
> >       unsigned long           j_fc_off;
> >
> > @@ -1110,7 +1112,9 @@ struct journal_s
> >
> >       /**
> >        * @j_fc_wbuf: Array of fast commit bhs for
> > -      * jbd2_journal_commit_transaction.
> > +      * jbd2_journal_commit_transaction. During the commit path, this
> > +      * variable is not protected by j_state_lock since only one process
> > +      * performs commit at a time.
> >        */
> >       struct buffer_head      **j_fc_wbuf;
>
> Here the bh's aren't really used in jbd2_journal_commit_transaction() are
> they? Please fix that when updating the comment. Also I'd find a
> reformulation like I suggested for the comment above more comprehensible.
Ack,

Thanks,
Harshad
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
diff mbox series

Patch

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 1b62d82b9622..b1ca55c7d32a 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -1134,7 +1134,7 @@  int ext4_fc_commit(journal_t *journal, tid_t commit_tid)
 		"Fast commit ended with blks = %d, reason = %d, subtid - %d",
 		nblks, reason, subtid);
 	if (reason == EXT4_FC_REASON_FC_FAILED)
-		return jbd2_fc_end_commit_fallback(journal, commit_tid);
+		return jbd2_fc_end_commit_fallback(journal);
 	if (reason == EXT4_FC_REASON_FC_START_FAILED ||
 		reason == EXT4_FC_REASON_INELIGIBLE)
 		return jbd2_complete_transaction(journal, commit_tid);
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 353534403769..2444414ad89e 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -450,6 +450,15 @@  void jbd2_journal_commit_transaction(journal_t *journal)
 		schedule();
 		write_lock(&journal->j_state_lock);
 		finish_wait(&journal->j_fc_wait, &wait);
+		/*
+		 * TODO: by blocking fast commits here, we are increasing
+		 * fsync() latency slightly. Strictly speaking, we don't need
+		 * to block fast commits until the transaction enters T_FLUSH
+		 * state. So an optimization is possible where we block new fast
+		 * commits here and wait for existing ones to complete
+		 * just before we enter T_FLUSH. That way, the existing fast
+		 * commits and this full commit can proceed parallely.
+		 */
 	}
 	write_unlock(&journal->j_state_lock);
 
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index ea15f55aff5c..368727ef3912 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -734,10 +734,12 @@  int jbd2_fc_begin_commit(journal_t *journal, tid_t tid)
 	if (!journal->j_stats.ts_tid)
 		return -EINVAL;
 
-	if (tid <= journal->j_commit_sequence)
+	write_lock(&journal->j_state_lock);
+	if (tid <= journal->j_commit_sequence) {
+		write_unlock(&journal->j_state_lock);
 		return -EALREADY;
+	}
 
-	write_lock(&journal->j_state_lock);
 	if (journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
 	    (journal->j_flags & JBD2_FAST_COMMIT_ONGOING)) {
 		DEFINE_WAIT(wait);
@@ -777,13 +779,19 @@  static int __jbd2_fc_end_commit(journal_t *journal, tid_t tid, bool fallback)
 
 int jbd2_fc_end_commit(journal_t *journal)
 {
-	return __jbd2_fc_end_commit(journal, 0, 0);
+	return __jbd2_fc_end_commit(journal, 0, false);
 }
 EXPORT_SYMBOL(jbd2_fc_end_commit);
 
-int jbd2_fc_end_commit_fallback(journal_t *journal, tid_t tid)
+int jbd2_fc_end_commit_fallback(journal_t *journal)
 {
-	return __jbd2_fc_end_commit(journal, tid, 1);
+	tid_t tid;
+
+	read_lock(&journal->j_state_lock);
+	tid = journal->j_running_transaction ?
+		journal->j_running_transaction->t_tid : 0;
+	read_unlock(&journal->j_state_lock);
+	return __jbd2_fc_end_commit(journal, tid, true);
 }
 EXPORT_SYMBOL(jbd2_fc_end_commit_fallback);
 
@@ -865,7 +873,6 @@  int jbd2_fc_get_buf(journal_t *journal, struct buffer_head **bh_out)
 	int fc_off;
 
 	*bh_out = NULL;
-	write_lock(&journal->j_state_lock);
 
 	if (journal->j_fc_off + journal->j_fc_first < journal->j_fc_last) {
 		fc_off = journal->j_fc_off;
@@ -874,7 +881,6 @@  int jbd2_fc_get_buf(journal_t *journal, struct buffer_head **bh_out)
 	} else {
 		ret = -EINVAL;
 	}
-	write_unlock(&journal->j_state_lock);
 
 	if (ret)
 		return ret;
@@ -887,11 +893,7 @@  int jbd2_fc_get_buf(journal_t *journal, struct buffer_head **bh_out)
 	if (!bh)
 		return -ENOMEM;
 
-	lock_buffer(bh);
 
-	clear_buffer_uptodate(bh);
-	set_buffer_dirty(bh);
-	unlock_buffer(bh);
 	journal->j_fc_wbuf[fc_off] = bh;
 
 	*bh_out = bh;
@@ -909,9 +911,7 @@  int jbd2_fc_wait_bufs(journal_t *journal, int num_blks)
 	struct buffer_head *bh;
 	int i, j_fc_off;
 
-	read_lock(&journal->j_state_lock);
 	j_fc_off = journal->j_fc_off;
-	read_unlock(&journal->j_state_lock);
 
 	/*
 	 * Wait in reverse order to minimize chances of us being woken up before
@@ -939,9 +939,7 @@  int jbd2_fc_release_bufs(journal_t *journal)
 	struct buffer_head *bh;
 	int i, j_fc_off;
 
-	read_lock(&journal->j_state_lock);
 	j_fc_off = journal->j_fc_off;
-	read_unlock(&journal->j_state_lock);
 
 	/*
 	 * Wait in reverse order to minimize chances of us being woken up before
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 9b4e87a0068b..df1285da7276 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -946,7 +946,9 @@  struct journal_s
 	 * @j_fc_off:
 	 *
 	 * Number of fast commit blocks currently allocated.
-	 * [j_state_lock].
+	 * [j_state_lock]. During the commit path, this variable is not
+	 * protected by j_state_lock since only one process performs commit
+	 * at a time.
 	 */
 	unsigned long		j_fc_off;
 
@@ -1110,7 +1112,9 @@  struct journal_s
 
 	/**
 	 * @j_fc_wbuf: Array of fast commit bhs for
-	 * jbd2_journal_commit_transaction.
+	 * jbd2_journal_commit_transaction. During the commit path, this
+	 * variable is not protected by j_state_lock since only one process
+	 * performs commit at a time.
 	 */
 	struct buffer_head	**j_fc_wbuf;
 
@@ -1618,7 +1622,7 @@  extern int jbd2_cleanup_journal_tail(journal_t *);
 int jbd2_fc_init(journal_t *journal);
 int jbd2_fc_begin_commit(journal_t *journal, tid_t tid);
 int jbd2_fc_end_commit(journal_t *journal);
-int jbd2_fc_end_commit_fallback(journal_t *journal, tid_t tid);
+int jbd2_fc_end_commit_fallback(journal_t *journal);
 int jbd2_fc_get_buf(journal_t *journal, struct buffer_head **bh_out);
 int jbd2_submit_inode_data(struct jbd2_inode *jinode);
 int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode);