On 1.11.18 г. 16:35 ч., Nikolay Borisov wrote:
>
>
> On 31.10.18 г. 19:06 ч., Omar Sandoval wrote:
>> From: Omar Sandoval <osandov@xxxxxx>
>>
>> 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@xxxxxx>
>
> 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);
>>
>>
