| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <10591a3f-6dad-4e3d-f3f1-f10981cb4fe8@linux.ibm.com>
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