| Message ID | c9858984b0807f758c8f1f5f64c1ec95c78930b2.1741662594.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] btrfs: add extra warning if delayed iput is added when it's not allowed | expand |
On Tue, Mar 11, 2025 at 3:10 AM Qu Wenruo <wqu@suse.com> wrote: > > Since I have triggered the ASSERT() on the delayed iput too many times, > now is the time to add some extra debug warnings for delayed iput. > > All delayed iputs should be queued after all ordered extents finish > their IO and all involved workqueues are flushed. > > Thus after the btrfs_run_delayed_iputs() inside close_ctree(), there > should be no more delayed puts added. > > So introduce a new BTRFS_FS_STATE_NO_DELAYED_IPUT, set after the above > mentioned timing. > And all btrfs_add_delayed_iput() will check that flag and give a > WARN_ON_ONCE() for debug builds. > > And since we're here, remove the btrfs_run_delayed_iputs() call inside > btrfs_error_commit_super(). > As that function will only wait for ordered extents to finish their IO, > delayed iputs will only be added when those ordered extents all finished > and removed from io tree, this is only ensured after the workqueue > flush. This last paragraph is no longer valid, as that part of the change was moved to another patch. Don't forget to delete it when committing to for-next. Otherwise: Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Changelog: > v3: > - Rebased to the latest for-next > Which has the removal of btrfs_run_delayed_iputs() from > btrfs_error_commit_super() in the previous patch. > > - Make the WARN_ON_ONCE() unconditional in btrfs_add_delayed_iput() > > v2: > - Set the new flag early > - Make the new flag unconditional except the WARN_ON_ONCE() check > - Remove the btrfs_run_delayed_iputs() inside btrfs_error_commit_super() > --- > fs/btrfs/disk-io.c | 2 ++ > fs/btrfs/fs.h | 3 +++ > fs/btrfs/inode.c | 1 + > 3 files changed, 6 insertions(+) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 0afd3c0f2fab..56e55f7a0f24 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4397,6 +4397,8 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info) > /* Ordered extents for free space inodes. */ > btrfs_flush_workqueue(fs_info->endio_freespace_worker); > btrfs_run_delayed_iputs(fs_info); > + /* There should be no more workload to generate new delayed iputs. */ > + set_bit(BTRFS_FS_STATE_NO_DELAYED_IPUT, &fs_info->fs_state); > > cancel_work_sync(&fs_info->async_reclaim_work); > cancel_work_sync(&fs_info->async_data_reclaim_work); > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h > index c799dfba5ebd..5a346d4a4b91 100644 > --- a/fs/btrfs/fs.h > +++ b/fs/btrfs/fs.h > @@ -117,6 +117,9 @@ enum { > /* Indicates there was an error cleaning up a log tree. */ > BTRFS_FS_STATE_LOG_CLEANUP_ERROR, > > + /* No more delayed iput can be queued. */ > + BTRFS_FS_STATE_NO_DELAYED_IPUT, > + > BTRFS_FS_STATE_COUNT > }; > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index e7e6accbaf6c..5c0a41c32dab 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3438,6 +3438,7 @@ void btrfs_add_delayed_iput(struct btrfs_inode *inode) > if (atomic_add_unless(&inode->vfs_inode.i_count, -1, 1)) > return; > > + WARN_ON_ONCE(test_bit(BTRFS_FS_STATE_NO_DELAYED_IPUT, &fs_info->fs_state)); > atomic_inc(&fs_info->nr_delayed_iputs); > /* > * Need to be irq safe here because we can be called from either an irq > -- > 2.48.1 > >
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 0afd3c0f2fab..56e55f7a0f24 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4397,6 +4397,8 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info) /* Ordered extents for free space inodes. */ btrfs_flush_workqueue(fs_info->endio_freespace_worker); btrfs_run_delayed_iputs(fs_info); + /* There should be no more workload to generate new delayed iputs. */ + set_bit(BTRFS_FS_STATE_NO_DELAYED_IPUT, &fs_info->fs_state); cancel_work_sync(&fs_info->async_reclaim_work); cancel_work_sync(&fs_info->async_data_reclaim_work); diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h index c799dfba5ebd..5a346d4a4b91 100644 --- a/fs/btrfs/fs.h +++ b/fs/btrfs/fs.h @@ -117,6 +117,9 @@ enum { /* Indicates there was an error cleaning up a log tree. */ BTRFS_FS_STATE_LOG_CLEANUP_ERROR, + /* No more delayed iput can be queued. */ + BTRFS_FS_STATE_NO_DELAYED_IPUT, + BTRFS_FS_STATE_COUNT }; diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index e7e6accbaf6c..5c0a41c32dab 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3438,6 +3438,7 @@ void btrfs_add_delayed_iput(struct btrfs_inode *inode) if (atomic_add_unless(&inode->vfs_inode.i_count, -1, 1)) return; + WARN_ON_ONCE(test_bit(BTRFS_FS_STATE_NO_DELAYED_IPUT, &fs_info->fs_state)); atomic_inc(&fs_info->nr_delayed_iputs); /* * Need to be irq safe here because we can be called from either an irq
Since I have triggered the ASSERT() on the delayed iput too many times, now is the time to add some extra debug warnings for delayed iput. All delayed iputs should be queued after all ordered extents finish their IO and all involved workqueues are flushed. Thus after the btrfs_run_delayed_iputs() inside close_ctree(), there should be no more delayed puts added. So introduce a new BTRFS_FS_STATE_NO_DELAYED_IPUT, set after the above mentioned timing. And all btrfs_add_delayed_iput() will check that flag and give a WARN_ON_ONCE() for debug builds. And since we're here, remove the btrfs_run_delayed_iputs() call inside btrfs_error_commit_super(). As that function will only wait for ordered extents to finish their IO, delayed iputs will only be added when those ordered extents all finished and removed from io tree, this is only ensured after the workqueue flush. Signed-off-by: Qu Wenruo <wqu@suse.com> --- Changelog: v3: - Rebased to the latest for-next Which has the removal of btrfs_run_delayed_iputs() from btrfs_error_commit_super() in the previous patch. - Make the WARN_ON_ONCE() unconditional in btrfs_add_delayed_iput() v2: - Set the new flag early - Make the new flag unconditional except the WARN_ON_ONCE() check - Remove the btrfs_run_delayed_iputs() inside btrfs_error_commit_super() --- fs/btrfs/disk-io.c | 2 ++ fs/btrfs/fs.h | 3 +++ fs/btrfs/inode.c | 1 + 3 files changed, 6 insertions(+)