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

[Omar Sandoval <osandov@osandov.com>]

On Wed, Oct 31, 2018 at 06:40:29PM +0200, Nikolay Borisov wrote:
> 
> 
> On 31.10.18 г. 18:35 ч., Omar Sandoval wrote:
> > On Wed, Oct 31, 2018 at 10:43:02AM +0200, Nikolay Borisov wrote:
> >>
> >>
> >> On 31.10.18 г. 2:14 ч., 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>
> >>> ---
> >>> We found this with a stress test that our containers team runs. I'm
> >>> wondering if this same race could have caused any other issues other
> >>> than this new iput thing, but I couldn't identify any.
> >>>
> >>>  fs/btrfs/disk-io.c | 40 +++++++---------------------------------
> >>>  1 file changed, 7 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>> index b0ab41da91d1..7c17284ae3c2 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,7 @@ void close_ctree(struct btrfs_fs_info *fs_info)
> >>>  	int ret;
> >>>  
> >>>  	set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> >>> +	kthread_park(fs_info->cleaner_kthread);
> >>
> >> Can't you directly call kthread_stop here? When you park the thread it
> >> will sleep and then when you call kthread_stop that function will unpark
> >> the thread and the cleaner kthread will see KTHREAD_SHOULD_STOP bit and
> >> just return 0. So the from the moment the thread is parked until it's
> >> stopped it doesn't have a chance to do useful work.
> > 
> > kthread_stop() frees the task_struct, but we might still try to wake up
> > the cleaner kthread from somewhere (e.g., from the transaction kthread).
> > So we really need to keep the cleaner alive but not doing work.
> 
> This dependency then needs to be documented via a comment or at the very
> least mentioned in the changelog. Is it possible to refactor the code
> (in a different patch) to actually ensure that transaction is stopped
> and then kthread as well to remove this dependency ?

Then we get the same issue with things trying to wake up the transaction
kthread after it's been freed (__btrfs_end_transaction(), in
particular). Maybe we could make it work, but it seems very fragile.

I'll send a v2 with a comment.