diff mbox series

[v3,02/13] jbd2: fast commit setup and enable

Message ID 20191001074101.256523-3-harshadshirwadkar@gmail.com
State Changes Requested
Headers show
Series ext4: add fast commit support | expand

Commit Message

harshad shirwadkar Oct. 1, 2019, 7:40 a.m. UTC 
This patch allows file systems to turn fast commits on and thereby
restrict the normal journalling space to total journal blocks minus
JBD2_FAST_COMMIT_BLOCKS. Fast commits are not actually performed, just
the interface to turn fast commits on is opened.

Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
---
 fs/ext4/super.c      |  5 +++-
 fs/jbd2/journal.c    | 68 +++++++++++++++++++++++++++++++++-----------
 include/linux/jbd2.h | 39 +++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 17 deletions(-)

Comments

Theodore Ts'o Oct. 16, 2019, 1:03 p.m. UTC | #1 
On Tue, Oct 01, 2019 at 12:40:51AM -0700, Harshad Shirwadkar wrote:
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 953990eb70a9..7c13834873ad 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1159,12 +1159,15 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	journal->j_blk_offset = start;
>  	journal->j_maxlen = len;
>  	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> -	journal->j_wbufsize = n;
> +	journal->j_wbufsize = n - JBD2_FAST_COMMIT_BLOCKS;
>  	journal->j_wbuf = kmalloc_array(n, sizeof(struct buffer_head *),
>  					GFP_KERNEL);
>  	if (!journal->j_wbuf)
>  		goto err_cleanup;
>  
> +	journal->j_fc_wbuf = &journal->j_wbuf[journal->j_wbufsize];
> +	journal->j_fc_wbufsize = JBD2_FAST_COMMIT_BLOCKS;
> +
>  	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
>  	if (!bh) {
>  		pr_err("%s: Cannot get buffer for journal superblock\n",

This is being done unconditionally, regardless of whether or not fast
commit has been enabled.  As a result, for the non-fc case, j_wbufsize
is going to be unconditionally reduced in size, which would be
unfortunate.

I suggest what you do is create a new function, called
jbd2_init_fast_commit() which is called from ext4_init_fast_commit(),
added in later patch, and which takes as an argument the size of the
fast_commit region (e.g., what is currently the constant
JBD2_FAST_COMMIT_BLOCKS), since this should be under the control of
the file system.

We can then pull these changes out of journal_init_common(), and move
them into jbd2_init_fast_commit().

> -/**
> - * int jbd2_journal_load() - Read journal from disk.
> - * @journal: Journal to act on.
> - *
> - * Given a journal_t structure which tells us which disk blocks contain
> - * a journal, read the journal from disk to initialise the in-memory
> - * structures.
> - */
> -int jbd2_journal_load(journal_t *journal)
> +static int __jbd2_journal_load(journal_t *journal, bool enable_fc)
>  {
>  	int err;
>  	journal_superblock_t *sb;

Instead of adding __jbd2_journal_load() with its enable_fc flag, we
can just test based on journal->j_fc_wbufsize being non-zero.  That
will have been set by jbd2_init_fast_commit(), which is called before
jbd2_journal_load().

As a result, we won't need to add __jbd2_journal_load() and the
jbd2_load_with_fc() functions.

> @@ -1684,6 +1694,12 @@ int jbd2_journal_load(journal_t *journal)
>  		return -EFSCORRUPTED;
>  	}
>  
> +	if (enable_fc)
> +		jbd2_journal_set_features(journal, 0, 0,
> +					  JBD2_FEATURE_INCOMPAT_FAST_COMMIT);
> +	else
> +		jbd2_journal_clear_features(journal, 0, 0,
> +					    JBD2_FEATURE_INCOMPAT_FAST_COMMIT);

We don't actually need to clear the feature, since it gets cleared
after the journal is successfully replayed.

> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index b7eed49b8ecd..84d04e1f3d92 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -918,6 +919,30 @@ struct journal_s
>  	 */
>  	unsigned long		j_last;
>  
> +	/**
> +	 * @j_first_fc:
> +	 *
> +	 * The block number of the first fast commit block in the journal
> +	 * [j_state_lock].
> +	 */
> +	unsigned long		j_first_fc;

Is this really protected by j_state_lock?  It's setup at journal load
time, and then never changed.  As a result, it's safe to read
j_first_fc without first taking the j_state_lock.

> +
> +	/**
> +	 * @j_fc_off:
> +	 *
> +	 * Number of fast commit blocks currently allocated.
> +	 * [j_state_lock].
> +	 */
> +	unsigned long		j_fc_off;

I'll mention this later, but we're not *actually* taking j_state_lock
when accessing j_fc_off.  In particular, jbd2_map_fc_buf() and its
caller (ext4_journal_fc_commit_cb) isn't taking j_state_lock.

I haven't had a chance to trace the locking hierarchy to figure out
whether the documentation or the locking is wrong, but my first
initial read is that the locking might be wrong?

> +
> +	/**
> +	 * @j_last_fc:
> +	 *
> +	 * The block number one beyond the last fast commit block in the journal
> +	 * [j_state_lock].
> +	 */
> +	unsigned long		j_last_fc;
> +

Again, this should never change once the journal structure is set up,
so it doesn't need to be protected by j_state_lock.

						- Ted
diff mbox series

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e376ac040cce..7725eb2105f4 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4933,7 +4933,10 @@  static int ext4_load_journal(struct super_block *sb,
 		if (save)
 			memcpy(save, ((char *) es) +
 			       EXT4_S_ERR_START, EXT4_S_ERR_LEN);
-		err = jbd2_journal_load(journal);
+		if (test_opt2(sb, JOURNAL_FAST_COMMIT))
+			err = jbd2_journal_load_with_fc(journal);
+		else
+			err = jbd2_journal_load(journal);
 		if (save)
 			memcpy(((char *) es) + EXT4_S_ERR_START,
 			       save, EXT4_S_ERR_LEN);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 953990eb70a9..7c13834873ad 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1159,12 +1159,15 @@  static journal_t *journal_init_common(struct block_device *bdev,
 	journal->j_blk_offset = start;
 	journal->j_maxlen = len;
 	n = journal->j_blocksize / sizeof(journal_block_tag_t);
-	journal->j_wbufsize = n;
+	journal->j_wbufsize = n - JBD2_FAST_COMMIT_BLOCKS;
 	journal->j_wbuf = kmalloc_array(n, sizeof(struct buffer_head *),
 					GFP_KERNEL);
 	if (!journal->j_wbuf)
 		goto err_cleanup;
 
+	journal->j_fc_wbuf = &journal->j_wbuf[journal->j_wbufsize];
+	journal->j_fc_wbufsize = JBD2_FAST_COMMIT_BLOCKS;
+
 	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
 	if (!bh) {
 		pr_err("%s: Cannot get buffer for journal superblock\n",
@@ -1297,11 +1300,19 @@  static int journal_reset(journal_t *journal)
 	}
 
 	journal->j_first = first;
-	journal->j_last = last;
 
-	journal->j_head = first;
-	journal->j_tail = first;
-	journal->j_free = last - first;
+	if (jbd2_has_feature_fast_commit(journal)) {
+		journal->j_last_fc = last;
+		journal->j_last = last - JBD2_FAST_COMMIT_BLOCKS;
+		journal->j_first_fc = journal->j_last + 1;
+		journal->j_fc_off = 0;
+	} else {
+		journal->j_last = last;
+	}
+
+	journal->j_head = journal->j_first;
+	journal->j_tail = journal->j_first;
+	journal->j_free = journal->j_last - journal->j_first;
 
 	journal->j_tail_sequence = journal->j_transaction_sequence;
 	journal->j_commit_sequence = journal->j_transaction_sequence - 1;
@@ -1626,22 +1637,21 @@  static int load_superblock(journal_t *journal)
 	journal->j_tail_sequence = be32_to_cpu(sb->s_sequence);
 	journal->j_tail = be32_to_cpu(sb->s_start);
 	journal->j_first = be32_to_cpu(sb->s_first);
-	journal->j_last = be32_to_cpu(sb->s_maxlen);
 	journal->j_errno = be32_to_cpu(sb->s_errno);
 
+	if (jbd2_has_feature_fast_commit(journal)) {
+		journal->j_last_fc = be32_to_cpu(sb->s_maxlen);
+		journal->j_last = journal->j_last_fc - JBD2_FAST_COMMIT_BLOCKS;
+		journal->j_first_fc = journal->j_last + 1;
+		journal->j_fc_off = 0;
+	} else {
+		journal->j_last = be32_to_cpu(sb->s_maxlen);
+	}
+
 	return 0;
 }
 
-
-/**
- * int jbd2_journal_load() - Read journal from disk.
- * @journal: Journal to act on.
- *
- * Given a journal_t structure which tells us which disk blocks contain
- * a journal, read the journal from disk to initialise the in-memory
- * structures.
- */
-int jbd2_journal_load(journal_t *journal)
+static int __jbd2_journal_load(journal_t *journal, bool enable_fc)
 {
 	int err;
 	journal_superblock_t *sb;
@@ -1684,6 +1694,12 @@  int jbd2_journal_load(journal_t *journal)
 		return -EFSCORRUPTED;
 	}
 
+	if (enable_fc)
+		jbd2_journal_set_features(journal, 0, 0,
+					  JBD2_FEATURE_INCOMPAT_FAST_COMMIT);
+	else
+		jbd2_journal_clear_features(journal, 0, 0,
+					    JBD2_FEATURE_INCOMPAT_FAST_COMMIT);
 	/* OK, we've finished with the dynamic journal bits:
 	 * reinitialise the dynamic contents of the superblock in memory
 	 * and reset them on disk. */
@@ -1699,6 +1715,26 @@  int jbd2_journal_load(journal_t *journal)
 	return -EIO;
 }
 
+/**
+ * int jbd2_journal_load() - Read journal from disk.
+ * @journal: Journal to act on.
+ *
+ * Given a journal_t structure which tells us which disk blocks contain
+ * a journal, read the journal from disk to initialise the in-memory
+ * structures.
+ */
+int jbd2_journal_load(journal_t *journal)
+{
+	return __jbd2_journal_load(journal, false);
+}
+
+/* Same as above but also enables fast commits. */
+int jbd2_journal_load_with_fc(journal_t *journal)
+{
+	return __jbd2_journal_load(journal, true);
+}
+
+
 /**
  * void jbd2_journal_destroy() - Release a journal_t structure.
  * @journal: Journal to act on.
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index b7eed49b8ecd..84d04e1f3d92 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -67,6 +67,7 @@  extern void *jbd2_alloc(size_t size, gfp_t flags);
 extern void jbd2_free(void *ptr, size_t size);
 
 #define JBD2_MIN_JOURNAL_BLOCKS 1024
+#define JBD2_FAST_COMMIT_BLOCKS 128
 
 #ifdef __KERNEL__
 
@@ -918,6 +919,30 @@  struct journal_s
 	 */
 	unsigned long		j_last;
 
+	/**
+	 * @j_first_fc:
+	 *
+	 * The block number of the first fast commit block in the journal
+	 * [j_state_lock].
+	 */
+	unsigned long		j_first_fc;
+
+	/**
+	 * @j_fc_off:
+	 *
+	 * Number of fast commit blocks currently allocated.
+	 * [j_state_lock].
+	 */
+	unsigned long		j_fc_off;
+
+	/**
+	 * @j_last_fc:
+	 *
+	 * The block number one beyond the last fast commit block in the journal
+	 * [j_state_lock].
+	 */
+	unsigned long		j_last_fc;
+
 	/**
 	 * @j_dev: Device where we store the journal.
 	 */
@@ -1061,6 +1086,12 @@  struct journal_s
 	 */
 	struct buffer_head	**j_wbuf;
 
+	/**
+	 * @j_fc_wbuf: Array of fast commit bhs for
+	 * jbd2_journal_commit_transaction.
+	 */
+	struct buffer_head	**j_fc_wbuf;
+
 	/**
 	 * @j_wbufsize:
 	 *
@@ -1068,6 +1099,13 @@  struct journal_s
 	 */
 	int			j_wbufsize;
 
+	/**
+	 * @j_fc_wbufsize:
+	 *
+	 * Size of @j_fc_wbuf array.
+	 */
+	int			j_fc_wbufsize;
+
 	/**
 	 * @j_last_sync_writer:
 	 *
@@ -1398,6 +1436,7 @@  extern int	   jbd2_journal_set_features
 extern void	   jbd2_journal_clear_features
 		   (journal_t *, unsigned long, unsigned long, unsigned long);
 extern int	   jbd2_journal_load       (journal_t *journal);
+extern int	   jbd2_journal_load_with_fc(journal_t *journal);
 extern int	   jbd2_journal_destroy    (journal_t *);
 extern int	   jbd2_journal_recover    (journal_t *journal);
 extern int	   jbd2_journal_wipe       (journal_t *, int);