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

[Nikolay Borisov <n.borisov.lkml@gmail.com>]

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);
>>  
>>