diff mbox series

[RFC,2/2] writeback: Fix false warning in inode_to_wb()

Message ID 20250407182104.716631-3-agruenba@redhat.com (mailing list archive)
State New
Headers show
Series Fix false warning in inode_to_wb | expand

Commit Message

Andreas Gruenbacher April 7, 2025, 6:21 p.m. UTC 
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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig April 10, 2025, 8:52 a.m. UTC | #1 
On Mon, Apr 07, 2025 at 08:21:02PM +0200, Andreas Gruenbacher wrote:
> -static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
> +static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
>  {
>  #ifdef CONFIG_LOCKDEP
>  	WARN_ON_ONCE(debug_locks &&
> +		     inode_cgwb_enabled(inode) &&
>  		     (!lockdep_is_held(&inode->i_lock) &&
>  		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
>  		      !lockdep_is_held(&inode->i_wb->list_lock)));
> -- 

This means that even on cgroup aware file systems we now only get
the locking validation if cgroups are actually enabled for the file
system instance and thus hugely reducing coverage, which is rather
unfortunate.
Andreas Gruenbacher April 10, 2025, 5:48 p.m. UTC | #2 
On Thu, Apr 10, 2025 at 10:52 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Apr 07, 2025 at 08:21:02PM +0200, Andreas Gruenbacher wrote:
> > -static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
> > +static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
> >  {
> >  #ifdef CONFIG_LOCKDEP
> >       WARN_ON_ONCE(debug_locks &&
> > +                  inode_cgwb_enabled(inode) &&
> >                    (!lockdep_is_held(&inode->i_lock) &&
> >                     !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
> >                     !lockdep_is_held(&inode->i_wb->list_lock)));
> > --
>
> This means that even on cgroup aware file systems we now only get
> the locking validation if cgroups are actually enabled for the file
> system instance and thus hugely reducing coverage, which is rather
> unfortunate.

Right. Is checking for (inode->i_sb->s_iflags & SB_I_CGROUPWB) instead okay?

Thanks,
Andreas
diff mbox series

Patch

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 8e7af9a03b41..4069a027582f 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -245,10 +245,11 @@  wb_get_create_current(struct backing_dev_info *bdi, gfp_t gfp)
  * holding either @inode->i_lock, the i_pages lock, or the
  * associated wb's list_lock.
  */
-static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
+static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
 {
 #ifdef CONFIG_LOCKDEP
 	WARN_ON_ONCE(debug_locks &&
+		     inode_cgwb_enabled(inode) &&
 		     (!lockdep_is_held(&inode->i_lock) &&
 		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
 		      !lockdep_is_held(&inode->i_wb->list_lock)));