diff mbox series

[v6,03/20] ext4, jbd2: add fast commit initialization routines

Message ID 20200408215530.25649-3-harshads@google.com
State Superseded
Headers show
Series [v6,01/20] ext4: update docs for fast commit feature | expand

Commit Message

harshad shirwadkar April 8, 2020, 9:55 p.m. UTC 
From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

Define feature flags for fast commits and add routines to allow ext4 to
initialize fast commits. Note that we allow 128 blocks to be used for
fast commits. As of now, that's the default constant value.

Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
---
 fs/ext4/ext4.h       |  2 ++
 fs/ext4/ext4_jbd2.c  |  6 ++++++
 fs/ext4/ext4_jbd2.h  | 13 +++++++++++++
 fs/ext4/super.c      |  5 +++++
 fs/jbd2/journal.c    | 11 +++++++++++
 include/linux/jbd2.h | 19 ++++++++++++++++++-
 6 files changed, 55 insertions(+), 1 deletion(-)

Comments

Andreas Dilger April 10, 2020, 12:12 p.m. UTC | #1 
On Apr 8, 2020, at 3:55 PM, Harshad Shirwadkar <harshadshirwadkar@gmail.com> wrote:
> 
> From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> 
> Define feature flags for fast commits and add routines to allow ext4 to
> initialize fast commits. Note that we allow 128 blocks to be used for
> fast commits. As of now, that's the default constant value.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

> +static inline int ext4_should_fast_commit(struct super_block *sb)
> +{
> +	if (!ext4_has_feature_fast_commit(sb))
> +		return 0;
> +	if (!test_opt2(sb, JOURNAL_FAST_COMMIT))
> +		return 0;
> +	if (test_opt(sb, QUOTA))
> +		return 0;
> +	return 1;
> +}

This function seems more complex than it should be.  In this patch the
ext4_should_fast_commit() function is only called once during mount, but
in later patches it looks like it is called many times per file/inode.

Why not just check JOURNAL_FAST_COMMIT, and clear it at mount/remount
time if the other conditions prevent fast commits being used at all?
It seems that JOURNAL_FAST_COMMIT is only set if the FAST_COMMIT feature
is already in the superblock, so always doing both checks seems redundant.

Also, maybe I missed the discussion, but why does having quotas enabled
on the filesystem disable fast commits entirely?  I see in patch 11/20
that EXT4_FC_REASON_QUOTA is a reason not to do fast commit on the quota
inodes themselves, which seems like a reasonable limitation if needed,
but the above check disables FC for any filesystem with quota, and I
can't find anywhere that this line is later removed in this series.

Cheers, Andreas
harshad shirwadkar April 10, 2020, 5:51 p.m. UTC | #2 
On Fri, Apr 10, 2020 at 5:12 AM Andreas Dilger <adilger@dilger.ca> wrote:
>
> On Apr 8, 2020, at 3:55 PM, Harshad Shirwadkar <harshadshirwadkar@gmail.com> wrote:
> >
> > From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> >
> > Define feature flags for fast commits and add routines to allow ext4 to
> > initialize fast commits. Note that we allow 128 blocks to be used for
> > fast commits. As of now, that's the default constant value.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
>
> > +static inline int ext4_should_fast_commit(struct super_block *sb)
> > +{
> > +     if (!ext4_has_feature_fast_commit(sb))
> > +             return 0;
> > +     if (!test_opt2(sb, JOURNAL_FAST_COMMIT))
> > +             return 0;
> > +     if (test_opt(sb, QUOTA))
> > +             return 0;
> > +     return 1;
> > +}
>
> This function seems more complex than it should be.  In this patch the
> ext4_should_fast_commit() function is only called once during mount, but
> in later patches it looks like it is called many times per file/inode.
>
> Why not just check JOURNAL_FAST_COMMIT, and clear it at mount/remount
> time if the other conditions prevent fast commits being used at all?
> It seems that JOURNAL_FAST_COMMIT is only set if the FAST_COMMIT feature
> is already in the superblock, so always doing both checks seems redundant.
Sounds good, will fix that in the next version.
>
> Also, maybe I missed the discussion, but why does having quotas enabled
> on the filesystem disable fast commits entirely?  I see in patch 11/20
> that EXT4_FC_REASON_QUOTA is a reason not to do fast commit on the quota
> inodes themselves, which seems like a reasonable limitation if needed,
> but the above check disables FC for any filesystem with quota, and I
> can't find anywhere that this line is later removed in this series.

Thanks Andreas for catching this. Actually, there is nothing stopping
us from enabling fast commits for quota. As it turns out, this is an
unintended carry over from an earlier version of the patchset. I'll
re-enable it in the next version.

Thanks,
Harshad

>
> Cheers, Andreas
>
>
>
>
>
diff mbox series

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 7c3d89007eca..57f8fd4fe6ad 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1770,6 +1770,7 @@  static inline bool ext4_verity_in_progress(struct inode *inode)
 #define EXT4_FEATURE_COMPAT_RESIZE_INODE	0x0010
 #define EXT4_FEATURE_COMPAT_DIR_INDEX		0x0020
 #define EXT4_FEATURE_COMPAT_SPARSE_SUPER2	0x0200
+#define EXT4_FEATURE_COMPAT_FAST_COMMIT		0x0400
 #define EXT4_FEATURE_COMPAT_STABLE_INODES	0x0800
 
 #define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER	0x0001
@@ -1872,6 +1873,7 @@  EXT4_FEATURE_COMPAT_FUNCS(xattr,		EXT_ATTR)
 EXT4_FEATURE_COMPAT_FUNCS(resize_inode,		RESIZE_INODE)
 EXT4_FEATURE_COMPAT_FUNCS(dir_index,		DIR_INDEX)
 EXT4_FEATURE_COMPAT_FUNCS(sparse_super2,	SPARSE_SUPER2)
+EXT4_FEATURE_COMPAT_FUNCS(fast_commit,		FAST_COMMIT)
 EXT4_FEATURE_COMPAT_FUNCS(stable_inodes,	STABLE_INODES)
 
 EXT4_FEATURE_RO_COMPAT_FUNCS(sparse_super,	SPARSE_SUPER)
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 7f16e1af8d5c..91d6437bc9b3 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -367,3 +367,9 @@  int __ext4_handle_dirty_super(const char *where, unsigned int line,
 		mark_buffer_dirty(bh);
 	return err;
 }
+void ext4_init_fast_commit(struct super_block *sb, journal_t *journal)
+{
+	if (!ext4_should_fast_commit(sb))
+		return;
+	jbd2_init_fast_commit(journal, EXT4_NUM_FC_BLKS);
+}
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 4b9002f0e84c..b15cfa89cf1d 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -440,6 +440,17 @@  static inline int ext4_jbd2_inode_add_wait(handle_t *handle,
 	return 0;
 }
 
+static inline int ext4_should_fast_commit(struct super_block *sb)
+{
+	if (!ext4_has_feature_fast_commit(sb))
+		return 0;
+	if (!test_opt2(sb, JOURNAL_FAST_COMMIT))
+		return 0;
+	if (test_opt(sb, QUOTA))
+		return 0;
+	return 1;
+}
+
 static inline void ext4_update_inode_fsync_trans(handle_t *handle,
 						 struct inode *inode,
 						 int datasync)
@@ -518,4 +529,6 @@  static inline int ext4_should_dioread_nolock(struct inode *inode)
 	return 1;
 }
 
+#define EXT4_NUM_FC_BLKS		128
+void ext4_init_fast_commit(struct super_block *sb, journal_t *journal);
 #endif	/* _EXT4_JBD2_H */
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 70aaea283a63..0bfaf76200d2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3809,6 +3809,9 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 #ifdef CONFIG_EXT4_FS_POSIX_ACL
 	set_opt(sb, POSIX_ACL);
 #endif
+	if (ext4_has_feature_fast_commit(sb))
+		set_opt2(sb, JOURNAL_FAST_COMMIT);
+
 	/* don't forget to enable journal_csum when metadata_csum is enabled. */
 	if (ext4_has_metadata_csum(sb))
 		set_opt(sb, JOURNAL_CHECKSUM);
@@ -4463,6 +4466,7 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
 		clear_opt(sb, JOURNAL_CHECKSUM);
 		clear_opt(sb, DATA_FLAGS);
+		clear_opt2(sb, JOURNAL_FAST_COMMIT);
 		sbi->s_journal = NULL;
 		needs_recovery = 0;
 		goto no_journal;
@@ -4821,6 +4825,7 @@  static void ext4_init_journal_params(struct super_block *sb, journal_t *journal)
 	journal->j_commit_interval = sbi->s_commit_interval;
 	journal->j_min_batch_time = sbi->s_min_batch_time;
 	journal->j_max_batch_time = sbi->s_max_batch_time;
+	ext4_init_fast_commit(sb, journal);
 
 	write_lock(&journal->j_state_lock);
 	if (test_opt(sb, BARRIER))
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index a49d0e670ddf..4e5d41d79b24 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1196,6 +1196,14 @@  static journal_t *journal_init_common(struct block_device *bdev,
 	return NULL;
 }
 
+void jbd2_init_fast_commit(journal_t *journal, int num_fc_blks)
+{
+	journal->j_fc_wbufsize = num_fc_blks;
+	journal->j_wbufsize = journal->j_blocksize / sizeof(journal_block_tag_t)
+				- journal->j_fc_wbufsize;
+	journal->j_fc_wbuf = &journal->j_wbuf[journal->j_wbufsize];
+}
+
 /* jbd2_journal_init_dev and jbd2_journal_init_inode:
  *
  * Create a journal structure assigned some fixed set of disk blocks to
@@ -1722,6 +1730,9 @@  int jbd2_journal_load(journal_t *journal)
 	 */
 	journal->j_flags &= ~JBD2_ABORT;
 
+	if (journal->j_fc_wbufsize > 0)
+		jbd2_journal_set_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. */
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index f613d8529863..3bd1431cb222 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -288,6 +288,7 @@  typedef struct journal_superblock_s
 #define JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT	0x00000004
 #define JBD2_FEATURE_INCOMPAT_CSUM_V2		0x00000008
 #define JBD2_FEATURE_INCOMPAT_CSUM_V3		0x00000010
+#define JBD2_FEATURE_INCOMPAT_FAST_COMMIT	0x00000020
 
 /* See "journal feature predicate functions" below */
 
@@ -298,7 +299,8 @@  typedef struct journal_superblock_s
 					JBD2_FEATURE_INCOMPAT_64BIT | \
 					JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT | \
 					JBD2_FEATURE_INCOMPAT_CSUM_V2 | \
-					JBD2_FEATURE_INCOMPAT_CSUM_V3)
+					JBD2_FEATURE_INCOMPAT_CSUM_V3 | \
+					JBD2_FEATURE_INCOMPAT_FAST_COMMIT)
 
 #ifdef __KERNEL__
 
@@ -1058,6 +1060,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:
 	 *
@@ -1065,6 +1073,13 @@  struct journal_s
 	 */
 	int			j_wbufsize;
 
+	/**
+	 * @j_fc_wbufsize:
+	 *
+	 * Size of @j_fc_wbuf array.
+	 */
+	int			j_fc_wbufsize;
+
 	/**
 	 * @j_last_sync_writer:
 	 *
@@ -1234,6 +1249,7 @@  JBD2_FEATURE_INCOMPAT_FUNCS(64bit,		64BIT)
 JBD2_FEATURE_INCOMPAT_FUNCS(async_commit,	ASYNC_COMMIT)
 JBD2_FEATURE_INCOMPAT_FUNCS(csum2,		CSUM_V2)
 JBD2_FEATURE_INCOMPAT_FUNCS(csum3,		CSUM_V3)
+JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit,	FAST_COMMIT)
 
 /*
  * Journal flag definitions
@@ -1500,6 +1516,7 @@  void __jbd2_log_wait_for_space(journal_t *journal);
 extern void __jbd2_journal_drop_transaction(journal_t *, transaction_t *);
 extern int jbd2_cleanup_journal_tail(journal_t *);
 
+void jbd2_init_fast_commit(journal_t *journal, int num_fc_blks);
 /*
  * is_journal_abort
  *