timer callback was running on bh, and there is no need to disable bh again.
Signed-off-by: Schspa Shi <[email protected]>
---
fs/btrfs/zstd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index fc42dd0badd7..faa74306f0b7 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -105,10 +105,10 @@ static void zstd_reclaim_timer_fn(struct timer_list *timer)
unsigned long reclaim_threshold = jiffies - ZSTD_BTRFS_RECLAIM_JIFFIES;
struct list_head *pos, *next;
- spin_lock_bh(&wsm.lock);
+ spin_lock(&wsm.lock);
if (list_empty(&wsm.lru_list)) {
- spin_unlock_bh(&wsm.lock);
+ spin_unlock(&wsm.lock);
return;
}
@@ -137,7 +137,7 @@ static void zstd_reclaim_timer_fn(struct timer_list *timer)
if (!list_empty(&wsm.lru_list))
mod_timer(&wsm.timer, jiffies + ZSTD_BTRFS_RECLAIM_JIFFIES);
- spin_unlock_bh(&wsm.lock);
+ spin_unlock(&wsm.lock);
}
/*
--
2.24.3 (Apple Git-128)
On Sat, Apr 09, 2022 at 02:15:23AM +0800, Schspa Shi wrote:
> timer callback was running on bh, and there is no need to disable bh again.
Why do you think so? There was a specific fix fee13fe96529 ("btrfs:
correct zstd workspace manager lock to use spin_lock_bh()") that
actually added the _bh, so either you need to explain why exactly it's
not needed anymore and verify that the reported lockdep warning from the
fix does not happen.
David Sterba <[email protected]> writes:
> On Sat, Apr 09, 2022 at 02:15:23AM +0800, Schspa Shi wrote:
>> timer callback was running on bh, and there is no need to disable bh again.
>
> Why do you think so? There was a specific fix fee13fe96529 ("btrfs:
> correct zstd workspace manager lock to use spin_lock_bh()") that
> actually added the _bh, so either you need to explain why exactly it's
> not needed anymore and verify that the reported lockdep warning from the
> fix does not happen.
Yes, I've seen this fix, and wsm.lru_list is protected by wsm.lock.
This patch will not remove all changes that were fixed. Just a little
improvement
to remove the unnecessary bh disabling. Like
static inline void red_adaptative_timer(struct timer_list *t)
in net/sched/sch_red.c.
Because the critical section is only used by the process context and
the softirq context,
it is safe to remove bh_disable in the softirq context since it will
not be preempted by the softirq.
BRs
Schspa Shi
On Sat, Apr 09, 2022 at 03:36:54PM +0800, Schspa Shi wrote:
> David Sterba <[email protected]> writes:
>
> > On Sat, Apr 09, 2022 at 02:15:23AM +0800, Schspa Shi wrote:
> >> timer callback was running on bh, and there is no need to disable bh again.
> >
> > Why do you think so? There was a specific fix fee13fe96529 ("btrfs:
> > correct zstd workspace manager lock to use spin_lock_bh()") that
> > actually added the _bh, so either you need to explain why exactly it's
> > not needed anymore and verify that the reported lockdep warning from the
> > fix does not happen.
>
> Yes, I've seen this fix, and wsm.lru_list is protected by wsm.lock.
> This patch will not remove all changes that were fixed. Just a little
> improvement
> to remove the unnecessary bh disabling. Like
> static inline void red_adaptative_timer(struct timer_list *t)
> in net/sched/sch_red.c.
>
> Because the critical section is only used by the process context and
> the softirq context,
> it is safe to remove bh_disable in the softirq context since it will
> not be preempted by the softirq.
So why haven't you written that as a proper explanation the first time,
you apparenly analyzed the correctness. Please update the changelog and
also please try to rephrase it so it's more readable, I kind of
understand what you mean but it still leaves some things to hard to
read. Thanks.