[PATCH 0/3] btrfs: fix a couple of hangs during unmount caused by races

[PATCH 0/3] btrfs: fix a couple of hangs during unmount caused by races

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Due to some races or bad timmings, we can hang during unmount when trying
to stop the block group reclaim task or one of the space reclaim tasks.
The second case if often triggered by generic/562 for a while, but the
underlying problem has been there for a long time, despite seeming to be
more frequent recently. More details in the changelogs.

Filipe Manana (3):
  btrfs: fix hang during unmount when stopping block group reclaim worker
  btrfs: fix hang during unmount when stopping a space reclaim worker
  btrfs: remove useless used space increment during space reservation

 fs/btrfs/disk-io.c    | 42 ++++++++++++++++++++++++++++++++++++------
 fs/btrfs/space-info.c |  1 -
 2 files changed, 36 insertions(+), 7 deletions(-)

-- 
2.35.1

[PATCH 1/3] btrfs: fix hang during unmount when stopping block group reclaim worker

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

During early unmount, at close_ctree(), we try to stop the block group
reclaim task with cancel_work_sync(), but that may hang if the block group
reclaim task is currently at btrfs_relocate_block_group() waiting for the
flag BTRFS_FS_UNFINISHED_DROPS to be cleared from fs_info->flags. During
unmount we only clear that flag later, after trying to stop the block
group reclaim task.

Fix that by clearing BTRFS_FS_UNFINISHED_DROPS before trying to stop the
block group reclaim task and after setting BTRFS_FS_CLOSING_START, so that
if the reclaim task is waiting on that bit, it will stop immediately after
being woken, because it sees the filesystem is closing (with a call to
btrfs_fs_closing()), and then returns immediately with -EINTR.

Fixes: 31e70e527806c5 ("btrfs: fix hang during unmount when block group reclaim task is running")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e67614afcf4f..928adde1963d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4488,6 +4488,17 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 
 	set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
 
+	/*
+	 * If we had UNFINISHED_DROPS we could still be processing them, so
+	 * clear that bit and wake up relocation so it can stop.
+	 * We must do this before stopping the block group reclaim task, because
+	 * at btrfs_relocate_block_group() we wait for this bit, and after the
+	 * wait we stop with -EINTR if btrfs_fs_closing() returns non-zero - we
+	 * have just set BTRFS_FS_CLOSING_START, so btrfs_fs_closing() will
+	 * return 1.
+	 */
+	btrfs_wake_unfinished_drop(fs_info);
+
 	/*
 	 * We may have the reclaim task running and relocating a data block group,
 	 * in which case it may create delayed iputs. So stop it before we park
@@ -4506,12 +4517,6 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	 */
 	kthread_park(fs_info->cleaner_kthread);
 
-	/*
-	 * If we had UNFINISHED_DROPS we could still be processing them, so
-	 * clear that bit and wake up relocation so it can stop.
-	 */
-	btrfs_wake_unfinished_drop(fs_info);
-
 	/* wait for the qgroup rescan worker to stop */
 	btrfs_qgroup_wait_for_completion(fs_info, false);
 
-- 
2.35.1

[PATCH 2/3] btrfs: fix hang during unmount when stopping a space reclaim worker

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Often when running generic/562 from fstests we can hang during unmount,
resulting in a trace like this:

    Sep 07 11:52:00 debian9 unknown: run fstests generic/562 at 2022-09-07 11:52:00
    Sep 07 11:55:32 debian9 kernel: INFO: task umount:49438 blocked for more than 120 seconds.
    Sep 07 11:55:32 debian9 kernel:       Not tainted 6.0.0-rc2-btrfs-next-122 #1
    Sep 07 11:55:32 debian9 kernel: "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
    Sep 07 11:55:32 debian9 kernel: task:umount          state:D stack:    0 pid:49438 ppid: 25683 flags:0x00004000
    Sep 07 11:55:32 debian9 kernel: Call Trace:
    Sep 07 11:55:32 debian9 kernel:  <TASK>
    Sep 07 11:55:32 debian9 kernel:  __schedule+0x3c8/0xec0
    Sep 07 11:55:32 debian9 kernel:  ? rcu_read_lock_sched_held+0x12/0x70
    Sep 07 11:55:32 debian9 kernel:  schedule+0x5d/0xf0
    Sep 07 11:55:32 debian9 kernel:  schedule_timeout+0xf1/0x130
    Sep 07 11:55:32 debian9 kernel:  ? lock_release+0x224/0x4a0
    Sep 07 11:55:32 debian9 kernel:  ? lock_acquired+0x1a0/0x420
    Sep 07 11:55:32 debian9 kernel:  ? trace_hardirqs_on+0x2c/0xd0
    Sep 07 11:55:32 debian9 kernel:  __wait_for_common+0xac/0x200
    Sep 07 11:55:32 debian9 kernel:  ? usleep_range_state+0xb0/0xb0
    Sep 07 11:55:32 debian9 kernel:  __flush_work+0x26d/0x530
    Sep 07 11:55:32 debian9 kernel:  ? flush_workqueue_prep_pwqs+0x140/0x140
    Sep 07 11:55:32 debian9 kernel:  ? trace_clock_local+0xc/0x30
    Sep 07 11:55:32 debian9 kernel:  __cancel_work_timer+0x11f/0x1b0
    Sep 07 11:55:32 debian9 kernel:  ? close_ctree+0x12b/0x5b3 [btrfs]
    Sep 07 11:55:32 debian9 kernel:  ? __trace_bputs+0x10b/0x170
    Sep 07 11:55:32 debian9 kernel:  close_ctree+0x152/0x5b3 [btrfs]
    Sep 07 11:55:32 debian9 kernel:  ? evict_inodes+0x166/0x1c0
    Sep 07 11:55:32 debian9 kernel:  generic_shutdown_super+0x71/0x120
    Sep 07 11:55:32 debian9 kernel:  kill_anon_super+0x14/0x30
    Sep 07 11:55:32 debian9 kernel:  btrfs_kill_super+0x12/0x20 [btrfs]
    Sep 07 11:55:32 debian9 kernel:  deactivate_locked_super+0x2e/0xa0
    Sep 07 11:55:32 debian9 kernel:  cleanup_mnt+0x100/0x160
    Sep 07 11:55:32 debian9 kernel:  task_work_run+0x59/0xa0
    Sep 07 11:55:32 debian9 kernel:  exit_to_user_mode_prepare+0x1a6/0x1b0
    Sep 07 11:55:32 debian9 kernel:  syscall_exit_to_user_mode+0x16/0x40
    Sep 07 11:55:32 debian9 kernel:  do_syscall_64+0x48/0x90
    Sep 07 11:55:32 debian9 kernel:  entry_SYSCALL_64_after_hwframe+0x63/0xcd
    Sep 07 11:55:32 debian9 kernel: RIP: 0033:0x7fcde59a57a7
    Sep 07 11:55:32 debian9 kernel: RSP: 002b:00007ffe914217c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
    Sep 07 11:55:32 debian9 kernel: RAX: 0000000000000000 RBX: 00007fcde5ae8264 RCX: 00007fcde59a57a7
    Sep 07 11:55:32 debian9 kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 000055b57556cdd0
    Sep 07 11:55:32 debian9 kernel: RBP: 000055b57556cba0 R08: 0000000000000000 R09: 00007ffe91420570
    Sep 07 11:55:32 debian9 kernel: R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
    Sep 07 11:55:32 debian9 kernel: R13: 000055b57556cdd0 R14: 000055b57556ccb8 R15: 0000000000000000
    Sep 07 11:55:32 debian9 kernel:  </TASK>

What happens is the following:

1) The cleaner kthread tries to start a transaction to delete an unused
   block group, but the metadata reservation can not be satisfied right
   away, so a reservation ticket is created and it starts the async
   metadata reclaim task (fs_info->async_reclaim_work);

2) Writeback for all the filler inodes with an i_size of 2K starts
   (generic/562 creates a lot of 2K files with the goal of filling
   metadata space). We try to create an inline extent for them, but we
   fail when trying to insert the inline extent with -ENOSPC (at
   cow_file_range_inline()) - since this is not critical, we fallback
   to non-inline mode (back to cow_file_range()), reserve extents, create
   extent maps and create the ordered extents;

3) An unmount starts, enters close_ctree();

4) The async reclaim task is flushing stuff, entering the flush states one
   by one, until it reaches RUN_DELAYED_IPUTS. There it runs all current
   delayed iputs.

   After running the delayed iputs and before calling
   btrfs_wait_on_delayed_iputs(), one or more ordered extents complete,
   and btrfs_add_delayed_iput() is called for each one through
   btrfs_finish_ordered_io() -> btrfs_put_ordered_extent(). This results
   in bumping fs_info->nr_delayed_iputs from 0 to some positive value.

   So the async reclaim task blocks at btrfs_wait_on_delayed_iputs() waiting
   for fs_info->nr_delayed_iputs to become 0;

5) The current transaction is committed by the transaction kthread, we then
   start unpinning extents and end up calling btrfs_try_granting_tickets()
   through unpin_extent_range(), since we released some space.
   This results in satisfying the ticket created by the cleaner kthread at
   step 1, waking up the cleaner kthread;

6) At close_ctree() we ask the cleaner kthread to park;

7) The cleaner kthread starts the transaction, deletes the unused block
   group, and then calls kthread_should_park(), which returns true, so it
   parks. And at this point we have the delayed iputs added by the
   completion of the ordered extents still pending;

8) Then later at close_ctree(), when we call:

       cancel_work_sync(&fs_info->async_reclaim_work);

   We hang forever, since the cleaner was parked and no one else can run
   delayed iputs after that, while the reclaim task is waiting for the
   remaining delayed iputs to be completed.

Fix this by waiting for all ordered extents to complete and running the
delayed iputs before attempting to stop the async reclaim tasks. Note that
we can not wait for ordered extents with btrfs_wait_ordered_roots() (or
other similar functions) because that waits for the BTRFS_ORDERED_COMPLETE
flag to be set on an ordered extent, but the delayed iput is added after
that, when doing the final btrfs_put_ordered_extent(). So instead wait for
the work queues used for executing ordered extent completion to be empty,
which works because we do the final put on an ordered extent at
btrfs_finish_ordered_io() (while we are in the unmount context).

Fixes: d6fd0ae25c6495 ("Btrfs: fix missing delayed iputs on unmount")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 928adde1963d..fab9c981de8d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4539,6 +4539,31 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	/* clear out the rbtree of defraggable inodes */
 	btrfs_cleanup_defrag_inodes(fs_info);
 
+	/*
+	 * After we parked the cleaner kthread, ordered extents may have
+	 * completed and created new delayed iputs. If one of the async reclaim
+	 * tasks is running and in the RUN_DELAYED_IPUTS flush state, then we
+	 * can hang forever trying to stop it, because if a delayed iput is
+	 * added after it ran btrfs_run_delayed_iputs() and before it called
+	 * btrfs_wait_on_delayed_iputs(), it will hang forever since there is
+	 * no one else to run iputs.
+	 *
+	 * So wait for all ongoing ordered extents to complete and then run
+	 * delayed iputs. This works because once we reach this point no one
+	 * can neither create new ordered extents nor create delayed iputs
+	 * through some other means.
+	 *
+	 * Also note that btrfs_wait_ordered_roots() is not safe here, because
+	 * it waits for BTRFS_ORDERED_COMPLETE to be set on an ordered extent,
+	 * but the delayed iput for the respective inode is made only when doing
+	 * the final btrfs_put_ordered_extent() (which must happen at
+	 * btrfs_finish_ordered_io() when we are unmounting).
+	 */
+	btrfs_flush_workqueue(fs_info->endio_write_workers);
+	/* Ordered extents for free space inodes. */
+	btrfs_flush_workqueue(fs_info->endio_freespace_worker);
+	btrfs_run_delayed_iputs(fs_info);
+
 	cancel_work_sync(&fs_info->async_reclaim_work);
 	cancel_work_sync(&fs_info->async_data_reclaim_work);
 	cancel_work_sync(&fs_info->preempt_reclaim_work);
-- 
2.35.1

[PATCH 3/3] btrfs: remove useless used space increment during space reservation

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At space-info.c:__reserve_bytes(), we increment the 'used' variable, but
then we don't use the variable anymore, making the increment pointless.
The increment became useless with commit 2e294c60497f29 ("btrfs: simplify
the logic in need_preemptive_flushing"), so just remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/space-info.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 477e57ace48d..30a256c8c2ea 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1666,7 +1666,6 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
 				      &space_info->priority_tickets);
 		}
 	} else if (!ret && space_info->flags & BTRFS_BLOCK_GROUP_METADATA) {
-		used += orig_bytes;
 		/*
 		 * We will do the space reservation dance during log replay,
 		 * which means we won't have fs_info->fs_root set, so don't do
-- 
2.35.1

Re: [PATCH 0/3] btrfs: fix a couple of hangs during unmount caused by races

[Josef Bacik]

On Thu, Sep 08, 2022 at 12:31:49PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Due to some races or bad timmings, we can hang during unmount when trying
> to stop the block group reclaim task or one of the space reclaim tasks.
> The second case if often triggered by generic/562 for a while, but the
> underlying problem has been there for a long time, despite seeming to be
> more frequent recently. More details in the changelogs.
> 

You can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the entire series, thanks,

Josef

Re: [PATCH 0/3] btrfs: fix a couple of hangs during unmount caused by races

[David Sterba]

On Thu, Sep 08, 2022 at 12:31:49PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Due to some races or bad timmings, we can hang during unmount when trying
> to stop the block group reclaim task or one of the space reclaim tasks.
> The second case if often triggered by generic/562 for a while, but the
> underlying problem has been there for a long time, despite seeming to be
> more frequent recently. More details in the changelogs.
> 
> Filipe Manana (3):
>   btrfs: fix hang during unmount when stopping block group reclaim worker
>   btrfs: fix hang during unmount when stopping a space reclaim worker
>   btrfs: remove useless used space increment during space reservation

Added to for-next, thanks.

Re: [PATCH 0/3] btrfs: fix a couple of hangs during unmount caused by races

[David Sterba]

On Fri, Sep 09, 2022 at 04:32:54PM +0200, David Sterba wrote:
> On Thu, Sep 08, 2022 at 12:31:49PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > Due to some races or bad timmings, we can hang during unmount when trying
> > to stop the block group reclaim task or one of the space reclaim tasks.
> > The second case if often triggered by generic/562 for a while, but the
> > underlying problem has been there for a long time, despite seeming to be
> > more frequent recently. More details in the changelogs.
> > 
> > Filipe Manana (3):
> >   btrfs: fix hang during unmount when stopping block group reclaim worker
> >   btrfs: fix hang during unmount when stopping a space reclaim worker
> >   btrfs: remove useless used space increment during space reservation
> 
> Added to for-next, thanks.

Moved to misc-next, thanks.