| 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
| ||
|
Date: Tue, 18 Aug 2020 08:39:57 +0530
From: Ritesh Harjani <riteshh@...ux.ibm.com>
To: brookxu <brookxu.cn@...il.com>, adilger.kernel@...ger.ca,
tytso@....edu
Cc: jack@...e.cz, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v5 2/2] ext4: limit the length of per-inode prealloc list
On 8/17/20 1:06 PM, brookxu wrote:
> In the scenario of writing sparse files, the per-inode prealloc list may
> be very long, resulting in high overhead for ext4_mb_use_preallocated().
> To circumvent this problem, we limit the maximum length of per-inode
> prealloc list to 512 and allow users to modify it.
>
> After patching, we observed that the sys ratio of cpu has dropped, and
> the system throughput has increased significantly. We created a process
> to write the sparse file, and the running time of the process on the
> fixed kernel was significantly reduced, as follows:
>
> Running time on unfixed kernel：
> [root@...CENT64 ~]# time taskset 0x01 ./sparse /data1/sparce.dat
> real 0m2.051s
> user 0m0.008s
> sys 0m2.026s
>
> Running time on fixed kernel：
> [root@...CENT64 ~]# time taskset 0x01 ./sparse /data1/sparce.dat
> real 0m0.471s
> user 0m0.004s
> sys 0m0.395s
>
> V5:
> 1. Fix the wrong parameter of list_move().
> 2. Use an atomic variable to count the length of the prealloc list.
>
> V4:
> 1. Add performance data to the commit log.
>
> Signed-off-by: Chunguang Xu <brookxu@...cent.com>
> ---
> Documentation/admin-guide/ext4.rst | 3 ++
> fs/ext4/ext4.h | 4 ++-
> fs/ext4/extents.c | 10 +++---
> fs/ext4/file.c | 2 +-
> fs/ext4/indirect.c | 2 +-
> fs/ext4/inode.c | 6 ++--
> fs/ext4/ioctl.c | 2 +-
> fs/ext4/mballoc.c | 74 +++++++++++++++++++++++++++++++++-----
> fs/ext4/mballoc.h | 4 +++
> fs/ext4/move_extent.c | 4 +--
> fs/ext4/super.c | 3 +-
> fs/ext4/sysfs.c | 2 ++
> include/trace/events/ext4.h | 17 +++++----
> 13 files changed, 104 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/admin-guide/ext4.rst b/Documentation/admin-guide/ext4.rst
> index 9443fce..f37d074 100644
> --- a/Documentation/admin-guide/ext4.rst
> +++ b/Documentation/admin-guide/ext4.rst
> @@ -482,6 +482,9 @@ Files in /sys/fs/ext4/<devname>:
> multiple of this tuning parameter if the stripe size is not set in the
> ext4 superblock
>
> + mb_max_inode_prealloc
> + The maximum length of per-inode ext4_prealloc_space list.
> +
> mb_max_to_scan
> The maximum number of extents the multiblock allocator will search to
> find the best extent.
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 42f5060..c4b465c 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1054,6 +1054,7 @@ struct ext4_inode_info {
> struct timespec64 i_crtime;
>
> /* mballoc */
> + atomic_t i_prealloc_active;
> struct list_head i_prealloc_list;
> spinlock_t i_prealloc_lock;
>
> @@ -1501,6 +1502,7 @@ struct ext4_sb_info {
> unsigned int s_mb_stats;
> unsigned int s_mb_order2_reqs;
> unsigned int s_mb_group_prealloc;
> + unsigned int s_mb_max_inode_prealloc;
> unsigned int s_max_dir_size_kb;
> /* where last allocation was done - for stream allocation */
> unsigned long s_mb_last_group;
> @@ -2651,7 +2653,7 @@ extern int ext4_init_inode_table(struct super_block *sb,
> extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *,
> struct ext4_allocation_request *, int *);
> extern int ext4_mb_reserve_blocks(struct super_block *, int);
> -extern void ext4_discard_preallocations(struct inode *);
> +extern void ext4_discard_preallocations(struct inode *, unsigned int);
> extern int __init ext4_init_mballoc(void);
> extern void ext4_exit_mballoc(void);
> extern void ext4_free_blocks(handle_t *handle, struct inode *inode,
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 221f240..a40f928 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -100,7 +100,7 @@ static int ext4_ext_trunc_restart_fn(struct inode *inode, int *dropped)
> * i_mutex. So we can safely drop the i_data_sem here.
> */
> BUG_ON(EXT4_JOURNAL(inode) == NULL);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
> up_write(&EXT4_I(inode)->i_data_sem);
> *dropped = 1;
> return 0;
> @@ -4272,7 +4272,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> * not a good idea to call discard here directly,
> * but otherwise we'd need to call it every free().
> */
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
> if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE)
> fb_flags = EXT4_FREE_BLOCKS_NO_QUOT_UPDATE;
> ext4_free_blocks(handle, inode, NULL, newblock,
> @@ -5299,7 +5299,7 @@ static int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
> }
>
> down_write(&EXT4_I(inode)->i_data_sem);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
>
> ret = ext4_es_remove_extent(inode, punch_start,
> EXT_MAX_BLOCKS - punch_start);
> @@ -5313,7 +5313,7 @@ static int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
> up_write(&EXT4_I(inode)->i_data_sem);
> goto out_stop;
> }
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
>
> ret = ext4_ext_shift_extents(inode, handle, punch_stop,
> punch_stop - punch_start, SHIFT_LEFT);
> @@ -5445,7 +5445,7 @@ static int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
> goto out_stop;
>
> down_write(&EXT4_I(inode)->i_data_sem);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
>
> path = ext4_find_extent(inode, offset_lblk, NULL, 0);
> if (IS_ERR(path)) {
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 2a01e31..e3ab8ea 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -148,7 +148,7 @@ static int ext4_release_file(struct inode *inode, struct file *filp)
> !EXT4_I(inode)->i_reserved_data_blocks)
> {
> down_write(&EXT4_I(inode)->i_data_sem);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
> up_write(&EXT4_I(inode)->i_data_sem);
> }
> if (is_dx(inode) && filp->private_data)
> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index be2b66e..ec6b930 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -696,7 +696,7 @@ static int ext4_ind_trunc_restart_fn(handle_t *handle, struct inode *inode,
> * i_mutex. So we can safely drop the i_data_sem here.
> */
> BUG_ON(EXT4_JOURNAL(inode) == NULL);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
> up_write(&EXT4_I(inode)->i_data_sem);
> *dropped = 1;
> return 0;
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 10dd470..bb9e1cd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -383,7 +383,7 @@ void ext4_da_update_reserve_space(struct inode *inode,
> */
> if ((ei->i_reserved_data_blocks == 0) &&
> !inode_is_open_for_write(inode))
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
> }
>
> static int __check_block_validity(struct inode *inode, const char *func,
> @@ -4056,7 +4056,7 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
> if (stop_block > first_block) {
>
> down_write(&EXT4_I(inode)->i_data_sem);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
>
> ret = ext4_es_remove_extent(inode, first_block,
> stop_block - first_block);
> @@ -4211,7 +4211,7 @@ int ext4_truncate(struct inode *inode)
>
> down_write(&EXT4_I(inode)->i_data_sem);
>
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
>
> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> err = ext4_ext_truncate(handle, inode);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 999cf6a..a5fcc23 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -202,7 +202,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
> reset_inode_seed(inode);
> reset_inode_seed(inode_bl);
>
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
>
> err = ext4_mark_inode_dirty(handle, inode);
> if (err < 0) {
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 51f37f1..58426c6 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2739,6 +2739,7 @@ int ext4_mb_init(struct super_block *sb)
> sbi->s_mb_stats = MB_DEFAULT_STATS;
> sbi->s_mb_stream_request = MB_DEFAULT_STREAM_THRESHOLD;
> sbi->s_mb_order2_reqs = MB_DEFAULT_ORDER2_REQS;
> + sbi->s_mb_max_inode_prealloc = MB_DEFAULT_MAX_INODE_PREALLOC;
> /*
> * The default group preallocation is 512, which for 4k block
> * sizes translates to 2 megabytes. However for bigalloc file
> @@ -3677,6 +3678,26 @@ void ext4_mb_generate_from_pa(struct super_block *sb, void *bitmap,
> mb_debug(sb, "preallocated %d for group %u\n", preallocated, group);
> }
>
> +static void ext4_mb_mark_pa_deleted(struct super_block *sb,
> + struct ext4_prealloc_space *pa)
> +{
> + struct ext4_inode_info *ei;
> +
> + if (pa->pa_deleted) {
> + ext4_warning(sb, "deaded pa, type:%d, pblk:%llu, lblk:%u, len:%d\n",
s/deaded/deleted ?
> + pa->pa_type, pa->pa_pstart, pa->pa_lstart,
> + pa->pa_len);
> + return;
> + }
> +
> + pa->pa_deleted = 1;
> +
> + if (pa->pa_type == MB_INODE_PA) {
> + ei = EXT4_I(pa->pa_inode);
> + atomic_dec(&ei->i_prealloc_active);
> + }
> +}
> +
> static void ext4_mb_pa_callback(struct rcu_head *head)
> {
> struct ext4_prealloc_space *pa;
> @@ -3709,7 +3730,7 @@ static void ext4_mb_put_pa(struct ext4_allocation_context *ac,
> return;
> }
>
> - pa->pa_deleted = 1;
> + ext4_mb_mark_pa_deleted(sb, pa);
> spin_unlock(&pa->pa_lock);
>
> grp_blk = pa->pa_pstart;
> @@ -3833,6 +3854,7 @@ static void ext4_mb_put_pa(struct ext4_allocation_context *ac,
> spin_lock(pa->pa_obj_lock);
> list_add_rcu(&pa->pa_inode_list, &ei->i_prealloc_list);
> spin_unlock(pa->pa_obj_lock);
> + atomic_inc(&ei->i_prealloc_active);
> }
>
> /*
> @@ -4043,7 +4065,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
> }
>
> /* seems this one can be freed ... */
> - pa->pa_deleted = 1;
> + ext4_mb_mark_pa_deleted(sb, pa);
>
> /* we can trust pa_free ... */
> free += pa->pa_free;
> @@ -4106,7 +4128,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
> *
> * FIXME!! Make sure it is valid at all the call sites
> */
> -void ext4_discard_preallocations(struct inode *inode)
> +void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
> {
> struct ext4_inode_info *ei = EXT4_I(inode);
> struct super_block *sb = inode->i_sb;
> @@ -4124,15 +4146,19 @@ void ext4_discard_preallocations(struct inode *inode)
>
> mb_debug(sb, "discard preallocation for inode %lu\n",
> inode->i_ino);
> - trace_ext4_discard_preallocations(inode);
> + trace_ext4_discard_preallocations(inode,
> + atomic_read(&ei->i_prealloc_active), needed);
>
> INIT_LIST_HEAD(&list);
>
> + if (needed == 0)
> + needed = UINT_MAX;
> +
> repeat:
> /* first, collect all pa's in the inode */
> spin_lock(&ei->i_prealloc_lock);
> - while (!list_empty(&ei->i_prealloc_list)) {
> - pa = list_entry(ei->i_prealloc_list.next,
> + while (!list_empty(&ei->i_prealloc_list) && needed) {
> + pa = list_entry(ei->i_prealloc_list.prev,
> struct ext4_prealloc_space, pa_inode_list);
> BUG_ON(pa->pa_obj_lock != &ei->i_prealloc_lock);
> spin_lock(&pa->pa_lock);
> @@ -4149,10 +4175,11 @@ void ext4_discard_preallocations(struct inode *inode)
>
> }
> if (pa->pa_deleted == 0) {
> - pa->pa_deleted = 1;
> + ext4_mb_mark_pa_deleted(sb, pa);
> spin_unlock(&pa->pa_lock);
> list_del_rcu(&pa->pa_inode_list);
> list_add(&pa->u.pa_tmp_list, &list);
> + needed--;
> continue;
> }
>
> @@ -4453,7 +4480,7 @@ static void ext4_mb_group_or_file(struct ext4_allocation_context *ac)
> BUG_ON(pa->pa_type != MB_GROUP_PA);
>
> /* seems this one can be freed ... */
> - pa->pa_deleted = 1;
> + ext4_mb_mark_pa_deleted(sb, pa);
Isn't this inside ext4_mb_discard_lg_preallocations()?
ext4_mb_mark_pa_deleted() is not required here. Since there is anyway a
BUG_ON() at top which ensures that this is MB_GROUP_PA type only.
-ritesh
> spin_unlock(&pa->pa_lock);
>
> list_del_rcu(&pa->pa_inode_list);
> @@ -4552,10 +4579,29 @@ static void ext4_mb_add_n_trim(struct ext4_allocation_context *ac)
> }
>
> /*
> + * if per-inode prealloc list is too long, trim some PA
> + */
> +static void ext4_mb_trim_inode_pa(struct inode *inode)
> +{
> + struct ext4_inode_info *ei = EXT4_I(inode);
> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> + int count, delta;
> +
> + count = atomic_read(&ei->i_prealloc_active);
> + delta = (sbi->s_mb_max_inode_prealloc >> 2) + 1;
> + if (count > sbi->s_mb_max_inode_prealloc + delta) {
> + count -= sbi->s_mb_max_inode_prealloc;
> + ext4_discard_preallocations(inode, count);
> + }
> +}
> +
> +/*
> * release all resource we used in allocation
> */
> static int ext4_mb_release_context(struct ext4_allocation_context *ac)
> {
> + struct inode *inode = ac->ac_inode;
> + struct ext4_inode_info *ei = EXT4_I(inode);
> struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
> struct ext4_prealloc_space *pa = ac->ac_pa;
> if (pa) {
> @@ -4581,6 +4627,17 @@ static int ext4_mb_release_context(struct ext4_allocation_context *ac)
> ext4_mb_add_n_trim(ac);
> }
> }
> +
> + if (pa->pa_type == MB_INODE_PA) {
> + /*
> + * treat per-inode prealloc list as a lru list, then try
> + * to trim the least recently used PA.
> + */
> + spin_lock(pa->pa_obj_lock);
> + list_move(&pa->pa_inode_list, &ei->i_prealloc_list);
> + spin_unlock(pa->pa_obj_lock);
> + }
> +
> ext4_mb_put_pa(ac, ac->ac_sb, pa);
> }
> if (ac->ac_bitmap_page)
> @@ -4590,6 +4647,7 @@ static int ext4_mb_release_context(struct ext4_allocation_context *ac)
> if (ac->ac_flags & EXT4_MB_HINT_GROUP_ALLOC)
> mutex_unlock(&ac->ac_lg->lg_mutex);
> ext4_mb_collect_stats(ac);
> + ext4_mb_trim_inode_pa(inode);
> return 0;
> }
>
> diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
> index 6b4d17c..e75b474 100644
> --- a/fs/ext4/mballoc.h
> +++ b/fs/ext4/mballoc.h
> @@ -73,6 +73,10 @@
> */
> #define MB_DEFAULT_GROUP_PREALLOC 512
>
> +/*
> + * maximum length of inode prealloc list
> + */
> +#define MB_DEFAULT_MAX_INODE_PREALLOC 512
>
> struct ext4_free_data {
> /* this links the free block information from sb_info */
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 1ed86fb..0d601b8 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -686,8 +686,8 @@
>
> out:
> if (*moved_len) {
> - ext4_discard_preallocations(orig_inode);
> - ext4_discard_preallocations(donor_inode);
> + ext4_discard_preallocations(orig_inode, 0);
> + ext4_discard_preallocations(donor_inode, 0);
> }
>
> ext4_ext_drop_refs(path);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 330957e..4df5dde 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1123,6 +1123,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
> inode_set_iversion(&ei->vfs_inode, 1);
> spin_lock_init(&ei->i_raw_lock);
> INIT_LIST_HEAD(&ei->i_prealloc_list);
> + atomic_set(&ei->i_prealloc_active, 0);
> spin_lock_init(&ei->i_prealloc_lock);
> ext4_es_init_tree(&ei->i_es_tree);
> rwlock_init(&ei->i_es_lock);
> @@ -1216,7 +1217,7 @@ void ext4_clear_inode(struct inode *inode)
> {
> invalidate_inode_buffers(inode);
> clear_inode(inode);
> - ext4_discard_preallocations(inode);
> + ext4_discard_preallocations(inode, 0);
> ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
> dquot_drop(inode);
> if (EXT4_I(inode)->jinode) {
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index 6c9fc9e..92f04e9 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -215,6 +215,7 @@ static ssize_t journal_task_show(struct ext4_sb_info *sbi, char *buf)
> EXT4_RW_ATTR_SBI_UI(mb_order2_req, s_mb_order2_reqs);
> EXT4_RW_ATTR_SBI_UI(mb_stream_req, s_mb_stream_request);
> EXT4_RW_ATTR_SBI_UI(mb_group_prealloc, s_mb_group_prealloc);
> +EXT4_RW_ATTR_SBI_UI(mb_max_inode_prealloc, s_mb_max_inode_prealloc);
> EXT4_RW_ATTR_SBI_UI(extent_max_zeroout_kb, s_extent_max_zeroout_kb);
> EXT4_ATTR(trigger_fs_error, 0200, trigger_test_error);
> EXT4_RW_ATTR_SBI_UI(err_ratelimit_interval_ms, s_err_ratelimit_state.interval);
> @@ -257,6 +258,7 @@ static ssize_t journal_task_show(struct ext4_sb_info *sbi, char *buf)
> ATTR_LIST(mb_order2_req),
> ATTR_LIST(mb_stream_req),
> ATTR_LIST(mb_group_prealloc),
> + ATTR_LIST(mb_max_inode_prealloc),
> ATTR_LIST(max_writeback_mb_bump),
> ATTR_LIST(extent_max_zeroout_kb),
> ATTR_LIST(trigger_fs_error),
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index cc41d69..628db6a 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -746,24 +746,29 @@
> );
>
> TRACE_EVENT(ext4_discard_preallocations,
> - TP_PROTO(struct inode *inode),
> + TP_PROTO(struct inode *inode, unsigned int len, unsigned int needed),
>
> - TP_ARGS(inode),
> + TP_ARGS(inode, len, needed),
>
> TP_STRUCT__entry(
> - __field( dev_t, dev )
> - __field( ino_t, ino )
> + __field( dev_t, dev )
> + __field( ino_t, ino )
> + __field( unsigned int, len )
> + __field( unsigned int, needed )
>
> ),
>
> TP_fast_assign(
> __entry->dev = inode->i_sb->s_dev;
> __entry->ino = inode->i_ino;
> + __entry->len = len;
> + __entry->needed = needed;
> ),
>
> - TP_printk("dev %d,%d ino %lu",
> + TP_printk("dev %d,%d ino %lu len: %u needed %u",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> - (unsigned long) __entry->ino)
> + (unsigned long) __entry->ino, __entry->len,
> + __entry->needed)
> );
>
> TRACE_EVENT(ext4_mb_discard_preallocations,
>
Powered by blists - more mailing lists