[PATCH v2] Btrfs: fix missing delayed iputs on unmount

[PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

There's a race between close_ctree() and cleaner_kthread().
close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
sees it set, but this is racy; the cleaner might have already checked
the bit and could be cleaning stuff. In particular, if it deletes unused
block groups, it will create delayed iputs for the free space cache
inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
longer running delayed iputs after a commit. Therefore, if the cleaner
creates more delayed iputs after delayed iputs are run in
btrfs_commit_super(), we will leak inodes on unmount and get a busy
inode crash from the VFS.

Fix it by parking the cleaner before we actually close anything. Then,
any remaining delayed iputs will always be handled in
btrfs_commit_super(). This also ensures that the commit in close_ctree()
is really the last commit, so we can get rid of the commit in
cleaner_kthread().

Fixes: 30928e9baac2 ("btrfs: don't run delayed_iputs in commit")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
Changes from v1:

- Add a comment explaining why it needs to be a kthread_park(), not
  kthread_stop()
- Update later comment now that the cleaner thread is definitely stopped

 fs/btrfs/disk-io.c | 51 ++++++++++++++--------------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b0ab41da91d1..40bcc45d827d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1664,9 +1664,8 @@ static int cleaner_kthread(void *arg)
 	struct btrfs_root *root = arg;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	int again;
-	struct btrfs_trans_handle *trans;
 
-	do {
+	while (1) {
 		again = 0;
 
 		/* Make the cleaner go to sleep early. */
@@ -1715,42 +1714,16 @@ static int cleaner_kthread(void *arg)
 		 */
 		btrfs_delete_unused_bgs(fs_info);
 sleep:
+		if (kthread_should_park())
+			kthread_parkme();
+		if (kthread_should_stop())
+			return 0;
 		if (!again) {
 			set_current_state(TASK_INTERRUPTIBLE);
-			if (!kthread_should_stop())
-				schedule();
+			schedule();
 			__set_current_state(TASK_RUNNING);
 		}
-	} while (!kthread_should_stop());
-
-	/*
-	 * Transaction kthread is stopped before us and wakes us up.
-	 * However we might have started a new transaction and COWed some
-	 * tree blocks when deleting unused block groups for example. So
-	 * make sure we commit the transaction we started to have a clean
-	 * shutdown when evicting the btree inode - if it has dirty pages
-	 * when we do the final iput() on it, eviction will trigger a
-	 * writeback for it which will fail with null pointer dereferences
-	 * since work queues and other resources were already released and
-	 * destroyed by the time the iput/eviction/writeback is made.
-	 */
-	trans = btrfs_attach_transaction(root);
-	if (IS_ERR(trans)) {
-		if (PTR_ERR(trans) != -ENOENT)
-			btrfs_err(fs_info,
-				  "cleaner transaction attach returned %ld",
-				  PTR_ERR(trans));
-	} else {
-		int ret;
-
-		ret = btrfs_commit_transaction(trans);
-		if (ret)
-			btrfs_err(fs_info,
-				  "cleaner open transaction commit returned %d",
-				  ret);
 	}
-
-	return 0;
 }
 
 static int transaction_kthread(void *arg)
@@ -3931,6 +3904,13 @@ void close_ctree(struct btrfs_fs_info *fs_info)
 	int ret;
 
 	set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
+	/*
+	 * We don't want the cleaner to start new transactions, add more delayed
+	 * iputs, etc. while we're closing. We can't use kthread_stop() yet
+	 * because that frees the task_struct, and the transaction kthread might
+	 * still try to wake up the cleaner.
+	 */
+	kthread_park(fs_info->cleaner_kthread);
 
 	/* wait for the qgroup rescan worker to stop */
 	btrfs_qgroup_wait_for_completion(fs_info, false);
@@ -3958,9 +3938,8 @@ void close_ctree(struct btrfs_fs_info *fs_info)
 
 	if (!sb_rdonly(fs_info->sb)) {
 		/*
-		 * If the cleaner thread is stopped and there are
-		 * block groups queued for removal, the deletion will be
-		 * skipped when we quit the cleaner thread.
+		 * The cleaner kthread is stopped, so do one final pass over
+		 * unused block groups.
 		 */
 		btrfs_delete_unused_bgs(fs_info);
 
-- 
2.19.1

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> There's a race between close_ctree() and cleaner_kthread().
> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> sees it set, but this is racy; the cleaner might have already checked
> the bit and could be cleaning stuff. In particular, if it deletes unused
> block groups, it will create delayed iputs for the free space cache
> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> longer running delayed iputs after a commit. Therefore, if the cleaner
> creates more delayed iputs after delayed iputs are run in
> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> inode crash from the VFS.
> 
> Fix it by parking the cleaner

Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
we're missing a commit or clean up data structures. Messing with state
of a thread would be the last thing I'd try after proving that it's not
possible to fix in the logic of btrfs itself.

The shutdown sequence in close_tree is quite tricky and we've had bugs
there. The interdependencies of thread and data structures and other
subsystems cannot have loops that could not find an ordering that will
not leak something.

It's not a big problem if some step is done more than once, like
committing or cleaning up some other structures if we know that
it could create new.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Chris Mason]

On 1 Nov 2018, at 6:15, David Sterba wrote:

> On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
>> From: Omar Sandoval <osandov@fb.com>
>>
>> There's a race between close_ctree() and cleaner_kthread().
>> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
>> sees it set, but this is racy; the cleaner might have already checked
>> the bit and could be cleaning stuff. In particular, if it deletes 
>> unused
>> block groups, it will create delayed iputs for the free space cache
>> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
>> longer running delayed iputs after a commit. Therefore, if the 
>> cleaner
>> creates more delayed iputs after delayed iputs are run in
>> btrfs_commit_super(), we will leak inodes on unmount and get a busy
>> inode crash from the VFS.
>>
>> Fix it by parking the cleaner
>
> Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> we're missing a commit or clean up data structures. Messing with state
> of a thread would be the last thing I'd try after proving that it's 
> not
> possible to fix in the logic of btrfs itself.
>
> The shutdown sequence in close_tree is quite tricky and we've had bugs
> there. The interdependencies of thread and data structures and other
> subsystems cannot have loops that could not find an ordering that will
> not leak something.
>
> It's not a big problem if some step is done more than once, like
> committing or cleaning up some other structures if we know that
> it could create new.

The problem is the cleaner thread needs to be told to stop doing new 
work, and we need to wait for the work it's already doing to be 
finished.  We're getting "stop doing new work" already because the 
cleaner thread checks to see if the FS is closing, but we don't have a 
way today to wait for him to finish what he's already doing.

kthread_park() is basically the same as adding another mutex or 
synchronization point.  I'm not sure how we could change close_tree() or 
the final commit to pick this up more effectively?

-chris

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> On 1 Nov 2018, at 6:15, David Sterba wrote:
> 
> > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> >> From: Omar Sandoval <osandov@fb.com>
> >>
> >> There's a race between close_ctree() and cleaner_kthread().
> >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> >> sees it set, but this is racy; the cleaner might have already checked
> >> the bit and could be cleaning stuff. In particular, if it deletes 
> >> unused
> >> block groups, it will create delayed iputs for the free space cache
> >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> >> longer running delayed iputs after a commit. Therefore, if the 
> >> cleaner
> >> creates more delayed iputs after delayed iputs are run in
> >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> >> inode crash from the VFS.
> >>
> >> Fix it by parking the cleaner
> >
> > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > we're missing a commit or clean up data structures. Messing with state
> > of a thread would be the last thing I'd try after proving that it's 
> > not
> > possible to fix in the logic of btrfs itself.
> >
> > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > there. The interdependencies of thread and data structures and other
> > subsystems cannot have loops that could not find an ordering that will
> > not leak something.
> >
> > It's not a big problem if some step is done more than once, like
> > committing or cleaning up some other structures if we know that
> > it could create new.
> 
> The problem is the cleaner thread needs to be told to stop doing new 
> work, and we need to wait for the work it's already doing to be 
> finished.  We're getting "stop doing new work" already because the 
> cleaner thread checks to see if the FS is closing, but we don't have a 
> way today to wait for him to finish what he's already doing.
> 
> kthread_park() is basically the same as adding another mutex or 
> synchronization point.  I'm not sure how we could change close_tree() or 
> the final commit to pick this up more effectively?

The idea is:

cleaner                                     close_ctree thread

                                            tell cleaner to stop
					    wait
start work
if should_stop, then exit
                                            cleaner is stopped

[does not run: finish work]
[does not run: loop]
                                            pick up the work or make
					    sure there's nothing in
					    progress anymore


A simplified version in code:

  set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);

  wait for defrag - could be started from cleaner but next iteration will
		    see the fs closed and will not continue

  kthread_stop(transaction_kthread)

  kthread_stop(cleaner_kthread)

  /* next, everything that could be left from cleaner should be finished */

  btrfs_delete_unused_bgs();
  assert there are no defrags
  assert there are no delayed iputs
  commit if necessary

IOW the unused block groups are removed from close_ctree too early,
moving that after the threads stop AND makins sure that it does not need
either of them should work.

The "AND" above is not currently implemented as btrfs_delete_unused_bgs
calls plain btrfs_end_transaction that wakes up transaction ktread, so
there would need to be an argument passed to tell it to do full commit.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > 
> > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > >> From: Omar Sandoval <osandov@fb.com>
> > >>
> > >> There's a race between close_ctree() and cleaner_kthread().
> > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > >> sees it set, but this is racy; the cleaner might have already checked
> > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > >> unused
> > >> block groups, it will create delayed iputs for the free space cache
> > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > >> longer running delayed iputs after a commit. Therefore, if the 
> > >> cleaner
> > >> creates more delayed iputs after delayed iputs are run in
> > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > >> inode crash from the VFS.
> > >>
> > >> Fix it by parking the cleaner
> > >
> > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > we're missing a commit or clean up data structures. Messing with state
> > > of a thread would be the last thing I'd try after proving that it's 
> > > not
> > > possible to fix in the logic of btrfs itself.
> > >
> > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > there. The interdependencies of thread and data structures and other
> > > subsystems cannot have loops that could not find an ordering that will
> > > not leak something.
> > >
> > > It's not a big problem if some step is done more than once, like
> > > committing or cleaning up some other structures if we know that
> > > it could create new.
> > 
> > The problem is the cleaner thread needs to be told to stop doing new 
> > work, and we need to wait for the work it's already doing to be 
> > finished.  We're getting "stop doing new work" already because the 
> > cleaner thread checks to see if the FS is closing, but we don't have a 
> > way today to wait for him to finish what he's already doing.
> > 
> > kthread_park() is basically the same as adding another mutex or 
> > synchronization point.  I'm not sure how we could change close_tree() or 
> > the final commit to pick this up more effectively?
> 
> The idea is:
> 
> cleaner                                     close_ctree thread
> 
>                                             tell cleaner to stop
> 					    wait
> start work
> if should_stop, then exit
>                                             cleaner is stopped
> 
> [does not run: finish work]
> [does not run: loop]
>                                             pick up the work or make
> 					    sure there's nothing in
> 					    progress anymore
> 
> 
> A simplified version in code:
> 
>   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> 
>   wait for defrag - could be started from cleaner but next iteration will
> 		    see the fs closed and will not continue
> 
>   kthread_stop(transaction_kthread)
> 
>   kthread_stop(cleaner_kthread)
> 
>   /* next, everything that could be left from cleaner should be finished */
> 
>   btrfs_delete_unused_bgs();
>   assert there are no defrags
>   assert there are no delayed iputs
>   commit if necessary
> 
> IOW the unused block groups are removed from close_ctree too early,
> moving that after the threads stop AND makins sure that it does not need
> either of them should work.
> 
> The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> calls plain btrfs_end_transaction that wakes up transaction ktread, so
> there would need to be an argument passed to tell it to do full commit.

Not perfect, relies on the fact that wake_up_process(thread) on a stopped
thread is a no-op, arguments would need to be added to skip that in
btrfs_delete_unused_bgs and btrfs_commit_super.

--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3956,11 +3956,18 @@ void close_ctree(struct btrfs_fs_info *fs_info)
 
        cancel_work_sync(&fs_info->async_reclaim_work);
 
+       if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state) ||
+           test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state))
+               btrfs_error_commit_super(fs_info);
+
+       kthread_stop(fs_info->transaction_kthread);
+       kthread_stop(fs_info->cleaner_kthread);
+
        if (!sb_rdonly(fs_info->sb)) {
                /*
-                * If the cleaner thread is stopped and there are
-                * block groups queued for removal, the deletion will be
-                * skipped when we quit the cleaner thread.
+                * The cleaner thread is now stopped and if there are block
+                * groups queued for removal, we have to pick up the work here
+                * so there are no delayed iputs triggered.
                 */
                btrfs_delete_unused_bgs(fs_info);
 
@@ -3969,13 +3976,6 @@ void close_ctree(struct btrfs_fs_info *fs_info)
                        btrfs_err(fs_info, "commit super ret %d", ret);
        }
 
-       if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state) ||
-           test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state))
-               btrfs_error_commit_super(fs_info);
-
-       kthread_stop(fs_info->transaction_kthread);
-       kthread_stop(fs_info->cleaner_kthread);
-
        ASSERT(list_empty(&fs_info->delayed_iputs));
        set_bit(BTRFS_FS_CLOSING_DONE, &fs_info->flags);

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Omar Sandoval]

On Thu, Nov 01, 2018 at 04:22:29PM +0100, David Sterba wrote:
> On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> > On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > > 
> > > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > > >> From: Omar Sandoval <osandov@fb.com>
> > > >>
> > > >> There's a race between close_ctree() and cleaner_kthread().
> > > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > > >> sees it set, but this is racy; the cleaner might have already checked
> > > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > > >> unused
> > > >> block groups, it will create delayed iputs for the free space cache
> > > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > > >> longer running delayed iputs after a commit. Therefore, if the 
> > > >> cleaner
> > > >> creates more delayed iputs after delayed iputs are run in
> > > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > > >> inode crash from the VFS.
> > > >>
> > > >> Fix it by parking the cleaner
> > > >
> > > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > > we're missing a commit or clean up data structures. Messing with state
> > > > of a thread would be the last thing I'd try after proving that it's 
> > > > not
> > > > possible to fix in the logic of btrfs itself.
> > > >
> > > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > > there. The interdependencies of thread and data structures and other
> > > > subsystems cannot have loops that could not find an ordering that will
> > > > not leak something.
> > > >
> > > > It's not a big problem if some step is done more than once, like
> > > > committing or cleaning up some other structures if we know that
> > > > it could create new.
> > > 
> > > The problem is the cleaner thread needs to be told to stop doing new 
> > > work, and we need to wait for the work it's already doing to be 
> > > finished.  We're getting "stop doing new work" already because the 
> > > cleaner thread checks to see if the FS is closing, but we don't have a 
> > > way today to wait for him to finish what he's already doing.
> > > 
> > > kthread_park() is basically the same as adding another mutex or 
> > > synchronization point.  I'm not sure how we could change close_tree() or 
> > > the final commit to pick this up more effectively?
> > 
> > The idea is:
> > 
> > cleaner                                     close_ctree thread
> > 
> >                                             tell cleaner to stop
> > 					    wait
> > start work
> > if should_stop, then exit
> >                                             cleaner is stopped
> > 
> > [does not run: finish work]
> > [does not run: loop]
> >                                             pick up the work or make
> > 					    sure there's nothing in
> > 					    progress anymore
> > 
> > 
> > A simplified version in code:
> > 
> >   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> > 
> >   wait for defrag - could be started from cleaner but next iteration will
> > 		    see the fs closed and will not continue
> > 
> >   kthread_stop(transaction_kthread)
> > 
> >   kthread_stop(cleaner_kthread)
> > 
> >   /* next, everything that could be left from cleaner should be finished */
> > 
> >   btrfs_delete_unused_bgs();
> >   assert there are no defrags
> >   assert there are no delayed iputs
> >   commit if necessary
> > 
> > IOW the unused block groups are removed from close_ctree too early,
> > moving that after the threads stop AND makins sure that it does not need
> > either of them should work.
> > 
> > The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> > calls plain btrfs_end_transaction that wakes up transaction ktread, so
> > there would need to be an argument passed to tell it to do full commit.
> 
> Not perfect, relies on the fact that wake_up_process(thread) on a stopped
> thread is a no-op,

How is that? kthread_stop() frees the task struct, so wake_up_process()
would be a use-after-free.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Omar Sandoval]

On Thu, Nov 01, 2018 at 08:24:25AM -0700, Omar Sandoval wrote:
> On Thu, Nov 01, 2018 at 04:22:29PM +0100, David Sterba wrote:
> > On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> > > On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > > > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > > > 
> > > > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > > > >> From: Omar Sandoval <osandov@fb.com>
> > > > >>
> > > > >> There's a race between close_ctree() and cleaner_kthread().
> > > > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > > > >> sees it set, but this is racy; the cleaner might have already checked
> > > > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > > > >> unused
> > > > >> block groups, it will create delayed iputs for the free space cache
> > > > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > > > >> longer running delayed iputs after a commit. Therefore, if the 
> > > > >> cleaner
> > > > >> creates more delayed iputs after delayed iputs are run in
> > > > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > > > >> inode crash from the VFS.
> > > > >>
> > > > >> Fix it by parking the cleaner
> > > > >
> > > > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > > > we're missing a commit or clean up data structures. Messing with state
> > > > > of a thread would be the last thing I'd try after proving that it's 
> > > > > not
> > > > > possible to fix in the logic of btrfs itself.
> > > > >
> > > > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > > > there. The interdependencies of thread and data structures and other
> > > > > subsystems cannot have loops that could not find an ordering that will
> > > > > not leak something.
> > > > >
> > > > > It's not a big problem if some step is done more than once, like
> > > > > committing or cleaning up some other structures if we know that
> > > > > it could create new.
> > > > 
> > > > The problem is the cleaner thread needs to be told to stop doing new 
> > > > work, and we need to wait for the work it's already doing to be 
> > > > finished.  We're getting "stop doing new work" already because the 
> > > > cleaner thread checks to see if the FS is closing, but we don't have a 
> > > > way today to wait for him to finish what he's already doing.
> > > > 
> > > > kthread_park() is basically the same as adding another mutex or 
> > > > synchronization point.  I'm not sure how we could change close_tree() or 
> > > > the final commit to pick this up more effectively?
> > > 
> > > The idea is:
> > > 
> > > cleaner                                     close_ctree thread
> > > 
> > >                                             tell cleaner to stop
> > > 					    wait
> > > start work
> > > if should_stop, then exit
> > >                                             cleaner is stopped
> > > 
> > > [does not run: finish work]
> > > [does not run: loop]
> > >                                             pick up the work or make
> > > 					    sure there's nothing in
> > > 					    progress anymore
> > > 
> > > 
> > > A simplified version in code:
> > > 
> > >   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> > > 
> > >   wait for defrag - could be started from cleaner but next iteration will
> > > 		    see the fs closed and will not continue
> > > 
> > >   kthread_stop(transaction_kthread)
> > > 
> > >   kthread_stop(cleaner_kthread)
> > > 
> > >   /* next, everything that could be left from cleaner should be finished */
> > > 
> > >   btrfs_delete_unused_bgs();
> > >   assert there are no defrags
> > >   assert there are no delayed iputs
> > >   commit if necessary
> > > 
> > > IOW the unused block groups are removed from close_ctree too early,
> > > moving that after the threads stop AND makins sure that it does not need
> > > either of them should work.
> > > 
> > > The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> > > calls plain btrfs_end_transaction that wakes up transaction ktread, so
> > > there would need to be an argument passed to tell it to do full commit.
> > 
> > Not perfect, relies on the fact that wake_up_process(thread) on a stopped
> > thread is a no-op,
> 
> How is that? kthread_stop() frees the task struct, so wake_up_process()
> would be a use-after-free.

(Indirectly, through the kthread calling do_exit() -> do_task_dead() ->
being put in the scheduler)

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Thu, Nov 01, 2018 at 08:24:25AM -0700, Omar Sandoval wrote:
> On Thu, Nov 01, 2018 at 04:22:29PM +0100, David Sterba wrote:
> > On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> > > On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > > > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > > > 
> > > > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > > > >> From: Omar Sandoval <osandov@fb.com>
> > > > >>
> > > > >> There's a race between close_ctree() and cleaner_kthread().
> > > > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > > > >> sees it set, but this is racy; the cleaner might have already checked
> > > > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > > > >> unused
> > > > >> block groups, it will create delayed iputs for the free space cache
> > > > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > > > >> longer running delayed iputs after a commit. Therefore, if the 
> > > > >> cleaner
> > > > >> creates more delayed iputs after delayed iputs are run in
> > > > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > > > >> inode crash from the VFS.
> > > > >>
> > > > >> Fix it by parking the cleaner
> > > > >
> > > > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > > > we're missing a commit or clean up data structures. Messing with state
> > > > > of a thread would be the last thing I'd try after proving that it's 
> > > > > not
> > > > > possible to fix in the logic of btrfs itself.
> > > > >
> > > > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > > > there. The interdependencies of thread and data structures and other
> > > > > subsystems cannot have loops that could not find an ordering that will
> > > > > not leak something.
> > > > >
> > > > > It's not a big problem if some step is done more than once, like
> > > > > committing or cleaning up some other structures if we know that
> > > > > it could create new.
> > > > 
> > > > The problem is the cleaner thread needs to be told to stop doing new 
> > > > work, and we need to wait for the work it's already doing to be 
> > > > finished.  We're getting "stop doing new work" already because the 
> > > > cleaner thread checks to see if the FS is closing, but we don't have a 
> > > > way today to wait for him to finish what he's already doing.
> > > > 
> > > > kthread_park() is basically the same as adding another mutex or 
> > > > synchronization point.  I'm not sure how we could change close_tree() or 
> > > > the final commit to pick this up more effectively?
> > > 
> > > The idea is:
> > > 
> > > cleaner                                     close_ctree thread
> > > 
> > >                                             tell cleaner to stop
> > > 					    wait
> > > start work
> > > if should_stop, then exit
> > >                                             cleaner is stopped
> > > 
> > > [does not run: finish work]
> > > [does not run: loop]
> > >                                             pick up the work or make
> > > 					    sure there's nothing in
> > > 					    progress anymore
> > > 
> > > 
> > > A simplified version in code:
> > > 
> > >   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> > > 
> > >   wait for defrag - could be started from cleaner but next iteration will
> > > 		    see the fs closed and will not continue
> > > 
> > >   kthread_stop(transaction_kthread)
> > > 
> > >   kthread_stop(cleaner_kthread)
> > > 
> > >   /* next, everything that could be left from cleaner should be finished */
> > > 
> > >   btrfs_delete_unused_bgs();
> > >   assert there are no defrags
> > >   assert there are no delayed iputs
> > >   commit if necessary
> > > 
> > > IOW the unused block groups are removed from close_ctree too early,
> > > moving that after the threads stop AND makins sure that it does not need
> > > either of them should work.
> > > 
> > > The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> > > calls plain btrfs_end_transaction that wakes up transaction ktread, so
> > > there would need to be an argument passed to tell it to do full commit.
> > 
> > Not perfect, relies on the fact that wake_up_process(thread) on a stopped
> > thread is a no-op,
> 
> How is that? kthread_stop() frees the task struct, so wake_up_process()
> would be a use-after-free.

That was an assumption for the demonstration purposes, the wording was
confusing sorry.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Omar Sandoval]

On Thu, Nov 01, 2018 at 04:29:48PM +0100, David Sterba wrote:
> On Thu, Nov 01, 2018 at 08:24:25AM -0700, Omar Sandoval wrote:
> > On Thu, Nov 01, 2018 at 04:22:29PM +0100, David Sterba wrote:
> > > On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> > > > On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > > > > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > > > > 
> > > > > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > > > > >> From: Omar Sandoval <osandov@fb.com>
> > > > > >>
> > > > > >> There's a race between close_ctree() and cleaner_kthread().
> > > > > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > > > > >> sees it set, but this is racy; the cleaner might have already checked
> > > > > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > > > > >> unused
> > > > > >> block groups, it will create delayed iputs for the free space cache
> > > > > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > > > > >> longer running delayed iputs after a commit. Therefore, if the 
> > > > > >> cleaner
> > > > > >> creates more delayed iputs after delayed iputs are run in
> > > > > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > > > > >> inode crash from the VFS.
> > > > > >>
> > > > > >> Fix it by parking the cleaner
> > > > > >
> > > > > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > > > > we're missing a commit or clean up data structures. Messing with state
> > > > > > of a thread would be the last thing I'd try after proving that it's 
> > > > > > not
> > > > > > possible to fix in the logic of btrfs itself.
> > > > > >
> > > > > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > > > > there. The interdependencies of thread and data structures and other
> > > > > > subsystems cannot have loops that could not find an ordering that will
> > > > > > not leak something.
> > > > > >
> > > > > > It's not a big problem if some step is done more than once, like
> > > > > > committing or cleaning up some other structures if we know that
> > > > > > it could create new.
> > > > > 
> > > > > The problem is the cleaner thread needs to be told to stop doing new 
> > > > > work, and we need to wait for the work it's already doing to be 
> > > > > finished.  We're getting "stop doing new work" already because the 
> > > > > cleaner thread checks to see if the FS is closing, but we don't have a 
> > > > > way today to wait for him to finish what he's already doing.
> > > > > 
> > > > > kthread_park() is basically the same as adding another mutex or 
> > > > > synchronization point.  I'm not sure how we could change close_tree() or 
> > > > > the final commit to pick this up more effectively?
> > > > 
> > > > The idea is:
> > > > 
> > > > cleaner                                     close_ctree thread
> > > > 
> > > >                                             tell cleaner to stop
> > > > 					    wait
> > > > start work
> > > > if should_stop, then exit
> > > >                                             cleaner is stopped
> > > > 
> > > > [does not run: finish work]
> > > > [does not run: loop]
> > > >                                             pick up the work or make
> > > > 					    sure there's nothing in
> > > > 					    progress anymore
> > > > 
> > > > 
> > > > A simplified version in code:
> > > > 
> > > >   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> > > > 
> > > >   wait for defrag - could be started from cleaner but next iteration will
> > > > 		    see the fs closed and will not continue
> > > > 
> > > >   kthread_stop(transaction_kthread)
> > > > 
> > > >   kthread_stop(cleaner_kthread)
> > > > 
> > > >   /* next, everything that could be left from cleaner should be finished */
> > > > 
> > > >   btrfs_delete_unused_bgs();
> > > >   assert there are no defrags
> > > >   assert there are no delayed iputs
> > > >   commit if necessary
> > > > 
> > > > IOW the unused block groups are removed from close_ctree too early,
> > > > moving that after the threads stop AND makins sure that it does not need
> > > > either of them should work.
> > > > 
> > > > The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> > > > calls plain btrfs_end_transaction that wakes up transaction ktread, so
> > > > there would need to be an argument passed to tell it to do full commit.
> > > 
> > > Not perfect, relies on the fact that wake_up_process(thread) on a stopped
> > > thread is a no-op,
> > 
> > How is that? kthread_stop() frees the task struct, so wake_up_process()
> > would be a use-after-free.
> 
> That was an assumption for the demonstration purposes, the wording was
> confusing sorry.

Oh, well in that case, that's exactly what kthread_park() is ;) Stop the
thread and make wake_up a noop, and then we don't need to add special
cases everywhere else.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Thu, Nov 01, 2018 at 09:00:56AM -0700, Omar Sandoval wrote:
> > That was an assumption for the demonstration purposes, the wording was
> > confusing sorry.
> 
> Oh, well in that case, that's exactly what kthread_park() is ;) Stop the
> thread and make wake_up a noop, and then we don't need to add special
> cases everywhere else.

The end result is equivalent and should be, I'm now weighing both
approaches. Explicit state tracking outside of the thread structures
seems more clear and in case we want to query the status, we can't after
kthread_stop is called. My current version below.

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 68ca41dbbef3..cc076ae45ced 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -715,6 +715,7 @@ struct btrfs_delayed_root;
 #define BTRFS_FS_BARRIER			1
 #define BTRFS_FS_CLOSING_START			2
 #define BTRFS_FS_CLOSING_DONE			3
+#define BTRFS_FS_CLOSING_SINGLE			19
 #define BTRFS_FS_LOG_RECOVERING			4
 #define BTRFS_FS_OPEN				5
 #define BTRFS_FS_QUOTA_ENABLED			6
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b0ab41da91d1..570096fb3f35 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1801,7 +1801,7 @@ static int transaction_kthread(void *arg)
 			btrfs_end_transaction(trans);
 		}
 sleep:
-		wake_up_process(fs_info->cleaner_kthread);
+		btrfs_wake_up_cleaner(fs_info);
 		mutex_unlock(&fs_info->transaction_kthread_mutex);
 
 		if (unlikely(test_bit(BTRFS_FS_STATE_ERROR,
@@ -3914,7 +3914,7 @@ int btrfs_commit_super(struct btrfs_fs_info *fs_info)
 	mutex_lock(&fs_info->cleaner_mutex);
 	btrfs_run_delayed_iputs(fs_info);
 	mutex_unlock(&fs_info->cleaner_mutex);
-	wake_up_process(fs_info->cleaner_kthread);
+	btrfs_wake_up_cleaner(fs_info);
 
 	/* wait until ongoing cleanup work done */
 	down_write(&fs_info->cleanup_work_sem);
@@ -3956,11 +3956,24 @@ void close_ctree(struct btrfs_fs_info *fs_info)
 
 	cancel_work_sync(&fs_info->async_reclaim_work);
 
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state) ||
+	    test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state))
+		btrfs_error_commit_super(fs_info);
+
+	kthread_stop(fs_info->transaction_kthread);
+	kthread_stop(fs_info->cleaner_kthread);
+
+	/*
+	 * btrfs_delete_unused_bgs or btrfs_commit_super can still try to wake
+	 * up the threads but will become a no-op
+	 */
+	set_bit(BTRFS_FS_CLOSING_SINGLE, &fs_info->flags);
+
 	if (!sb_rdonly(fs_info->sb)) {
 		/*
-		 * If the cleaner thread is stopped and there are
-		 * block groups queued for removal, the deletion will be
-		 * skipped when we quit the cleaner thread.
+		 * The cleaner thread is now stopped and if there are block
+		 * groups queued for removal, we have to pick up the work here
+		 * so there are no delayed iputs triggered.
 		 */
 		btrfs_delete_unused_bgs(fs_info);
 
@@ -3969,13 +3982,6 @@ void close_ctree(struct btrfs_fs_info *fs_info)
 			btrfs_err(fs_info, "commit super ret %d", ret);
 	}
 
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state) ||
-	    test_bit(BTRFS_FS_STATE_TRANS_ABORTED, &fs_info->fs_state))
-		btrfs_error_commit_super(fs_info);
-
-	kthread_stop(fs_info->transaction_kthread);
-	kthread_stop(fs_info->cleaner_kthread);
-
 	ASSERT(list_empty(&fs_info->delayed_iputs));
 	set_bit(BTRFS_FS_CLOSING_DONE, &fs_info->flags);
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a990a9045139..a5786b6e8d37 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5915,7 +5915,7 @@ long btrfs_ioctl(struct file *file, unsigned int
 		 * namely it pokes the cleaner kthread that will start
 		 * processing uncleaned subvols.
 		 */
-		wake_up_process(fs_info->transaction_kthread);
+		btrfs_wake_up_transaction(fs_info);
 		return ret;
 	}
 	case BTRFS_IOC_START_SYNC:
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b362b45dd757..9e73e35b94ea 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1896,7 +1896,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		set_bit(BTRFS_FS_OPEN, &fs_info->flags);
 	}
 out:
-	wake_up_process(fs_info->transaction_kthread);
+	btrfs_wake_up_transaction(fs_info);
 	btrfs_remount_cleanup(fs_info, old_opts);
 	return 0;
 
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 3717c864ba23..d00311dc76d2 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -154,7 +154,7 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
 	 * We don't want to do full transaction commit from inside sysfs
 	 */
 	btrfs_set_pending(fs_info, COMMIT);
-	wake_up_process(fs_info->transaction_kthread);
+	btrfs_wake_up_transaction(fs_info);
 
 	return count;
 }
@@ -427,7 +427,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	 * We don't want to do full transaction commit from inside sysfs
 	 */
 	btrfs_set_pending(fs_info, COMMIT);
-	wake_up_process(fs_info->transaction_kthread);
+	btrfs_wake_up_transaction(fs_info);
 
 	return len;
 }
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index d1eeef9ec5da..3ffed32c680e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -867,7 +867,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 		if (throttle)
 			return btrfs_commit_transaction(trans);
 		else
-			wake_up_process(info->transaction_kthread);
+			btrfs_wake_up_transaction(info);
 	}
 
 	if (trans->type & __TRANS_FREEZABLE)
@@ -889,7 +889,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 
 	if (trans->aborted ||
 	    test_bit(BTRFS_FS_STATE_ERROR, &info->fs_state)) {
-		wake_up_process(info->transaction_kthread);
+		btrfs_wake_up_transaction(info);
 		err = -EIO;
 	}
 
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 4cbb1b55387d..64a38c4af565 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -199,6 +199,25 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans);
 int btrfs_commit_transaction_async(struct btrfs_trans_handle *trans,
 				   int wait_for_unblock);
 
+static inline void btrfs_wake_up_cleaner(struct btrfs_fs_info *fs_info)
+{
+	if (!test_bit(BTRFS_FS_CLOSING_SINGLE, &fs_info->flags)) {
+		wake_up_process(fs_info->cleaner_kthread);
+	} else {
+		btrfs_debug(fs_info,
+			"attempt to wake up cleaner kthread during shutdown");
+	}
+}
+static inline void btrfs_wake_up_transaction(struct btrfs_fs_info *fs_info)
+{
+	if (!test_bit(BTRFS_FS_CLOSING_SINGLE, &fs_info->flags)) {
+		wake_up_process(fs_info->transaction_kthread);
+	} else {
+		btrfs_debug(fs_info,
+			"attempt to wake up transaction kthread during shutdown");
+	}
+}
+
 /*
  * Try to commit transaction asynchronously, so this is safe to call
  * even holding a spinlock.
@@ -210,7 +229,7 @@ static inline void btrfs_commit_transaction_locksafe(
 		struct btrfs_fs_info *fs_info)
 {
 	set_bit(BTRFS_FS_NEED_ASYNC_COMMIT, &fs_info->flags);
-	wake_up_process(fs_info->transaction_kthread);
+	btrfs_wake_up_transaction(fs_info);
 }
 int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans);
 int btrfs_should_end_transaction(struct btrfs_trans_handle *trans);
-- 
2.19.1

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Nikolay Borisov]

On 1.11.18 г. 18:44 ч., David Sterba wrote:
> On Thu, Nov 01, 2018 at 09:00:56AM -0700, Omar Sandoval wrote:
>>> That was an assumption for the demonstration purposes, the wording was
>>> confusing sorry.
>>
>> Oh, well in that case, that's exactly what kthread_park() is ;) Stop the
>> thread and make wake_up a noop, and then we don't need to add special
>> cases everywhere else.
> 
> The end result is equivalent and should be, I'm now weighing both
> approaches. Explicit state tracking outside of the thread structures
> seems more clear and in case we want to query the status, we can't after
> kthread_stop is called. My current version below.
> 

<snip>

> +static inline void btrfs_wake_up_transaction(struct btrfs_fs_info *fs_info)
> +{
> +	if (!test_bit(BTRFS_FS_CLOSING_SINGLE, &fs_info->flags)) {

Why do we need a separate flag for a predicate? If we use
FS_CLOSING_START then waking up becomes a noop from the moment
close_ctree is called.

> +		wake_up_process(fs_info->transaction_kthread);
> +	} else {
> +		btrfs_debug(fs_info,
> +			"attempt to wake up transaction kthread during shutdown");
> +	}
> +}
> +
>  /*
>   * Try to commit transaction asynchronously, so this is safe to call
>   * even holding a spinlock.
> @@ -210,7 +229,7 @@ static inline void btrfs_commit_transaction_locksafe(
>  		struct btrfs_fs_info *fs_info)
>  {
>  	set_bit(BTRFS_FS_NEED_ASYNC_COMMIT, &fs_info->flags);
> -	wake_up_process(fs_info->transaction_kthread);
> +	btrfs_wake_up_transaction(fs_info);
>  }
>  int btrfs_end_transaction_throttle(struct btrfs_trans_handle *trans);
>  int btrfs_should_end_transaction(struct btrfs_trans_handle *trans);
> 

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Thu, Nov 01, 2018 at 06:50:03PM +0200, Nikolay Borisov wrote:
> 
> 
> On 1.11.18 г. 18:44 ч., David Sterba wrote:
> > On Thu, Nov 01, 2018 at 09:00:56AM -0700, Omar Sandoval wrote:
> >>> That was an assumption for the demonstration purposes, the wording was
> >>> confusing sorry.
> >>
> >> Oh, well in that case, that's exactly what kthread_park() is ;) Stop the
> >> thread and make wake_up a noop, and then we don't need to add special
> >> cases everywhere else.
> > 
> > The end result is equivalent and should be, I'm now weighing both
> > approaches. Explicit state tracking outside of the thread structures
> > seems more clear and in case we want to query the status, we can't after
> > kthread_stop is called. My current version below.
> > 
> 
> <snip>
> 
> > +static inline void btrfs_wake_up_transaction(struct btrfs_fs_info *fs_info)
> > +{
> > +	if (!test_bit(BTRFS_FS_CLOSING_SINGLE, &fs_info->flags)) {
> 
> Why do we need a separate flag for a predicate? If we use
> FS_CLOSING_START then waking up becomes a noop from the moment
> close_ctree is called.

Yeah it's not strictly necessary, it's a leftover from when I had a
WARN_ON in the wakeup helpers that triggered too often so another flag
was added just after the threads were stopped.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Chris Mason]

On 1 Nov 2018, at 12:00, Omar Sandoval wrote:

> On Thu, Nov 01, 2018 at 04:29:48PM +0100, David Sterba wrote:
>>>
>>> How is that? kthread_stop() frees the task struct, so 
>>> wake_up_process()
>>> would be a use-after-free.
>>
>> That was an assumption for the demonstration purposes, the wording 
>> was
>> confusing sorry.
>
> Oh, well in that case, that's exactly what kthread_park() is ;) Stop 
> the
> thread and make wake_up a noop, and then we don't need to add special
> cases everywhere else.

This is how Omar won me over to kthread_park().  I think Dave's 
alternatives can be made correct, but Omar's way solves a whole class of 
potential problems.  The park() makes the thread safely ignore future 
work, and won't don't have to chase down weird bugs where people are 
sending future work at the wrong time.

-chris

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Omar Sandoval]

On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > 
> > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > >> From: Omar Sandoval <osandov@fb.com>
> > >>
> > >> There's a race between close_ctree() and cleaner_kthread().
> > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > >> sees it set, but this is racy; the cleaner might have already checked
> > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > >> unused
> > >> block groups, it will create delayed iputs for the free space cache
> > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > >> longer running delayed iputs after a commit. Therefore, if the 
> > >> cleaner
> > >> creates more delayed iputs after delayed iputs are run in
> > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > >> inode crash from the VFS.
> > >>
> > >> Fix it by parking the cleaner
> > >
> > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > we're missing a commit or clean up data structures. Messing with state
> > > of a thread would be the last thing I'd try after proving that it's 
> > > not
> > > possible to fix in the logic of btrfs itself.
> > >
> > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > there. The interdependencies of thread and data structures and other
> > > subsystems cannot have loops that could not find an ordering that will
> > > not leak something.
> > >
> > > It's not a big problem if some step is done more than once, like
> > > committing or cleaning up some other structures if we know that
> > > it could create new.
> > 
> > The problem is the cleaner thread needs to be told to stop doing new 
> > work, and we need to wait for the work it's already doing to be 
> > finished.  We're getting "stop doing new work" already because the 
> > cleaner thread checks to see if the FS is closing, but we don't have a 
> > way today to wait for him to finish what he's already doing.
> > 
> > kthread_park() is basically the same as adding another mutex or 
> > synchronization point.  I'm not sure how we could change close_tree() or 
> > the final commit to pick this up more effectively?
> 
> The idea is:
> 
> cleaner                                     close_ctree thread
> 
>                                             tell cleaner to stop
> 					    wait
> start work
> if should_stop, then exit
>                                             cleaner is stopped
> 
> [does not run: finish work]
> [does not run: loop]
>                                             pick up the work or make
> 					    sure there's nothing in
> 					    progress anymore
> 
> 
> A simplified version in code:
> 
>   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> 
>   wait for defrag - could be started from cleaner but next iteration will
> 		    see the fs closed and will not continue
> 
>   kthread_stop(transaction_kthread)
> 
>   kthread_stop(cleaner_kthread)
> 
>   /* next, everything that could be left from cleaner should be finished */
> 
>   btrfs_delete_unused_bgs();
>   assert there are no defrags
>   assert there are no delayed iputs
>   commit if necessary
> 
> IOW the unused block groups are removed from close_ctree too early,
> moving that after the threads stop AND makins sure that it does not need
> either of them should work.
> 
> The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> calls plain btrfs_end_transaction that wakes up transaction ktread, so
> there would need to be an argument passed to tell it to do full commit.

It's too fragile to run around in the filesystem with these threads
freed. We can probably make it now, but I'm worried that we'll add
another wakeup somewhere and blow up.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Thu, Nov 01, 2018 at 08:23:17AM -0700, Omar Sandoval wrote:
> On Thu, Nov 01, 2018 at 04:08:32PM +0100, David Sterba wrote:
> > On Thu, Nov 01, 2018 at 01:31:18PM +0000, Chris Mason wrote:
> > > On 1 Nov 2018, at 6:15, David Sterba wrote:
> > > 
> > > > On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > > >> From: Omar Sandoval <osandov@fb.com>
> > > >>
> > > >> There's a race between close_ctree() and cleaner_kthread().
> > > >> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > > >> sees it set, but this is racy; the cleaner might have already checked
> > > >> the bit and could be cleaning stuff. In particular, if it deletes 
> > > >> unused
> > > >> block groups, it will create delayed iputs for the free space cache
> > > >> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > > >> longer running delayed iputs after a commit. Therefore, if the 
> > > >> cleaner
> > > >> creates more delayed iputs after delayed iputs are run in
> > > >> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > > >> inode crash from the VFS.
> > > >>
> > > >> Fix it by parking the cleaner
> > > >
> > > > Ouch, that's IMO wrong way to fix it. The bug is on a higher level,
> > > > we're missing a commit or clean up data structures. Messing with state
> > > > of a thread would be the last thing I'd try after proving that it's 
> > > > not
> > > > possible to fix in the logic of btrfs itself.
> > > >
> > > > The shutdown sequence in close_tree is quite tricky and we've had bugs
> > > > there. The interdependencies of thread and data structures and other
> > > > subsystems cannot have loops that could not find an ordering that will
> > > > not leak something.
> > > >
> > > > It's not a big problem if some step is done more than once, like
> > > > committing or cleaning up some other structures if we know that
> > > > it could create new.
> > > 
> > > The problem is the cleaner thread needs to be told to stop doing new 
> > > work, and we need to wait for the work it's already doing to be 
> > > finished.  We're getting "stop doing new work" already because the 
> > > cleaner thread checks to see if the FS is closing, but we don't have a 
> > > way today to wait for him to finish what he's already doing.
> > > 
> > > kthread_park() is basically the same as adding another mutex or 
> > > synchronization point.  I'm not sure how we could change close_tree() or 
> > > the final commit to pick this up more effectively?
> > 
> > The idea is:
> > 
> > cleaner                                     close_ctree thread
> > 
> >                                             tell cleaner to stop
> > 					    wait
> > start work
> > if should_stop, then exit
> >                                             cleaner is stopped
> > 
> > [does not run: finish work]
> > [does not run: loop]
> >                                             pick up the work or make
> > 					    sure there's nothing in
> > 					    progress anymore
> > 
> > 
> > A simplified version in code:
> > 
> >   set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> > 
> >   wait for defrag - could be started from cleaner but next iteration will
> > 		    see the fs closed and will not continue
> > 
> >   kthread_stop(transaction_kthread)
> > 
> >   kthread_stop(cleaner_kthread)
> > 
> >   /* next, everything that could be left from cleaner should be finished */
> > 
> >   btrfs_delete_unused_bgs();
> >   assert there are no defrags
> >   assert there are no delayed iputs
> >   commit if necessary
> > 
> > IOW the unused block groups are removed from close_ctree too early,
> > moving that after the threads stop AND makins sure that it does not need
> > either of them should work.
> > 
> > The "AND" above is not currently implemented as btrfs_delete_unused_bgs
> > calls plain btrfs_end_transaction that wakes up transaction ktread, so
> > there would need to be an argument passed to tell it to do full commit.
> 
> It's too fragile to run around in the filesystem with these threads
> freed. We can probably make it now, but I'm worried that we'll add
> another wakeup somewhere and blow up.

So functions that are called from close_ctree will be careful to either
not wake up or check before trying to wake them up. We can also wrap the
wakeups like

wake_up_cleaner(fs_info) {
	if (!fs_info->state & CLOSING)
		wake_up_process(cleaner_kthread)
	else
		BUG_ON
}

Same for kthread.

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Nikolay Borisov]

On 31.10.18 г. 19:06 ч., Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> There's a race between close_ctree() and cleaner_kthread().
> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> sees it set, but this is racy; the cleaner might have already checked
> the bit and could be cleaning stuff. In particular, if it deletes unused
> block groups, it will create delayed iputs for the free space cache
> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> longer running delayed iputs after a commit. Therefore, if the cleaner
> creates more delayed iputs after delayed iputs are run in
> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> inode crash from the VFS.
> 
> Fix it by parking the cleaner before we actually close anything. Then,
> any remaining delayed iputs will always be handled in
> btrfs_commit_super(). This also ensures that the commit in close_ctree()
> is really the last commit, so we can get rid of the commit in
> cleaner_kthread().
> 
> Fixes: 30928e9baac2 ("btrfs: don't run delayed_iputs in commit")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Also I believe this patch renders the wake_up_process in
btrfs_commit_super a null op so it can also be removed, which leaves a
single place that could wake up the cleaner - transaction_kthread.

So can't we stop transaction and cleaner thread right after setting
CLOSING_FS. And commit the transaction in close_ctree whenever we deem
necessary (in btrfs_commit_super for example) ?

> ---
> Changes from v1:
> 
> - Add a comment explaining why it needs to be a kthread_park(), not
>   kthread_stop()
> - Update later comment now that the cleaner thread is definitely stopped
> 
>  fs/btrfs/disk-io.c | 51 ++++++++++++++--------------------------------
>  1 file changed, 15 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b0ab41da91d1..40bcc45d827d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1664,9 +1664,8 @@ static int cleaner_kthread(void *arg)
>  	struct btrfs_root *root = arg;
>  	struct btrfs_fs_info *fs_info = root->fs_info;
>  	int again;
> -	struct btrfs_trans_handle *trans;
>  
> -	do {
> +	while (1) {
>  		again = 0;
>  
>  		/* Make the cleaner go to sleep early. */
> @@ -1715,42 +1714,16 @@ static int cleaner_kthread(void *arg)
>  		 */
>  		btrfs_delete_unused_bgs(fs_info);
>  sleep:
> +		if (kthread_should_park())
> +			kthread_parkme();
> +		if (kthread_should_stop())
> +			return 0;
>  		if (!again) {
>  			set_current_state(TASK_INTERRUPTIBLE);
> -			if (!kthread_should_stop())
> -				schedule();
> +			schedule();
>  			__set_current_state(TASK_RUNNING);
>  		}
> -	} while (!kthread_should_stop());
> -
> -	/*
> -	 * Transaction kthread is stopped before us and wakes us up.
> -	 * However we might have started a new transaction and COWed some
> -	 * tree blocks when deleting unused block groups for example. So
> -	 * make sure we commit the transaction we started to have a clean
> -	 * shutdown when evicting the btree inode - if it has dirty pages
> -	 * when we do the final iput() on it, eviction will trigger a
> -	 * writeback for it which will fail with null pointer dereferences
> -	 * since work queues and other resources were already released and
> -	 * destroyed by the time the iput/eviction/writeback is made.
> -	 */
> -	trans = btrfs_attach_transaction(root);
> -	if (IS_ERR(trans)) {
> -		if (PTR_ERR(trans) != -ENOENT)
> -			btrfs_err(fs_info,
> -				  "cleaner transaction attach returned %ld",
> -				  PTR_ERR(trans));
> -	} else {
> -		int ret;
> -
> -		ret = btrfs_commit_transaction(trans);
> -		if (ret)
> -			btrfs_err(fs_info,
> -				  "cleaner open transaction commit returned %d",
> -				  ret);
>  	}
> -
> -	return 0;
>  }
>  
>  static int transaction_kthread(void *arg)
> @@ -3931,6 +3904,13 @@ void close_ctree(struct btrfs_fs_info *fs_info)
>  	int ret;
>  
>  	set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> +	/*
> +	 * We don't want the cleaner to start new transactions, add more delayed
> +	 * iputs, etc. while we're closing. We can't use kthread_stop() yet
> +	 * because that frees the task_struct, and the transaction kthread might
> +	 * still try to wake up the cleaner.
> +	 */
> +	kthread_park(fs_info->cleaner_kthread);
>  
>  	/* wait for the qgroup rescan worker to stop */
>  	btrfs_qgroup_wait_for_completion(fs_info, false);
> @@ -3958,9 +3938,8 @@ void close_ctree(struct btrfs_fs_info *fs_info)
>  
>  	if (!sb_rdonly(fs_info->sb)) {
>  		/*
> -		 * If the cleaner thread is stopped and there are
> -		 * block groups queued for removal, the deletion will be
> -		 * skipped when we quit the cleaner thread.
> +		 * The cleaner kthread is stopped, so do one final pass over
> +		 * unused block groups.
>  		 */
>  		btrfs_delete_unused_bgs(fs_info);
>  
> 

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Nikolay Borisov]

On 1.11.18 г. 16:35 ч., Nikolay Borisov wrote:
> 
> 
> On 31.10.18 г. 19:06 ч., Omar Sandoval wrote:
>> From: Omar Sandoval <osandov@fb.com>
>>
>> There's a race between close_ctree() and cleaner_kthread().
>> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
>> sees it set, but this is racy; the cleaner might have already checked
>> the bit and could be cleaning stuff. In particular, if it deletes unused
>> block groups, it will create delayed iputs for the free space cache
>> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
>> longer running delayed iputs after a commit. Therefore, if the cleaner
>> creates more delayed iputs after delayed iputs are run in
>> btrfs_commit_super(), we will leak inodes on unmount and get a busy
>> inode crash from the VFS.
>>
>> Fix it by parking the cleaner before we actually close anything. Then,
>> any remaining delayed iputs will always be handled in
>> btrfs_commit_super(). This also ensures that the commit in close_ctree()
>> is really the last commit, so we can get rid of the commit in
>> cleaner_kthread().
>>
>> Fixes: 30928e9baac2 ("btrfs: don't run delayed_iputs in commit")
>> Signed-off-by: Omar Sandoval <osandov@fb.com>
> 
> Also I believe this patch renders the wake_up_process in
> btrfs_commit_super a null op so it can also be removed, which leaves a
> single place that could wake up the cleaner - transaction_kthread.
> 
> So can't we stop transaction and cleaner thread right after setting
> CLOSING_FS. And commit the transaction in close_ctree whenever we deem
> necessary (in btrfs_commit_super for example) ?

Ok, that won't work because commit_super is called in other contexts
where it can genuinely wake up the trans kthread, blimey. Why can't we
stop transaction kthread first thing in close_ctree ?



> 
>> ---
>> Changes from v1:
>>
>> - Add a comment explaining why it needs to be a kthread_park(), not
>>   kthread_stop()
>> - Update later comment now that the cleaner thread is definitely stopped
>>
>>  fs/btrfs/disk-io.c | 51 ++++++++++++++--------------------------------
>>  1 file changed, 15 insertions(+), 36 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index b0ab41da91d1..40bcc45d827d 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -1664,9 +1664,8 @@ static int cleaner_kthread(void *arg)
>>  	struct btrfs_root *root = arg;
>>  	struct btrfs_fs_info *fs_info = root->fs_info;
>>  	int again;
>> -	struct btrfs_trans_handle *trans;
>>  
>> -	do {
>> +	while (1) {
>>  		again = 0;
>>  
>>  		/* Make the cleaner go to sleep early. */
>> @@ -1715,42 +1714,16 @@ static int cleaner_kthread(void *arg)
>>  		 */
>>  		btrfs_delete_unused_bgs(fs_info);
>>  sleep:
>> +		if (kthread_should_park())
>> +			kthread_parkme();
>> +		if (kthread_should_stop())
>> +			return 0;
>>  		if (!again) {
>>  			set_current_state(TASK_INTERRUPTIBLE);
>> -			if (!kthread_should_stop())
>> -				schedule();
>> +			schedule();
>>  			__set_current_state(TASK_RUNNING);
>>  		}
>> -	} while (!kthread_should_stop());
>> -
>> -	/*
>> -	 * Transaction kthread is stopped before us and wakes us up.
>> -	 * However we might have started a new transaction and COWed some
>> -	 * tree blocks when deleting unused block groups for example. So
>> -	 * make sure we commit the transaction we started to have a clean
>> -	 * shutdown when evicting the btree inode - if it has dirty pages
>> -	 * when we do the final iput() on it, eviction will trigger a
>> -	 * writeback for it which will fail with null pointer dereferences
>> -	 * since work queues and other resources were already released and
>> -	 * destroyed by the time the iput/eviction/writeback is made.
>> -	 */
>> -	trans = btrfs_attach_transaction(root);
>> -	if (IS_ERR(trans)) {
>> -		if (PTR_ERR(trans) != -ENOENT)
>> -			btrfs_err(fs_info,
>> -				  "cleaner transaction attach returned %ld",
>> -				  PTR_ERR(trans));
>> -	} else {
>> -		int ret;
>> -
>> -		ret = btrfs_commit_transaction(trans);
>> -		if (ret)
>> -			btrfs_err(fs_info,
>> -				  "cleaner open transaction commit returned %d",
>> -				  ret);
>>  	}
>> -
>> -	return 0;
>>  }
>>  
>>  static int transaction_kthread(void *arg)
>> @@ -3931,6 +3904,13 @@ void close_ctree(struct btrfs_fs_info *fs_info)
>>  	int ret;
>>  
>>  	set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
>> +	/*
>> +	 * We don't want the cleaner to start new transactions, add more delayed
>> +	 * iputs, etc. while we're closing. We can't use kthread_stop() yet
>> +	 * because that frees the task_struct, and the transaction kthread might
>> +	 * still try to wake up the cleaner.
>> +	 */
>> +	kthread_park(fs_info->cleaner_kthread);
>>  
>>  	/* wait for the qgroup rescan worker to stop */
>>  	btrfs_qgroup_wait_for_completion(fs_info, false);
>> @@ -3958,9 +3938,8 @@ void close_ctree(struct btrfs_fs_info *fs_info)
>>  
>>  	if (!sb_rdonly(fs_info->sb)) {
>>  		/*
>> -		 * If the cleaner thread is stopped and there are
>> -		 * block groups queued for removal, the deletion will be
>> -		 * skipped when we quit the cleaner thread.
>> +		 * The cleaner kthread is stopped, so do one final pass over
>> +		 * unused block groups.
>>  		 */
>>  		btrfs_delete_unused_bgs(fs_info);
>>  
>>

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[David Sterba]

On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> There's a race between close_ctree() and cleaner_kthread().
> close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> sees it set, but this is racy; the cleaner might have already checked
> the bit and could be cleaning stuff. In particular, if it deletes unused
> block groups, it will create delayed iputs for the free space cache
> inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> longer running delayed iputs after a commit. Therefore, if the cleaner
> creates more delayed iputs after delayed iputs are run in
> btrfs_commit_super(), we will leak inodes on unmount and get a busy
> inode crash from the VFS.
> 
> Fix it by parking the cleaner before we actually close anything. Then,
> any remaining delayed iputs will always be handled in
> btrfs_commit_super(). This also ensures that the commit in close_ctree()
> is really the last commit, so we can get rid of the commit in
> cleaner_kthread().
> 
> Fixes: 30928e9baac2 ("btrfs: don't run delayed_iputs in commit")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

I'll queue this patch for rc2 as it fixes crashes I see during testing.
My version does more changes and would be more suitable for a series,
that could actually document the shutdown sequence and add a few
assertions on top.

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2] Btrfs: fix missing delayed iputs on unmount

[Omar Sandoval]

On Wed, Nov 07, 2018 at 05:01:19PM +0100, David Sterba wrote:
> On Wed, Oct 31, 2018 at 10:06:08AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > There's a race between close_ctree() and cleaner_kthread().
> > close_ctree() sets btrfs_fs_closing(), and the cleaner stops when it
> > sees it set, but this is racy; the cleaner might have already checked
> > the bit and could be cleaning stuff. In particular, if it deletes unused
> > block groups, it will create delayed iputs for the free space cache
> > inodes. As of "btrfs: don't run delayed_iputs in commit", we're no
> > longer running delayed iputs after a commit. Therefore, if the cleaner
> > creates more delayed iputs after delayed iputs are run in
> > btrfs_commit_super(), we will leak inodes on unmount and get a busy
> > inode crash from the VFS.
> > 
> > Fix it by parking the cleaner before we actually close anything. Then,
> > any remaining delayed iputs will always be handled in
> > btrfs_commit_super(). This also ensures that the commit in close_ctree()
> > is really the last commit, so we can get rid of the commit in
> > cleaner_kthread().
> > 
> > Fixes: 30928e9baac2 ("btrfs: don't run delayed_iputs in commit")
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> 
> I'll queue this patch for rc2 as it fixes crashes I see during testing.
> My version does more changes and would be more suitable for a series,
> that could actually document the shutdown sequence and add a few
> assertions on top.

Thanks, Dave! I'll keep an eye out for the further cleanups.

> Reviewed-by: David Sterba <dsterba@suse.com>