[PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()

Andreas Gruenbacher posted 2 patches 8 months, 1 week ago
[PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()
Posted by Andreas Gruenbacher 8 months, 1 week ago
From: Jan Kara <jack@suse.cz>

inode_to_wb() is used also for filesystems that don't support cgroup
writeback. For these filesystems inode->i_wb is stable during the
lifetime of the inode (it points to bdi->wb) and there's no need to hold
locks protecting the inode->i_wb dereference. Improve the warning in
inode_to_wb() to not trigger for these filesystems.

Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 include/linux/backing-dev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 8e7af9a03b41..e721148c95d0 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -249,6 +249,7 @@ static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
 {
 #ifdef CONFIG_LOCKDEP
 	WARN_ON_ONCE(debug_locks &&
+		     (inode->i_sb->s_iflags & SB_I_CGROUPWB) &&
 		     (!lockdep_is_held(&inode->i_lock) &&
 		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
 		      !lockdep_is_held(&inode->i_wb->list_lock)));
-- 
2.48.1
Re: [PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()
Posted by Andrew Morton 8 months, 1 week ago
On Sat, 12 Apr 2025 18:39:12 +0200 Andreas Gruenbacher <agruenba@redhat.com> wrote:

> inode_to_wb() is used also for filesystems that don't support cgroup
> writeback. For these filesystems inode->i_wb is stable during the
> lifetime of the inode (it points to bdi->wb) and there's no need to hold
> locks protecting the inode->i_wb dereference. Improve the warning in
> inode_to_wb() to not trigger for these filesystems.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

Yoo were on the patch delivery path so there should be a
signed-off-by:Andreas somewhere.  I made that change to the mm.git copy
of this patch.
Re: [PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()
Posted by Andreas Gruenbacher 8 months, 1 week ago
On Mon, Apr 14, 2025 at 11:51 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sat, 12 Apr 2025 18:39:12 +0200 Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> > inode_to_wb() is used also for filesystems that don't support cgroup
> > writeback. For these filesystems inode->i_wb is stable during the
> > lifetime of the inode (it points to bdi->wb) and there's no need to hold
> > locks protecting the inode->i_wb dereference. Improve the warning in
> > inode_to_wb() to not trigger for these filesystems.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
>
> Yoo were on the patch delivery path so there should be a
> signed-off-by:Andreas somewhere.  I made that change to the mm.git copy
> of this patch.

I guess that's fine as long as Jan is credited as the author.

Thanks,
Andreas
Re: [PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()
Posted by Andrew Morton 8 months, 1 week ago
On Sat, 12 Apr 2025 18:39:12 +0200 Andreas Gruenbacher <agruenba@redhat.com> wrote:

> From: Jan Kara <jack@suse.cz>
> 
> inode_to_wb() is used also for filesystems that don't support cgroup
> writeback. For these filesystems inode->i_wb is stable during the
> lifetime of the inode (it points to bdi->wb) and there's no need to hold
> locks protecting the inode->i_wb dereference. Improve the warning in
> inode_to_wb() to not trigger for these filesystems.
> 
> ...
>
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -249,6 +249,7 @@ static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
>  {
>  #ifdef CONFIG_LOCKDEP
>  	WARN_ON_ONCE(debug_locks &&
> +		     (inode->i_sb->s_iflags & SB_I_CGROUPWB) &&
>  		     (!lockdep_is_held(&inode->i_lock) &&
>  		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
>  		      !lockdep_is_held(&inode->i_wb->list_lock)));

Is this a does-nothing now GFS2 has been altered?

Otherwise, a bogus WARN is something we'll want to eliminate from
-stable kernels also.  Are we able to identify a Fixes: for this?

Thanks.
Re: [PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()
Posted by Andreas Gruenbacher 8 months, 1 week ago
On Mon, Apr 14, 2025 at 11:47 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sat, 12 Apr 2025 18:39:12 +0200 Andreas Gruenbacher <agruenba@redhat.com> wrote:
> > From: Jan Kara <jack@suse.cz>
> >
> > inode_to_wb() is used also for filesystems that don't support cgroup
> > writeback. For these filesystems inode->i_wb is stable during the
> > lifetime of the inode (it points to bdi->wb) and there's no need to hold
> > locks protecting the inode->i_wb dereference. Improve the warning in
> > inode_to_wb() to not trigger for these filesystems.
> >
> > ...
> >
> > --- a/include/linux/backing-dev.h
> > +++ b/include/linux/backing-dev.h
> > @@ -249,6 +249,7 @@ static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
> >  {
> >  #ifdef CONFIG_LOCKDEP
> >       WARN_ON_ONCE(debug_locks &&
> > +                  (inode->i_sb->s_iflags & SB_I_CGROUPWB) &&
> >                    (!lockdep_is_held(&inode->i_lock) &&
> >                     !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
> >                     !lockdep_is_held(&inode->i_wb->list_lock)));
>
> Is this a does-nothing now GFS2 has been altered?
>
> Otherwise, a bogus WARN is something we'll want to eliminate from
> -stable kernels also.  Are we able to identify a Fixes: for this?

The excess warnings started with commit:

  Fixes: aaa2cacf8184 ("writeback: add lockdep annotation to inode_to_wb()")

Getting rid of them requires this change, together with "gfs2: replace
sd_aspace with sd_inode" from gfs2 for-next:

  https://web.git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=
    a5fb828aba730d08aa6dec6bce3839f25e1f7a9d

Thanks,
Andreas
Re: [PATCH v3 2/2] writeback: Fix false warning in inode_to_wb()
Posted by Jan Kara 8 months, 1 week ago
Hi Andrew!

Can you please take this patch through MM tree? Thanks! Andreas is already
taking the first patch in the series through GFS2 tree.

								Honza

On Sat 12-04-25 18:39:12, Andreas Gruenbacher wrote:
> From: Jan Kara <jack@suse.cz>
> 
> inode_to_wb() is used also for filesystems that don't support cgroup
> writeback. For these filesystems inode->i_wb is stable during the
> lifetime of the inode (it points to bdi->wb) and there's no need to hold
> locks protecting the inode->i_wb dereference. Improve the warning in
> inode_to_wb() to not trigger for these filesystems.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  include/linux/backing-dev.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 8e7af9a03b41..e721148c95d0 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -249,6 +249,7 @@ static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
>  {
>  #ifdef CONFIG_LOCKDEP
>  	WARN_ON_ONCE(debug_locks &&
> +		     (inode->i_sb->s_iflags & SB_I_CGROUPWB) &&
>  		     (!lockdep_is_held(&inode->i_lock) &&
>  		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
>  		      !lockdep_is_held(&inode->i_wb->list_lock)));
> -- 
> 2.48.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR