* [PATCH -next 1/3] ext4: Use scoped()/scoped_guard() to drop read_lock()/unlock pair
2024-08-23 6:18 [PATCH -next 0/3] ext4: Using scope-based resource management function Li Zetao
@ 2024-08-23 6:18 ` Li Zetao
2024-08-23 6:18 ` [PATCH -next 2/3] ext4: Use scoped()/scoped_guard() to drop write_lock()/unlock pair Li Zetao
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Li Zetao @ 2024-08-23 6:18 UTC (permalink / raw)
To: tytso, adilger.kernel; +Cc: lizetao1, linux-ext4
A read_lock() and read_unlock() pair can be replaced by a
scope-based resource management function scoped() or scoped_guard()
which can make the code more readable and safer.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
fs/ext4/extents_status.c | 41 +++++++---------------------
fs/ext4/fast_commit.c | 3 +--
fs/ext4/inode.c | 11 ++++----
fs/ext4/mballoc.c | 58 ++++++++++++++++++----------------------
4 files changed, 42 insertions(+), 71 deletions(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 17dcf13adde2..407447819864 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -317,9 +317,8 @@ void ext4_es_find_extent_range(struct inode *inode,
trace_ext4_es_find_extent_range_enter(inode, lblk);
- read_lock(&EXT4_I(inode)->i_es_lock);
- __es_find_extent_range(inode, matching_fn, lblk, end, es);
- read_unlock(&EXT4_I(inode)->i_es_lock);
+ scoped_guard(read_lock, &EXT4_I(inode)->i_es_lock)
+ __es_find_extent_range(inode, matching_fn, lblk, end, es);
trace_ext4_es_find_extent_range_exit(inode, es);
}
@@ -363,16 +362,11 @@ bool ext4_es_scan_range(struct inode *inode,
int (*matching_fn)(struct extent_status *es),
ext4_lblk_t lblk, ext4_lblk_t end)
{
- bool ret;
-
if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
return false;
- read_lock(&EXT4_I(inode)->i_es_lock);
- ret = __es_scan_range(inode, matching_fn, lblk, end);
- read_unlock(&EXT4_I(inode)->i_es_lock);
-
- return ret;
+ guard(read_lock)(&EXT4_I(inode)->i_es_lock);
+ return __es_scan_range(inode, matching_fn, lblk, end);
}
/*
@@ -409,16 +403,11 @@ bool ext4_es_scan_clu(struct inode *inode,
int (*matching_fn)(struct extent_status *es),
ext4_lblk_t lblk)
{
- bool ret;
-
if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
return false;
- read_lock(&EXT4_I(inode)->i_es_lock);
- ret = __es_scan_clu(inode, matching_fn, lblk);
- read_unlock(&EXT4_I(inode)->i_es_lock);
-
- return ret;
+ guard(read_lock)(&EXT4_I(inode)->i_es_lock);
+ return __es_scan_clu(inode, matching_fn, lblk);
}
static void ext4_es_list_add(struct inode *inode)
@@ -2044,13 +2033,9 @@ bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk)
{
struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
struct ext4_inode_info *ei = EXT4_I(inode);
- bool ret;
- read_lock(&ei->i_es_lock);
- ret = (bool)(__get_pending(inode, EXT4_B2C(sbi, lblk)) != NULL);
- read_unlock(&ei->i_es_lock);
-
- return ret;
+ guard(read_lock)(&ei->i_es_lock);
+ return __get_pending(inode, EXT4_B2C(sbi, lblk)) != NULL;
}
/*
@@ -2232,7 +2217,6 @@ unsigned int ext4_es_delayed_clu(struct inode *inode, ext4_lblk_t lblk,
{
struct ext4_inode_info *ei = EXT4_I(inode);
ext4_lblk_t end;
- unsigned int n;
if (len == 0)
return 0;
@@ -2240,13 +2224,8 @@ unsigned int ext4_es_delayed_clu(struct inode *inode, ext4_lblk_t lblk,
end = lblk + len - 1;
WARN_ON(end < lblk);
- read_lock(&ei->i_es_lock);
-
- n = __es_delayed_clu(inode, lblk, end);
-
- read_unlock(&ei->i_es_lock);
-
- return n;
+ guard(read_lock)(&ei->i_es_lock);
+ return __es_delayed_clu(inode, lblk, end);
}
/*
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 3926a05eceee..e2a773221523 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -347,10 +347,9 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
if (handle && !IS_ERR(handle))
tid = handle->h_transaction->t_tid;
else {
- read_lock(&sbi->s_journal->j_state_lock);
+ guard(read_lock)(&sbi->s_journal->j_state_lock);
tid = sbi->s_journal->j_running_transaction ?
sbi->s_journal->j_running_transaction->t_tid : 0;
- read_unlock(&sbi->s_journal->j_state_lock);
}
spin_lock(&sbi->s_fc_lock);
if (tid_gt(tid, sbi->s_fc_ineligible_tid))
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 03374dc215d1..2c978d8ff3ba 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4919,7 +4919,7 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
transaction_t *transaction;
tid_t tid;
- read_lock(&journal->j_state_lock);
+ guard(read_lock)(&journal->j_state_lock);
if (journal->j_running_transaction)
transaction = journal->j_running_transaction;
else
@@ -4928,7 +4928,6 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
tid = transaction->t_tid;
else
tid = journal->j_commit_sequence;
- read_unlock(&journal->j_state_lock);
ei->i_sync_tid = tid;
ei->i_datasync_tid = tid;
}
@@ -5303,10 +5302,10 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
if (ret != -EBUSY)
return;
commit_tid = 0;
- read_lock(&journal->j_state_lock);
- if (journal->j_committing_transaction)
- commit_tid = journal->j_committing_transaction->t_tid;
- read_unlock(&journal->j_state_lock);
+ scoped_guard(read_lock, &journal->j_state_lock)
+ if (journal->j_committing_transaction)
+ commit_tid = journal->j_committing_transaction->t_tid;
+
if (commit_tid)
jbd2_log_wait_commit(journal, commit_tid);
}
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 9dda9cd68ab2..db35148cc84a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -887,23 +887,22 @@ static void ext4_mb_choose_next_group_p2_aligned(struct ext4_allocation_context
for (i = ac->ac_2order; i < MB_NUM_ORDERS(ac->ac_sb); i++) {
if (list_empty(&sbi->s_mb_largest_free_orders[i]))
continue;
- read_lock(&sbi->s_mb_largest_free_orders_locks[i]);
- if (list_empty(&sbi->s_mb_largest_free_orders[i])) {
- read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
- continue;
- }
- list_for_each_entry(iter, &sbi->s_mb_largest_free_orders[i],
- bb_largest_free_order_node) {
- if (sbi->s_mb_stats)
- atomic64_inc(&sbi->s_bal_cX_groups_considered[CR_POWER2_ALIGNED]);
- if (likely(ext4_mb_good_group(ac, iter->bb_group, CR_POWER2_ALIGNED))) {
- *group = iter->bb_group;
- ac->ac_flags |= EXT4_MB_CR_POWER2_ALIGNED_OPTIMIZED;
- read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
- return;
+ scoped_guard(read_lock, &sbi->s_mb_largest_free_orders_locks[i]) {
+ if (list_empty(&sbi->s_mb_largest_free_orders[i]))
+ continue;
+ list_for_each_entry(iter, &sbi->s_mb_largest_free_orders[i],
+ bb_largest_free_order_node) {
+ if (sbi->s_mb_stats)
+ atomic64_inc(
+ &sbi->s_bal_cX_groups_considered[CR_POWER2_ALIGNED]);
+ if (likely(ext4_mb_good_group(ac, iter->bb_group,
+ CR_POWER2_ALIGNED))) {
+ *group = iter->bb_group;
+ ac->ac_flags |= EXT4_MB_CR_POWER2_ALIGNED_OPTIMIZED;
+ return;
+ }
}
}
- read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
}
/* Increment cr and search again if no group is found */
@@ -924,11 +923,10 @@ ext4_mb_find_good_group_avg_frag_lists(struct ext4_allocation_context *ac, int o
if (list_empty(frag_list))
return NULL;
- read_lock(frag_list_lock);
- if (list_empty(frag_list)) {
- read_unlock(frag_list_lock);
+ guard(read_lock)(frag_list_lock);
+ if (list_empty(frag_list))
return NULL;
- }
+
list_for_each_entry(iter, frag_list, bb_avg_fragment_size_node) {
if (sbi->s_mb_stats)
atomic64_inc(&sbi->s_bal_cX_groups_considered[cr]);
@@ -937,7 +935,6 @@ ext4_mb_find_good_group_avg_frag_lists(struct ext4_allocation_context *ac, int o
break;
}
}
- read_unlock(frag_list_lock);
return grp;
}
@@ -3236,11 +3233,10 @@ static int ext4_mb_seq_structs_summary_show(struct seq_file *seq, void *v)
seq_puts(seq, "avg_fragment_size_lists:\n");
count = 0;
- read_lock(&sbi->s_mb_avg_fragment_size_locks[position]);
- list_for_each_entry(grp, &sbi->s_mb_avg_fragment_size[position],
- bb_avg_fragment_size_node)
- count++;
- read_unlock(&sbi->s_mb_avg_fragment_size_locks[position]);
+ scoped_guard(read_lock, &sbi->s_mb_avg_fragment_size_locks[position])
+ list_for_each_entry(grp, &sbi->s_mb_avg_fragment_size[position],
+ bb_avg_fragment_size_node)
+ count++;
seq_printf(seq, "\tlist_order_%u_groups: %u\n",
(unsigned int)position, count);
return 0;
@@ -3252,11 +3248,10 @@ static int ext4_mb_seq_structs_summary_show(struct seq_file *seq, void *v)
seq_puts(seq, "max_free_order_lists:\n");
}
count = 0;
- read_lock(&sbi->s_mb_largest_free_orders_locks[position]);
- list_for_each_entry(grp, &sbi->s_mb_largest_free_orders[position],
- bb_largest_free_order_node)
- count++;
- read_unlock(&sbi->s_mb_largest_free_orders_locks[position]);
+ scoped_guard(read_lock, &sbi->s_mb_largest_free_orders_locks[position])
+ list_for_each_entry(grp, &sbi->s_mb_largest_free_orders[position],
+ bb_largest_free_order_node)
+ count++;
seq_printf(seq, "\tlist_order_%u_groups: %u\n",
(unsigned int)position, count);
@@ -4251,7 +4246,7 @@ ext4_mb_pa_assert_overlap(struct ext4_allocation_context *ac,
loff_t tmp_pa_end;
struct rb_node *iter;
- read_lock(&ei->i_prealloc_lock);
+ guard(read_lock)(&ei->i_prealloc_lock);
for (iter = ei->i_prealloc_node.rb_node; iter;
iter = ext4_mb_pa_rb_next_iter(start, tmp_pa_start, iter)) {
tmp_pa = rb_entry(iter, struct ext4_prealloc_space,
@@ -4264,7 +4259,6 @@ ext4_mb_pa_assert_overlap(struct ext4_allocation_context *ac,
BUG_ON(!(start >= tmp_pa_end || end <= tmp_pa_start));
spin_unlock(&tmp_pa->pa_lock);
}
- read_unlock(&ei->i_prealloc_lock);
}
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH -next 2/3] ext4: Use scoped()/scoped_guard() to drop write_lock()/unlock pair
2024-08-23 6:18 [PATCH -next 0/3] ext4: Using scope-based resource management function Li Zetao
2024-08-23 6:18 ` [PATCH -next 1/3] ext4: Use scoped()/scoped_guard() to drop read_lock()/unlock pair Li Zetao
@ 2024-08-23 6:18 ` Li Zetao
2024-08-23 6:18 ` [PATCH -next 3/3] ext4: Use scoped()/scoped_guard() to drop rcu_read_lock()/unlock pair Li Zetao
2024-11-07 4:16 ` [PATCH -next 0/3] ext4: Using scope-based resource management function Theodore Ts'o
3 siblings, 0 replies; 8+ messages in thread
From: Li Zetao @ 2024-08-23 6:18 UTC (permalink / raw)
To: tytso, adilger.kernel; +Cc: lizetao1, linux-ext4
A write_lock() and write_unlock() pair can be replaced by a
scope-based resource management function scoped() or scoped_guard()
which can make the code more readable and safer.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
fs/ext4/extents_status.c | 26 +++++----
fs/ext4/mballoc.c | 113 ++++++++++++++++++---------------------
fs/ext4/super.c | 3 +-
3 files changed, 64 insertions(+), 78 deletions(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 407447819864..b65e857bc686 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -953,12 +953,11 @@ void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
BUG_ON(end < lblk);
- write_lock(&EXT4_I(inode)->i_es_lock);
+ guard(write_lock)(&EXT4_I(inode)->i_es_lock);
es = __es_tree_search(&EXT4_I(inode)->i_es_tree.root, lblk);
if (!es || es->es_lblk > end)
__es_insert_extent(inode, &newes, NULL);
- write_unlock(&EXT4_I(inode)->i_es_lock);
}
/*
@@ -1512,15 +1511,16 @@ void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
* so that we are sure __es_shrink() is done with the inode before it
* is reclaimed.
*/
- write_lock(&EXT4_I(inode)->i_es_lock);
- err = __es_remove_extent(inode, lblk, end, &reserved, es);
- /* Free preallocated extent if it didn't get used. */
- if (es) {
- if (!es->es_len)
- __es_free_extent(es);
- es = NULL;
+ scoped_guard(write_lock, &EXT4_I(inode)->i_es_lock) {
+ err = __es_remove_extent(inode, lblk, end, &reserved, es);
+ /* Free preallocated extent if it didn't get used. */
+ if (es) {
+ if (!es->es_len)
+ __es_free_extent(es);
+ es = NULL;
+ }
}
- write_unlock(&EXT4_I(inode)->i_es_lock);
+
if (err)
goto retry;
@@ -1835,7 +1835,7 @@ void ext4_clear_inode_es(struct inode *inode)
struct ext4_es_tree *tree;
struct rb_node *node;
- write_lock(&ei->i_es_lock);
+ guard(write_lock)(&ei->i_es_lock);
tree = &EXT4_I(inode)->i_es_tree;
tree->cache_es = NULL;
node = rb_first(&tree->root);
@@ -1848,7 +1848,6 @@ void ext4_clear_inode_es(struct inode *inode)
}
}
ext4_clear_inode_state(inode, EXT4_STATE_EXT_PRECACHED);
- write_unlock(&ei->i_es_lock);
}
#ifdef ES_DEBUG__
@@ -2014,9 +2013,8 @@ void ext4_remove_pending(struct inode *inode, ext4_lblk_t lblk)
{
struct ext4_inode_info *ei = EXT4_I(inode);
- write_lock(&ei->i_es_lock);
+ guard(write_lock)(&ei->i_es_lock);
__remove_pending(inode, lblk);
- write_unlock(&ei->i_es_lock);
}
/*
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index db35148cc84a..e9bc4056ea94 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -852,19 +852,15 @@ mb_update_avg_fragment_size(struct super_block *sb, struct ext4_group_info *grp)
return;
if (grp->bb_avg_fragment_size_order != -1) {
- write_lock(&sbi->s_mb_avg_fragment_size_locks[
+ guard(write_lock)(&sbi->s_mb_avg_fragment_size_locks[
grp->bb_avg_fragment_size_order]);
list_del(&grp->bb_avg_fragment_size_node);
- write_unlock(&sbi->s_mb_avg_fragment_size_locks[
- grp->bb_avg_fragment_size_order]);
}
grp->bb_avg_fragment_size_order = new_order;
- write_lock(&sbi->s_mb_avg_fragment_size_locks[
+ guard(write_lock)(&sbi->s_mb_avg_fragment_size_locks[
grp->bb_avg_fragment_size_order]);
list_add_tail(&grp->bb_avg_fragment_size_node,
&sbi->s_mb_avg_fragment_size[grp->bb_avg_fragment_size_order]);
- write_unlock(&sbi->s_mb_avg_fragment_size_locks[
- grp->bb_avg_fragment_size_order]);
}
/*
@@ -1160,20 +1156,16 @@ mb_set_largest_free_order(struct super_block *sb, struct ext4_group_info *grp)
}
if (grp->bb_largest_free_order >= 0) {
- write_lock(&sbi->s_mb_largest_free_orders_locks[
+ guard(write_lock)(&sbi->s_mb_largest_free_orders_locks[
grp->bb_largest_free_order]);
list_del_init(&grp->bb_largest_free_order_node);
- write_unlock(&sbi->s_mb_largest_free_orders_locks[
- grp->bb_largest_free_order]);
}
grp->bb_largest_free_order = i;
if (grp->bb_largest_free_order >= 0 && grp->bb_free) {
- write_lock(&sbi->s_mb_largest_free_orders_locks[
+ guard(write_lock)(&sbi->s_mb_largest_free_orders_locks[
grp->bb_largest_free_order]);
list_add_tail(&grp->bb_largest_free_order_node,
&sbi->s_mb_largest_free_orders[grp->bb_largest_free_order]);
- write_unlock(&sbi->s_mb_largest_free_orders_locks[
- grp->bb_largest_free_order]);
}
}
@@ -5110,9 +5102,9 @@ static void ext4_mb_put_pa(struct ext4_allocation_context *ac,
ext4_unlock_group(sb, grp);
if (pa->pa_type == MB_INODE_PA) {
- write_lock(pa->pa_node_lock.inode_lock);
- rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
- write_unlock(pa->pa_node_lock.inode_lock);
+ scoped_guard(write_lock, pa->pa_node_lock.inode_lock)
+ rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
+
ext4_mb_pa_free(pa);
} else {
spin_lock(pa->pa_node_lock.lg_lock);
@@ -5241,9 +5233,9 @@ ext4_mb_new_inode_pa(struct ext4_allocation_context *ac)
list_add(&pa->pa_group_list, &grp->bb_prealloc_list);
- write_lock(pa->pa_node_lock.inode_lock);
- ext4_mb_pa_rb_insert(&ei->i_prealloc_node, &pa->pa_node.inode_node);
- write_unlock(pa->pa_node_lock.inode_lock);
+ scoped_guard(write_lock, pa->pa_node_lock.inode_lock)
+ ext4_mb_pa_rb_insert(&ei->i_prealloc_node, &pa->pa_node.inode_node);
+
atomic_inc(&ei->i_prealloc_active);
}
@@ -5472,10 +5464,9 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
list_del_rcu(&pa->pa_node.lg_list);
spin_unlock(pa->pa_node_lock.lg_lock);
} else {
- write_lock(pa->pa_node_lock.inode_lock);
+ guard(write_lock)(pa->pa_node_lock.inode_lock);
ei = EXT4_I(pa->pa_inode);
rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
- write_unlock(pa->pa_node_lock.inode_lock);
}
list_del(&pa->u.pa_tmp_list);
@@ -5532,54 +5523,52 @@ void ext4_discard_preallocations(struct inode *inode)
repeat:
/* first, collect all pa's in the inode */
- write_lock(&ei->i_prealloc_lock);
- for (iter = rb_first(&ei->i_prealloc_node); iter;
- iter = rb_next(iter)) {
- pa = rb_entry(iter, struct ext4_prealloc_space,
- pa_node.inode_node);
- BUG_ON(pa->pa_node_lock.inode_lock != &ei->i_prealloc_lock);
+ scoped_guard(write_lock, &ei->i_prealloc_lock) {
+ for (iter = rb_first(&ei->i_prealloc_node); iter;
+ iter = rb_next(iter)) {
+ pa = rb_entry(iter, struct ext4_prealloc_space,
+ pa_node.inode_node);
+ BUG_ON(pa->pa_node_lock.inode_lock != &ei->i_prealloc_lock);
- spin_lock(&pa->pa_lock);
- if (atomic_read(&pa->pa_count)) {
- /* this shouldn't happen often - nobody should
- * use preallocation while we're discarding it */
+ spin_lock(&pa->pa_lock);
+ if (atomic_read(&pa->pa_count)) {
+ /* this shouldn't happen often - nobody should
+ * use preallocation while we're discarding it */
+ spin_unlock(&pa->pa_lock);
+ ext4_msg(sb, KERN_ERR,
+ "uh-oh! used pa while discarding");
+ WARN_ON(1);
+ schedule_timeout_uninterruptible(HZ);
+ goto repeat;
+
+ }
+ if (pa->pa_deleted == 0) {
+ ext4_mb_mark_pa_deleted(sb, pa);
+ spin_unlock(&pa->pa_lock);
+ rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
+ list_add(&pa->u.pa_tmp_list, &list);
+ continue;
+ }
+
+ /* someone is deleting pa right now */
spin_unlock(&pa->pa_lock);
- write_unlock(&ei->i_prealloc_lock);
- ext4_msg(sb, KERN_ERR,
- "uh-oh! used pa while discarding");
- WARN_ON(1);
+
+ /* we have to wait here because pa_deleted
+ * doesn't mean pa is already unlinked from
+ * the list. as we might be called from
+ * ->clear_inode() the inode will get freed
+ * and concurrent thread which is unlinking
+ * pa from inode's list may access already
+ * freed memory, bad-bad-bad */
+
+ /* XXX: if this happens too often, we can
+ * add a flag to force wait only in case
+ * of ->clear_inode(), but not in case of
+ * regular truncate */
schedule_timeout_uninterruptible(HZ);
goto repeat;
-
- }
- if (pa->pa_deleted == 0) {
- ext4_mb_mark_pa_deleted(sb, pa);
- spin_unlock(&pa->pa_lock);
- rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
- list_add(&pa->u.pa_tmp_list, &list);
- continue;
}
-
- /* someone is deleting pa right now */
- spin_unlock(&pa->pa_lock);
- write_unlock(&ei->i_prealloc_lock);
-
- /* we have to wait here because pa_deleted
- * doesn't mean pa is already unlinked from
- * the list. as we might be called from
- * ->clear_inode() the inode will get freed
- * and concurrent thread which is unlinking
- * pa from inode's list may access already
- * freed memory, bad-bad-bad */
-
- /* XXX: if this happens too often, we can
- * add a flag to force wait only in case
- * of ->clear_inode(), but not in case of
- * regular truncate */
- schedule_timeout_uninterruptible(HZ);
- goto repeat;
}
- write_unlock(&ei->i_prealloc_lock);
list_for_each_entry_safe(pa, tmp, &list, u.pa_tmp_list) {
BUG_ON(pa->pa_type != MB_INODE_PA);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7ac562fd50a0..5ae7bc36eb78 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5717,7 +5717,7 @@ static void ext4_init_journal_params(struct super_block *sb, journal_t *journal)
journal->j_max_batch_time = sbi->s_max_batch_time;
ext4_fc_init(sb, journal);
- write_lock(&journal->j_state_lock);
+ guard(write_lock)(&journal->j_state_lock);
if (test_opt(sb, BARRIER))
journal->j_flags |= JBD2_BARRIER;
else
@@ -5731,7 +5731,6 @@ static void ext4_init_journal_params(struct super_block *sb, journal_t *journal)
* records log transactions continuously between each mount.
*/
journal->j_flags |= JBD2_CYCLE_RECORD;
- write_unlock(&journal->j_state_lock);
}
static struct inode *ext4_get_journal_inode(struct super_block *sb,
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH -next 3/3] ext4: Use scoped()/scoped_guard() to drop rcu_read_lock()/unlock pair
2024-08-23 6:18 [PATCH -next 0/3] ext4: Using scope-based resource management function Li Zetao
2024-08-23 6:18 ` [PATCH -next 1/3] ext4: Use scoped()/scoped_guard() to drop read_lock()/unlock pair Li Zetao
2024-08-23 6:18 ` [PATCH -next 2/3] ext4: Use scoped()/scoped_guard() to drop write_lock()/unlock pair Li Zetao
@ 2024-08-23 6:18 ` Li Zetao
2024-08-28 21:34 ` kernel test robot
2024-11-07 4:16 ` [PATCH -next 0/3] ext4: Using scope-based resource management function Theodore Ts'o
3 siblings, 1 reply; 8+ messages in thread
From: Li Zetao @ 2024-08-23 6:18 UTC (permalink / raw)
To: tytso, adilger.kernel; +Cc: lizetao1, linux-ext4
A rcu_read_lock() and rcu_read_unlock() pair can be replaced by a
scope-based resource management function scoped(rcu) or scoped_guard(rcu)
which can make the code more readable and safer. In some functions, we
can remove the goto label which will release the lock, and return from
function directly.
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
fs/ext4/block_validity.c | 27 +++++++++++++--------------
fs/ext4/ext4.h | 3 +--
fs/ext4/inode.c | 3 +--
fs/ext4/mballoc.c | 37 ++++++++++++++++---------------------
fs/ext4/resize.c | 20 ++++++++++----------
fs/ext4/super.c | 26 +++++++++++---------------
6 files changed, 52 insertions(+), 64 deletions(-)
diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 87ee3a17bd29..962ee95017fd 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -130,17 +130,17 @@ static void debug_print_tree(struct ext4_sb_info *sbi)
int first = 1;
printk(KERN_INFO "System zones: ");
- rcu_read_lock();
- system_blks = rcu_dereference(sbi->s_system_blks);
- node = rb_first(&system_blks->root);
- while (node) {
- entry = rb_entry(node, struct ext4_system_zone, node);
- printk(KERN_CONT "%s%llu-%llu", first ? "" : ", ",
- entry->start_blk, entry->start_blk + entry->count - 1);
- first = 0;
- node = rb_next(node);
+ scoped_guard(rcu) {
+ system_blks = rcu_dereference(sbi->s_system_blks);
+ node = rb_first(&system_blks->root);
+ while (node) {
+ entry = rb_entry(node, struct ext4_system_zone, node);
+ printk(KERN_CONT "%s%llu-%llu", first ? "" : ", ",
+ entry->start_blk, entry->start_blk + entry->count - 1);
+ first = 0;
+ node = rb_next(node);
+ }
}
- rcu_read_unlock();
printk(KERN_CONT "\n");
}
@@ -311,10 +311,10 @@ int ext4_sb_block_valid(struct super_block *sb, struct inode *inode,
* when doing a remount which inverse current "[no]block_validity"
* mount option.
*/
- rcu_read_lock();
+ guard(rcu)();
system_blks = rcu_dereference(sbi->s_system_blks);
if (system_blks == NULL)
- goto out_rcu;
+ return ret;
n = system_blks->root.rb_node;
while (n) {
@@ -330,8 +330,7 @@ int ext4_sb_block_valid(struct super_block *sb, struct inode *inode,
break;
}
}
-out_rcu:
- rcu_read_unlock();
+
return ret;
}
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 92a5e2d29599..13402929c757 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1809,9 +1809,8 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
#define sbi_array_rcu_deref(sbi, field, index) \
({ \
typeof(*((sbi)->field)) _v; \
- rcu_read_lock(); \
+ guard(rcu)(); \
_v = ((typeof(_v)*)rcu_dereference((sbi)->field))[index]; \
- rcu_read_unlock(); \
_v; \
})
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2c978d8ff3ba..1172b43dcc18 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5099,14 +5099,13 @@ static void ext4_update_other_inodes_time(struct super_block *sb,
* (assuming 4k blocks and 256 byte inodes) is (n*16 + 1).
*/
ino = ((orig_ino - 1) & ~(inodes_per_block - 1)) + 1;
- rcu_read_lock();
+ guard(rcu)();
for (i = 0; i < inodes_per_block; i++, ino++, buf += inode_size) {
if (ino == orig_ino)
continue;
__ext4_update_other_inode_time(sb, orig_ino, ino,
(struct ext4_inode *)buf);
}
- rcu_read_unlock();
}
/*
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e9bc4056ea94..905752a396c1 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3291,12 +3291,12 @@ int ext4_mb_alloc_groupinfo(struct super_block *sb, ext4_group_t ngroups)
ext4_msg(sb, KERN_ERR, "can't allocate buddy meta group");
return -ENOMEM;
}
- rcu_read_lock();
- old_groupinfo = rcu_dereference(sbi->s_group_info);
- if (old_groupinfo)
- memcpy(new_groupinfo, old_groupinfo,
- sbi->s_group_info_size * sizeof(*sbi->s_group_info));
- rcu_read_unlock();
+ scoped_guard(rcu) {
+ old_groupinfo = rcu_dereference(sbi->s_group_info);
+ if (old_groupinfo)
+ memcpy(new_groupinfo, old_groupinfo,
+ sbi->s_group_info_size * sizeof(*sbi->s_group_info));
+ }
rcu_assign_pointer(sbi->s_group_info, new_groupinfo);
sbi->s_group_info_size = size / sizeof(*sbi->s_group_info);
if (old_groupinfo)
@@ -3331,9 +3331,8 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
"for a buddy group");
return -ENOMEM;
}
- rcu_read_lock();
+ guard(rcu)();
rcu_dereference(sbi->s_group_info)[idx] = meta_group_info;
- rcu_read_unlock();
}
meta_group_info = sbi_array_rcu_deref(sbi, s_group_info, idx);
@@ -3377,11 +3376,10 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
if (group % EXT4_DESC_PER_BLOCK(sb) == 0) {
struct ext4_group_info ***group_info;
- rcu_read_lock();
+ guard(rcu)();
group_info = rcu_dereference(sbi->s_group_info);
kfree(group_info[idx]);
group_info[idx] = NULL;
- rcu_read_unlock();
}
return -ENOMEM;
} /* ext4_mb_add_groupinfo */
@@ -3462,16 +3460,15 @@ static int ext4_mb_init_backend(struct super_block *sb)
kmem_cache_free(cachep, grp);
}
i = sbi->s_group_info_size;
- rcu_read_lock();
- group_info = rcu_dereference(sbi->s_group_info);
- while (i-- > 0)
- kfree(group_info[i]);
- rcu_read_unlock();
+ scoped_guard(rcu) {
+ group_info = rcu_dereference(sbi->s_group_info);
+ while (i-- > 0)
+ kfree(group_info[i]);
+ }
iput(sbi->s_buddy_cache);
err_freesgi:
- rcu_read_lock();
+ guard(rcu)();
kvfree(rcu_dereference(sbi->s_group_info));
- rcu_read_unlock();
return -ENOMEM;
}
@@ -3789,12 +3786,11 @@ void ext4_mb_release(struct super_block *sb)
num_meta_group_infos = (ngroups +
EXT4_DESC_PER_BLOCK(sb) - 1) >>
EXT4_DESC_PER_BLOCK_BITS(sb);
- rcu_read_lock();
+ guard(rcu)();
group_info = rcu_dereference(sbi->s_group_info);
for (i = 0; i < num_meta_group_infos; i++)
kfree(group_info[i]);
kvfree(group_info);
- rcu_read_unlock();
}
kfree(sbi->s_mb_avg_fragment_size);
kfree(sbi->s_mb_avg_fragment_size_locks);
@@ -4946,7 +4942,7 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
* minimal distance from the goal block.
*/
for (i = order; i < PREALLOC_TB_SIZE; i++) {
- rcu_read_lock();
+ guard(rcu)();
list_for_each_entry_rcu(tmp_pa, &lg->lg_prealloc_list[i],
pa_node.lg_list) {
spin_lock(&tmp_pa->pa_lock);
@@ -4958,7 +4954,6 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
}
spin_unlock(&tmp_pa->pa_lock);
}
- rcu_read_unlock();
}
if (cpa) {
ext4_mb_use_group_pa(ac, cpa);
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 0ba9837d65ca..a72e0b1f6435 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -920,11 +920,11 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
}
brelse(dind);
- rcu_read_lock();
- o_group_desc = rcu_dereference(EXT4_SB(sb)->s_group_desc);
- memcpy(n_group_desc, o_group_desc,
- EXT4_SB(sb)->s_gdb_count * sizeof(struct buffer_head *));
- rcu_read_unlock();
+ scoped_guard(rcu) {
+ o_group_desc = rcu_dereference(EXT4_SB(sb)->s_group_desc);
+ memcpy(n_group_desc, o_group_desc,
+ EXT4_SB(sb)->s_gdb_count * sizeof(struct buffer_head *));
+ }
n_group_desc[gdb_num] = gdb_bh;
rcu_assign_pointer(EXT4_SB(sb)->s_group_desc, n_group_desc);
EXT4_SB(sb)->s_gdb_count++;
@@ -980,11 +980,11 @@ static int add_new_gdb_meta_bg(struct super_block *sb,
return err;
}
- rcu_read_lock();
- o_group_desc = rcu_dereference(EXT4_SB(sb)->s_group_desc);
- memcpy(n_group_desc, o_group_desc,
- EXT4_SB(sb)->s_gdb_count * sizeof(struct buffer_head *));
- rcu_read_unlock();
+ scoped_guard(rcu) {
+ o_group_desc = rcu_dereference(EXT4_SB(sb)->s_group_desc);
+ memcpy(n_group_desc, o_group_desc,
+ EXT4_SB(sb)->s_gdb_count * sizeof(struct buffer_head *));
+ }
n_group_desc[gdb_num] = gdb_bh;
BUFFER_TRACE(gdb_bh, "get_write_access");
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 5ae7bc36eb78..4826b1689c53 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1249,12 +1249,11 @@ static void ext4_group_desc_free(struct ext4_sb_info *sbi)
struct buffer_head **group_desc;
int i;
- rcu_read_lock();
+ guard(rcu)();
group_desc = rcu_dereference(sbi->s_group_desc);
for (i = 0; i < sbi->s_gdb_count; i++)
brelse(group_desc[i]);
kvfree(group_desc);
- rcu_read_unlock();
}
static void ext4_flex_groups_free(struct ext4_sb_info *sbi)
@@ -1262,14 +1261,13 @@ static void ext4_flex_groups_free(struct ext4_sb_info *sbi)
struct flex_groups **flex_groups;
int i;
- rcu_read_lock();
+ guard(rcu)();
flex_groups = rcu_dereference(sbi->s_flex_groups);
if (flex_groups) {
for (i = 0; i < sbi->s_flex_groups_allocated; i++)
kvfree(flex_groups[i]);
kvfree(flex_groups);
}
- rcu_read_unlock();
}
static void ext4_put_super(struct super_block *sb)
@@ -2892,14 +2890,13 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
seq_printf(seq, ",jqfmt=%s", fmtname);
}
- rcu_read_lock();
+ guard(rcu)();
usr_qf_name = rcu_dereference(sbi->s_qf_names[USRQUOTA]);
grp_qf_name = rcu_dereference(sbi->s_qf_names[GRPQUOTA]);
if (usr_qf_name)
seq_show_option(seq, "usrjquota", usr_qf_name);
if (grp_qf_name)
seq_show_option(seq, "grpjquota", grp_qf_name);
- rcu_read_unlock();
#endif
}
@@ -3140,13 +3137,13 @@ int ext4_alloc_flex_bg_array(struct super_block *sb, ext4_group_t ngroup)
return -ENOMEM;
}
}
- rcu_read_lock();
- old_groups = rcu_dereference(sbi->s_flex_groups);
- if (old_groups)
- memcpy(new_groups, old_groups,
- (sbi->s_flex_groups_allocated *
- sizeof(struct flex_groups *)));
- rcu_read_unlock();
+ scoped_guard(rcu) {
+ old_groups = rcu_dereference(sbi->s_flex_groups);
+ if (old_groups)
+ memcpy(new_groups, old_groups,
+ (sbi->s_flex_groups_allocated *
+ sizeof(struct flex_groups *)));
+ }
rcu_assign_pointer(sbi->s_flex_groups, new_groups);
sbi->s_flex_groups_allocated = size;
if (old_groups)
@@ -4851,9 +4848,8 @@ static int ext4_group_desc_init(struct super_block *sb,
sbi->s_gdb_count = i;
return PTR_ERR(bh);
}
- rcu_read_lock();
+ guard(rcu)();
rcu_dereference(sbi->s_group_desc)[i] = bh;
- rcu_read_unlock();
}
sbi->s_gdb_count = db_count;
if (!ext4_check_descriptors(sb, logical_sb_block, first_not_zeroed)) {
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH -next 3/3] ext4: Use scoped()/scoped_guard() to drop rcu_read_lock()/unlock pair
2024-08-23 6:18 ` [PATCH -next 3/3] ext4: Use scoped()/scoped_guard() to drop rcu_read_lock()/unlock pair Li Zetao
@ 2024-08-28 21:34 ` kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-08-28 21:34 UTC (permalink / raw)
To: Li Zetao, tytso, adilger.kernel; +Cc: llvm, oe-kbuild-all, lizetao1, linux-ext4
Hi Li,
kernel test robot noticed the following build warnings:
[auto build test WARNING on next-20240823]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Zetao/ext4-Use-scoped-scoped_guard-to-drop-read_lock-unlock-pair/20240826-123331
base: next-20240823
patch link: https://lore.kernel.org/r/20240823061824.3323522-4-lizetao1%40huawei.com
patch subject: [PATCH -next 3/3] ext4: Use scoped()/scoped_guard() to drop rcu_read_lock()/unlock pair
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240829/202408290407.XQuWf1oH-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 08e5a1de8227512d4774a534b91cb2353cef6284)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240829/202408290407.XQuWf1oH-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408290407.XQuWf1oH-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from fs/ext4/mballoc.c:12:
In file included from fs/ext4/ext4_jbd2.h:16:
In file included from include/linux/jbd2.h:23:
In file included from include/linux/buffer_head.h:12:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:10:
In file included from include/linux/mm.h:2202:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> fs/ext4/mballoc.c:3470:2: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
3470 | guard(rcu)();
| ^
include/linux/cleanup.h:303:2: note: expanded from macro 'guard'
303 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/cleanup.h:258:2: note: expanded from macro 'CLASS'
258 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^
<scratch space>:94:1: note: expanded from here
94 | class_rcu_t
| ^
5 warnings generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for OMAP2PLUS_MBOX
Depends on [n]: MAILBOX [=y] && (ARCH_OMAP2PLUS || ARCH_K3)
Selected by [m]:
- TI_K3_M4_REMOTEPROC [=m] && REMOTEPROC [=y] && (ARCH_K3 || COMPILE_TEST [=y])
vim +3470 fs/ext4/mballoc.c
3386
3387 static int ext4_mb_init_backend(struct super_block *sb)
3388 {
3389 ext4_group_t ngroups = ext4_get_groups_count(sb);
3390 ext4_group_t i;
3391 struct ext4_sb_info *sbi = EXT4_SB(sb);
3392 int err;
3393 struct ext4_group_desc *desc;
3394 struct ext4_group_info ***group_info;
3395 struct kmem_cache *cachep;
3396
3397 err = ext4_mb_alloc_groupinfo(sb, ngroups);
3398 if (err)
3399 return err;
3400
3401 sbi->s_buddy_cache = new_inode(sb);
3402 if (sbi->s_buddy_cache == NULL) {
3403 ext4_msg(sb, KERN_ERR, "can't get new inode");
3404 goto err_freesgi;
3405 }
3406 /* To avoid potentially colliding with an valid on-disk inode number,
3407 * use EXT4_BAD_INO for the buddy cache inode number. This inode is
3408 * not in the inode hash, so it should never be found by iget(), but
3409 * this will avoid confusion if it ever shows up during debugging. */
3410 sbi->s_buddy_cache->i_ino = EXT4_BAD_INO;
3411 EXT4_I(sbi->s_buddy_cache)->i_disksize = 0;
3412 for (i = 0; i < ngroups; i++) {
3413 cond_resched();
3414 desc = ext4_get_group_desc(sb, i, NULL);
3415 if (desc == NULL) {
3416 ext4_msg(sb, KERN_ERR, "can't read descriptor %u", i);
3417 goto err_freebuddy;
3418 }
3419 if (ext4_mb_add_groupinfo(sb, i, desc) != 0)
3420 goto err_freebuddy;
3421 }
3422
3423 if (ext4_has_feature_flex_bg(sb)) {
3424 /* a single flex group is supposed to be read by a single IO.
3425 * 2 ^ s_log_groups_per_flex != UINT_MAX as s_mb_prefetch is
3426 * unsigned integer, so the maximum shift is 32.
3427 */
3428 if (sbi->s_es->s_log_groups_per_flex >= 32) {
3429 ext4_msg(sb, KERN_ERR, "too many log groups per flexible block group");
3430 goto err_freebuddy;
3431 }
3432 sbi->s_mb_prefetch = min_t(uint, 1 << sbi->s_es->s_log_groups_per_flex,
3433 BLK_MAX_SEGMENT_SIZE >> (sb->s_blocksize_bits - 9));
3434 sbi->s_mb_prefetch *= 8; /* 8 prefetch IOs in flight at most */
3435 } else {
3436 sbi->s_mb_prefetch = 32;
3437 }
3438 if (sbi->s_mb_prefetch > ext4_get_groups_count(sb))
3439 sbi->s_mb_prefetch = ext4_get_groups_count(sb);
3440 /*
3441 * now many real IOs to prefetch within a single allocation at
3442 * CR_POWER2_ALIGNED. Given CR_POWER2_ALIGNED is an CPU-related
3443 * optimization we shouldn't try to load too many groups, at some point
3444 * we should start to use what we've got in memory.
3445 * with an average random access time 5ms, it'd take a second to get
3446 * 200 groups (* N with flex_bg), so let's make this limit 4
3447 */
3448 sbi->s_mb_prefetch_limit = sbi->s_mb_prefetch * 4;
3449 if (sbi->s_mb_prefetch_limit > ext4_get_groups_count(sb))
3450 sbi->s_mb_prefetch_limit = ext4_get_groups_count(sb);
3451
3452 return 0;
3453
3454 err_freebuddy:
3455 cachep = get_groupinfo_cache(sb->s_blocksize_bits);
3456 while (i-- > 0) {
3457 struct ext4_group_info *grp = ext4_get_group_info(sb, i);
3458
3459 if (grp)
3460 kmem_cache_free(cachep, grp);
3461 }
3462 i = sbi->s_group_info_size;
3463 scoped_guard(rcu) {
3464 group_info = rcu_dereference(sbi->s_group_info);
3465 while (i-- > 0)
3466 kfree(group_info[i]);
3467 }
3468 iput(sbi->s_buddy_cache);
3469 err_freesgi:
> 3470 guard(rcu)();
3471 kvfree(rcu_dereference(sbi->s_group_info));
3472 return -ENOMEM;
3473 }
3474
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH -next 0/3] ext4: Using scope-based resource management function
2024-08-23 6:18 [PATCH -next 0/3] ext4: Using scope-based resource management function Li Zetao
` (2 preceding siblings ...)
2024-08-23 6:18 ` [PATCH -next 3/3] ext4: Use scoped()/scoped_guard() to drop rcu_read_lock()/unlock pair Li Zetao
@ 2024-11-07 4:16 ` Theodore Ts'o
2024-11-07 21:00 ` Andreas Dilger
2024-11-08 11:06 ` Jan Kara
3 siblings, 2 replies; 8+ messages in thread
From: Theodore Ts'o @ 2024-11-07 4:16 UTC (permalink / raw)
To: Li Zetao; +Cc: adilger.kernel, linux-ext4
On Fri, Aug 23, 2024 at 02:18:21PM +0800, Li Zetao wrote:
> Hi all,
>
> This patch set is dedicated to using scope-based resource management
> functions to replace the direct use of lock/unlock methods, so that
> developers can focus more on using resources in a certain scope and
> avoid overly focusing on resource leakage issues.
>
> At the same time, some functions can remove the controversial goto
> label(eg: patch 3), which usually only releases resources and then
> exits the function. After replacement, these functions can exit
> directly without worrying about resources not being released.
>
> This patch set has been tested by fsstress for a long time and no
> problems were found.
Hmm, I'm torn. I do like the simplification that these patches can
offer.
The potential downsides/problems that are worrying me:
1) The zero day test bot has flagged a number of warnings[1]
[1] https://lore.kernel.org/r/202408290407.XQuWf1oH-lkp@intel.com
2) The documentation for guard() and scoped_guard() is pretty sparse,
and the comments in include/linux/cleanup.h are positively
confusing. There is a real need for a tutorial which explains how
they should be used in the Documentation directory, or maybe a
LWN.net article. Still, after staring that the implementation, I
was able to figure it out, but I'm bit worried that people who
aren't familiar with this construt which appears to have laned in
August 2023, might find the code less readable.
3) Once this this lands, I could see potential problems when bug fixes
are backported to stable kernels older than 6.6, since this changes
how lock and unlock calls in the ext4 code. So unless
include/linux/cleanup.h is backported to all of the LTS kernels, as
well as these ext4 patches, there is a ris that a future (possibly
security) bug fix will result in a missing unlock leading to
hilarity and/or sadness.
I'm reminded of the story of XFS changing the error return
semantics from errno to -errno, and resulting bugs when patches
were automatically backported to the stable kernels leading to
real problems, which is why XFS opted out of LTS backports. This
patch series could have the same problem.... and I haven't been
able to recruit someone to be the ext4 stable kernel maintainers
who could monitor xfstests resullts with lockdep enabled to catch
potential problems.
That being said, I do see the value of the change
What do other ext4 developers think?
- Ted
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH -next 0/3] ext4: Using scope-based resource management function
2024-11-07 4:16 ` [PATCH -next 0/3] ext4: Using scope-based resource management function Theodore Ts'o
@ 2024-11-07 21:00 ` Andreas Dilger
2024-11-08 11:06 ` Jan Kara
1 sibling, 0 replies; 8+ messages in thread
From: Andreas Dilger @ 2024-11-07 21:00 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Li Zetao, Ext4 Developers List
[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]
On Nov 6, 2024, at 9:16 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Aug 23, 2024 at 02:18:21PM +0800, Li Zetao wrote:
>> Hi all,
>>
>> This patch set is dedicated to using scope-based resource management
>> functions to replace the direct use of lock/unlock methods, so that
>> developers can focus more on using resources in a certain scope and
>> avoid overly focusing on resource leakage issues.
>>
>> At the same time, some functions can remove the controversial goto
>> label(eg: patch 3), which usually only releases resources and then
>> exits the function. After replacement, these functions can exit
>> directly without worrying about resources not being released.
>>
>> This patch set has been tested by fsstress for a long time and no
>> problems were found.
>
> Hmm, I'm torn. I do like the simplification that these patches can
> offer.
>
> The potential downsides/problems that are worrying me:
>
> 1) The zero day test bot has flagged a number of warnings[1]
>
> [1] https://lore.kernel.org/r/202408290407.XQuWf1oH-lkp@intel.com
>
> 2) The documentation for guard() and scoped_guard() is pretty sparse,
> and the comments in include/linux/cleanup.h are positively
> confusing. There is a real need for a tutorial which explains how
> they should be used in the Documentation directory, or maybe a
> LWN.net article. Still, after staring that the implementation, I
> was able to figure it out, but I'm bit worried that people who
> aren't familiar with this construt which appears to have laned in
> August 2023, might find the code less readable.
>
> 3) Once this this lands, I could see potential problems when bug fixes
> are backported to stable kernels older than 6.6, since this changes
> how lock and unlock calls in the ext4 code. So unless
> include/linux/cleanup.h is backported to all of the LTS kernels, as
> well as these ext4 patches, there is a ris that a future (possibly
> security) bug fix will result in a missing unlock leading to
> hilarity and/or sadness.
>
> I'm reminded of the story of XFS changing the error return
> semantics from errno to -errno, and resulting bugs when patches
> were automatically backported to the stable kernels leading to
> real problems, which is why XFS opted out of LTS backports. This
> patch series could have the same problem.... and I haven't been
> able to recruit someone to be the ext4 stable kernel maintainers
> who could monitor xfstests resullts with lockdep enabled to catch
> potential problems.
>
> That being said, I do see the value of the change
>
> What do other ext4 developers think?
Personally I don't see much improvement between the new code vs.
the existing code. Essentially it looks like some macros that wrap
the following block of code with the lock/unlock.
Could it avoid some classes of bugs? Maybe, for very simple cases
where the code block is very short, but I don't think few-line bodies
are the cases where "forgot to unlock" happens in practice. That is
more likely to happen with huge sprawling functions with multiple
intermediate error cases that need to be unwound, and it isn't clear
if these constructs will help in the real cases where they are needed.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH -next 0/3] ext4: Using scope-based resource management function
2024-11-07 4:16 ` [PATCH -next 0/3] ext4: Using scope-based resource management function Theodore Ts'o
2024-11-07 21:00 ` Andreas Dilger
@ 2024-11-08 11:06 ` Jan Kara
1 sibling, 0 replies; 8+ messages in thread
From: Jan Kara @ 2024-11-08 11:06 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Li Zetao, adilger.kernel, linux-ext4
On Wed 06-11-24 23:16:44, Theodore Ts'o wrote:
> On Fri, Aug 23, 2024 at 02:18:21PM +0800, Li Zetao wrote:
> > Hi all,
> >
> > This patch set is dedicated to using scope-based resource management
> > functions to replace the direct use of lock/unlock methods, so that
> > developers can focus more on using resources in a certain scope and
> > avoid overly focusing on resource leakage issues.
> >
> > At the same time, some functions can remove the controversial goto
> > label(eg: patch 3), which usually only releases resources and then
> > exits the function. After replacement, these functions can exit
> > directly without worrying about resources not being released.
> >
> > This patch set has been tested by fsstress for a long time and no
> > problems were found.
>
> Hmm, I'm torn. I do like the simplification that these patches can
> offer.
>
> The potential downsides/problems that are worrying me:
>
> 1) The zero day test bot has flagged a number of warnings[1]
>
> [1] https://lore.kernel.org/r/202408290407.XQuWf1oH-lkp@intel.com
>
> 2) The documentation for guard() and scoped_guard() is pretty sparse,
> and the comments in include/linux/cleanup.h are positively
> confusing. There is a real need for a tutorial which explains how
> they should be used in the Documentation directory, or maybe a
> LWN.net article. Still, after staring that the implementation, I
> was able to figure it out, but I'm bit worried that people who
> aren't familiar with this construt which appears to have laned in
> August 2023, might find the code less readable.
>
> 3) Once this this lands, I could see potential problems when bug fixes
> are backported to stable kernels older than 6.6, since this changes
> how lock and unlock calls in the ext4 code. So unless
> include/linux/cleanup.h is backported to all of the LTS kernels, as
> well as these ext4 patches, there is a ris that a future (possibly
> security) bug fix will result in a missing unlock leading to
> hilarity and/or sadness.
>
> I'm reminded of the story of XFS changing the error return
> semantics from errno to -errno, and resulting bugs when patches
> were automatically backported to the stable kernels leading to
> real problems, which is why XFS opted out of LTS backports. This
> patch series could have the same problem.... and I haven't been
> able to recruit someone to be the ext4 stable kernel maintainers
> who could monitor xfstests resullts with lockdep enabled to catch
> potential problems.
>
> That being said, I do see the value of the change
>
> What do other ext4 developers think?
So overall I consider guards a useful feature. That being said I find
changes as:
- rcu_read_lock();
+ guard(rcu)();
rcu_dereference(sbi->s_group_desc)[i] = bh;
- rcu_read_unlock();
actively harmful because they make you go "Eww, so when *does* the RCU get
released now?" If this was modified to:
- rcu_read_lock();
+ guard(rcu)() {
rcu_dereference(sbi->s_group_desc)[i] = bh;
}
- rcu_read_unlock();
then I wouldn't mind *that* much but I see such change mostly as a
pointless churn.
OTOH if we managed to successfully convert e.g. ext4_fill_super() into
using guards, then I would absolutely love that and in principle I think
that is possible. The unwinding code there is not pretty with various
#ifdef blocks, __maybe_unused annotations to silence warnings etc. But it
would take more than just a quick replacement of obvious blocks to do that
conversion.
So to sum it up, I'm not against using guards but as I've glanced over the
posted patches I find them to do more harm than good.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 8+ messages in thread