public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Schspa Shi <schspa@gmail.com>
Cc: clm@fb.com, Josef Bacik <josef@toxicpanda.com>,
	dsterba@suse.com, terrelln@fb.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] btrfs: zstd: use spin_lock in timer function
Date: Mon, 11 Apr 2022 15:51:37 +0200	[thread overview]
Message-ID: <20220411135136.GG15609@suse.cz> (raw)
In-Reply-To: <CAMA88Tp26+AWtcgU5yN=3Q5B8MSxqWMt=BpigQ3XADRJdOrpiA@mail.gmail.com>

On Sat, Apr 09, 2022 at 03:36:54PM +0800, Schspa Shi wrote:
> David Sterba <dsterba@suse.cz> 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.

  reply	other threads:[~2022-04-11 13:55 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-08 18:15 [PATCH] btrfs: zstd: use spin_lock in timer function Schspa Shi
2022-04-08 18:44 ` David Sterba
2022-04-09  7:36   ` Schspa Shi
2022-04-11 13:51     ` David Sterba [this message]
2022-04-11 15:55       ` [PATCH v2] btrfs: zstd: use spin_lock in timer callback Schspa Shi
2022-04-13 14:58         ` Nikolay Borisov
2022-04-13 16:00           ` David Sterba
2022-04-13 16:03           ` Schspa Shi
2022-04-13 19:08             ` Nikolay Borisov
2022-04-14 16:39               ` Schspa Shi
2022-04-14 19:26         ` David Sterba

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=20220411135136.GG15609@suse.cz \
    --to=dsterba@suse.cz \
    --cc=clm@fb.com \
    --cc=dsterba@suse.com \
    --cc=josef@toxicpanda.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=schspa@gmail.com \
    --cc=terrelln@fb.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox