From: Ritesh Harjani <riteshh@linux.ibm.com>
To: Harshad Shirwadkar <harshadshirwadkar@gmail.com>,
linux-ext4@vger.kernel.org
Cc: tytso@mit.edu
Subject: Re: [PATCH v9 3/9] ext4 / jbd2: add fast commit initialization
Date: Fri, 9 Oct 2020 21:40:04 +0530 [thread overview]
Message-ID: <10591a3f-6dad-4e3d-f3f1-f10981cb4fe8@linux.ibm.com> (raw)
In-Reply-To: <20200919005451.3899779-4-harshadshirwadkar@gmail.com>
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@gmail.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@gmail.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
> *
>
next prev parent reply other threads:[~2020-10-09 16:10 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-19 0:54 [PATCH v9 0/9] ext4: add fast commits feature Harshad Shirwadkar
2020-09-19 0:54 ` [PATCH v9 1/9] doc: update ext4 and journalling docs to include fast commit feature Harshad Shirwadkar
2020-09-22 17:50 ` Darrick J. Wong
2020-09-24 6:56 ` harshad shirwadkar
2020-10-09 18:28 ` Theodore Y. Ts'o
2020-10-13 0:27 ` harshad shirwadkar
2020-09-19 0:54 ` [PATCH v9 2/9] ext4: add fast_commit feature and handling for extended mount options Harshad Shirwadkar
2020-10-09 17:58 ` Theodore Y. Ts'o
2020-10-13 0:27 ` harshad shirwadkar
2020-09-19 0:54 ` [PATCH v9 3/9] ext4 / jbd2: add fast commit initialization Harshad Shirwadkar
2020-09-19 15:22 ` kernel test robot
2020-09-19 15:22 ` kernel test robot
2020-10-09 16:10 ` Ritesh Harjani [this message]
2020-10-13 0:28 ` harshad shirwadkar
2020-09-19 0:54 ` [PATCH v9 4/9] jbd2: add fast commit machinery Harshad Shirwadkar
2020-10-09 16:16 ` Ritesh Harjani
2020-10-13 0:27 ` harshad shirwadkar
2020-09-19 0:54 ` [PATCH v9 5/9] ext4: main fast-commit commit path Harshad Shirwadkar
2020-09-19 4:03 ` kernel test robot
2020-09-19 8:19 ` kernel test robot
2020-09-19 8:19 ` kernel test robot
2020-09-20 1:34 ` kernel test robot
2020-10-09 17:04 ` Ritesh Harjani
2020-10-13 0:25 ` harshad shirwadkar
2020-10-09 19:14 ` Theodore Y. Ts'o
2020-10-13 0:27 ` harshad shirwadkar
2020-09-19 0:54 ` [PATCH v9 6/9] jbd2: fast commit recovery path Harshad Shirwadkar
2020-09-19 11:27 ` kernel test robot
2020-09-19 0:54 ` [PATCH v9 7/9] ext4: " Harshad Shirwadkar
2020-09-19 14:15 ` kernel test robot
2020-09-19 14:15 ` kernel test robot
2020-10-09 17:14 ` Ritesh Harjani
2020-10-13 0:27 ` harshad shirwadkar
2020-09-19 0:54 ` [PATCH v9 8/9] ext4: add a mount opt to forcefully turn fast commits on Harshad Shirwadkar
2020-09-19 0:54 ` [PATCH v9 9/9] ext4: add fast commit stats in procfs Harshad Shirwadkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10591a3f-6dad-4e3d-f3f1-f10981cb4fe8@linux.ibm.com \
--to=riteshh@linux.ibm.com \
--cc=harshadshirwadkar@gmail.com \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.