| Message ID | 20250412163914.3773459-3-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix false warning in inode_to_wb | expand |
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 >
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.
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.
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
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
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)));