Date:   Fri, 9 Oct 2020 21:40:04 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Harshad Shirwadkar <harshadshirwadkar@...il.com>,
        linux-ext4@...r.kernel.org
Cc:     tytso@....edu
Subject: Re: [PATCH v9 3/9] ext4 / jbd2: add fast commit initialization


Sorry about the delay. Few comments below.

On 9/19/20 6:24 AM, Harshad Shirwadkar wrote:
> This patch adds fast commit area trackers in the journal_t
> structure. These are initialized via the jbd2_fc_init() routine that
> this patch adds. This patch also adds ext4/fast_commit.c and
> ext4/fast_commit.h files for fast commit code that will be added in
> subsequent patches in this series.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>
> ---
>   fs/ext4/Makefile      |  2 +-
>   fs/ext4/ext4.h        |  4 ++++
>   fs/ext4/fast_commit.c | 20 +++++++++++++++++
>   fs/ext4/fast_commit.h |  9 ++++++++
>   fs/ext4/super.c       |  1 +
>   fs/jbd2/journal.c     | 52 ++++++++++++++++++++++++++++++++++++++-----
>   include/linux/jbd2.h  | 39 ++++++++++++++++++++++++++++++++
>   7 files changed, 121 insertions(+), 6 deletions(-)
>   create mode 100644 fs/ext4/fast_commit.c
>   create mode 100644 fs/ext4/fast_commit.h
> 
> diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
> index 2e42f47a7f98..49e7af6cc93f 100644
> --- a/fs/ext4/Makefile
> +++ b/fs/ext4/Makefile
> @@ -10,7 +10,7 @@ ext4-y	:= balloc.o bitmap.o block_validity.o dir.o ext4_jbd2.o extents.o \
>   		indirect.o inline.o inode.o ioctl.o mballoc.o migrate.o \
>   		mmp.o move_extent.o namei.o page-io.o readpage.o resize.o \
>   		super.o symlink.o sysfs.o xattr.o xattr_hurd.o xattr_trusted.o \
> -		xattr_user.o
> +		xattr_user.o fast_commit.o
> 
>   ext4-$(CONFIG_EXT4_FS_POSIX_ACL)	+= acl.o
>   ext4-$(CONFIG_EXT4_FS_SECURITY)		+= xattr_security.o
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 82e889d5c2ed..9af3971dd12e 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -964,6 +964,7 @@ do {									       \
>   #endif /* defined(__KERNEL__) || defined(__linux__) */
> 
>   #include "extents_status.h"
> +#include "fast_commit.h"
> 
>   /*
>    * Lock subclasses for i_data_sem in the ext4_inode_info structure.
> @@ -2679,6 +2680,9 @@ extern int ext4_init_inode_table(struct super_block *sb,
>   				 ext4_group_t group, int barrier);
>   extern void ext4_end_bitmap_read(struct buffer_head *bh, int uptodate);
> 
> +/* fast_commit.c */
> +
> +void ext4_fc_init(struct super_block *sb, journal_t *journal);
>   /* mballoc.c */
>   extern const struct seq_operations ext4_mb_seq_groups_ops;
>   extern long ext4_mb_stats;
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> new file mode 100644
> index 000000000000..0dad8bdb1253
> --- /dev/null
> +++ b/fs/ext4/fast_commit.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * fs/ext4/fast_commit.c
> + *
> + * Written by Harshad Shirwadkar <harshadshirwadkar@...il.com>
> + *
> + * Ext4 fast commits routines.
> + */
> +#include "ext4_jbd2.h"
> +
> +void ext4_fc_init(struct super_block *sb, journal_t *journal)
> +{
> +	if (!test_opt2(sb, JOURNAL_FAST_COMMIT))
> +		return;
> +	if (jbd2_fc_init(journal, EXT4_NUM_FC_BLKS)) {
> +		pr_warn("Error while enabling fast commits, turning off.");
> +		ext4_clear_feature_fast_commit(sb);
> +	}
> +}
> diff --git a/fs/ext4/fast_commit.h b/fs/ext4/fast_commit.h
> new file mode 100644
> index 000000000000..8362bf5e6e00
> --- /dev/null
> +++ b/fs/ext4/fast_commit.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __FAST_COMMIT_H__
> +#define __FAST_COMMIT_H__
> +
> +/* Number of blocks in journal area to allocate for fast commits */
> +#define EXT4_NUM_FC_BLKS		256

Just wanted to understand how is this value determined?
Do you think this needs to be configurable?
Just thinking since, on some platforms blksz could be of 64K.

> +
> +#endif /* __FAST_COMMIT_H__ */
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b62858ee420b..94aaaf940449 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4962,6 +4962,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_fc_init(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 17fdc482f554..736a1736619f 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1179,6 +1179,14 @@ static journal_t *journal_init_common(struct block_device *bdev,
>   	if (!journal->j_wbuf)
>   		goto err_cleanup;
> 
> +	if (journal->j_fc_wbufsize > 0) {
> +		journal->j_fc_wbuf = kmalloc_array(journal->j_fc_wbufsize,
> +					sizeof(struct buffer_head *),
> +					GFP_KERNEL);
> +		if (!journal->j_fc_wbuf)
> +			goto err_cleanup;
> +	}
> +
>   	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
>   	if (!bh) {
>   		pr_err("%s: Cannot get buffer for journal superblock\n",
> @@ -1192,11 +1200,22 @@ static journal_t *journal_init_common(struct block_device *bdev,
> 
>   err_cleanup:
>   	kfree(journal->j_wbuf);
> +	kfree(journal->j_fc_wbuf);
>   	jbd2_journal_destroy_revoke(journal);
>   	kfree(journal);
>   	return NULL;
>   }
> 
> +int jbd2_fc_init(journal_t *journal, int num_fc_blks)
> +{
> +	journal->j_fc_wbufsize = num_fc_blks;
> +	journal->j_fc_wbuf = kmalloc_array(journal->j_fc_wbufsize,
> +				sizeof(struct buffer_head *), GFP_KERNEL);
> +	if (!journal->j_fc_wbuf)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
>   /* jbd2_journal_init_dev and jbd2_journal_init_inode:
>    *
>    * Create a journal structure assigned some fixed set of disk blocks to
> @@ -1314,11 +1333,20 @@ 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_fc_wbufsize > 0) {
> +		journal->j_last_fc = last;
> +		journal->j_last = last - journal->j_fc_wbufsize;
> +		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;
> @@ -1663,9 +1691,18 @@ 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_fc_wbufsize > 0) {
> +		journal->j_last_fc = be32_to_cpu(sb->s_maxlen);
> +		journal->j_last = journal->j_last_fc - journal->j_fc_wbufsize;
> +		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;
>   }
> 
> @@ -1726,6 +1763,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. */
> @@ -1809,6 +1849,8 @@ int jbd2_journal_destroy(journal_t *journal)
>   		jbd2_journal_destroy_revoke(journal);
>   	if (journal->j_chksum_driver)
>   		crypto_free_shash(journal->j_chksum_driver);
> +	if (journal->j_fc_wbufsize > 0)
> +		kfree(journal->j_fc_wbuf);
>   	kfree(journal->j_wbuf);
>   	kfree(journal);
> 
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index f438257d7f31..36f65a818366 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -915,6 +915,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;

I guess choosing a single naming convention for fast commit would be 
very helpful for grepping/searching.
So for e.g. we could have everything using j_fc_**
If you agree, then we may have to change other members of this structure
accordingly.

-ritesh

> +
> +	/**
> +	 * @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.
>   	 */
> @@ -1065,6 +1089,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:
>   	 *
> @@ -1072,6 +1102,13 @@ struct journal_s
>   	 */
>   	int			j_wbufsize;
> 
> +	/**
> +	 * @j_fc_wbufsize:
> +	 *
> +	 * Size of @j_fc_wbuf array.
> +	 */
> +	int			j_fc_wbufsize;
> +
>   	/**
>   	 * @j_last_sync_writer:
>   	 *
> @@ -1507,6 +1544,8 @@ 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 *);
> 
> +/* Fast commit related APIs */
> +int jbd2_fc_init(journal_t *journal, int num_fc_blks);
>   /*
>    * is_journal_abort
>    *
> 

Powered by blists - more mailing lists