Message ID | 21e034b59ba97c7f39086e77e08250dcad172717.1607940240.git.fdmanana@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs: fix transaction leaks and crashes during unmount | expand |
On Mon, Dec 14, 2020 at 10:10:45AM +0000, fdmanana@kernel.org wrote: > +static bool rescan_should_stop(struct btrfs_fs_info *fs_info) > +{ > + return btrfs_fs_closing(fs_info) || > + test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); > +} > + > static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > { > struct btrfs_fs_info *fs_info = container_of(work, struct btrfs_fs_info, > @@ -3198,6 +3204,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > struct btrfs_trans_handle *trans = NULL; > int err = -ENOMEM; > int ret = 0; > + bool stopped = false; > > path = btrfs_alloc_path(); > if (!path) > @@ -3210,7 +3217,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > path->skip_locking = 1; > > err = 0; > - while (!err && !btrfs_fs_closing(fs_info)) { > + while (!err && !(stopped = rescan_should_stop(fs_info))) { > trans = btrfs_start_transaction(fs_info->fs_root, 0); > if (IS_ERR(trans)) { > err = PTR_ERR(trans); > @@ -3253,7 +3260,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > } > > mutex_lock(&fs_info->qgroup_rescan_lock); > - if (!btrfs_fs_closing(fs_info)) > + if (!stopped) > fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN; > if (trans) { > ret = update_qgroup_status_item(trans); > @@ -3272,7 +3279,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > > btrfs_end_transaction(trans); > > - if (btrfs_fs_closing(fs_info)) { > + if (stopped) { Thinking aloud, this is slightly different as it uses the cached status of fs_closing but there is mutex lock/unlock or transaction start/end between the checks so the status could change. But as the flow goes, we want to get fresh status in the while loop. Once it stops because of the fs_closing or remount request, the following code does the qgroup status update, wakeups, even tough this means one more transaction. Remount needs to sync anyway and this should be no problem.
On Thu, Dec 17, 2020 at 5:45 PM David Sterba <dsterba@suse.cz> wrote: > > On Mon, Dec 14, 2020 at 10:10:45AM +0000, fdmanana@kernel.org wrote: > > +static bool rescan_should_stop(struct btrfs_fs_info *fs_info) > > +{ > > + return btrfs_fs_closing(fs_info) || > > + test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); > > +} > > + > > static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > > { > > struct btrfs_fs_info *fs_info = container_of(work, struct btrfs_fs_info, > > @@ -3198,6 +3204,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > > struct btrfs_trans_handle *trans = NULL; > > int err = -ENOMEM; > > int ret = 0; > > + bool stopped = false; > > > > path = btrfs_alloc_path(); > > if (!path) > > @@ -3210,7 +3217,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > > path->skip_locking = 1; > > > > err = 0; > > - while (!err && !btrfs_fs_closing(fs_info)) { > > + while (!err && !(stopped = rescan_should_stop(fs_info))) { > > trans = btrfs_start_transaction(fs_info->fs_root, 0); > > if (IS_ERR(trans)) { > > err = PTR_ERR(trans); > > @@ -3253,7 +3260,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > > } > > > > mutex_lock(&fs_info->qgroup_rescan_lock); > > - if (!btrfs_fs_closing(fs_info)) > > + if (!stopped) > > fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN; > > if (trans) { > > ret = update_qgroup_status_item(trans); > > @@ -3272,7 +3279,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) > > > > btrfs_end_transaction(trans); > > > > - if (btrfs_fs_closing(fs_info)) { > > + if (stopped) { > > Thinking aloud, this is slightly different as it uses the cached status > of fs_closing but there is mutex lock/unlock or transaction start/end > between the checks so the status could change. > > But as the flow goes, we want to get fresh status in the while loop. > Once it stops because of the fs_closing or remount request, the > following code does the qgroup status update, wakeups, even tough this > means one more transaction. Remount needs to sync anyway and this should > be no problem. Yes, that and the fact that the rescan calls complete_all(&fs_info->qgroup_rescan_completion) before it logs the reason why it finished. So it would be possible for remount to stop it, then remount completes, and then the rescan worker logs that an error happened instead of logging that it was stopped - it's a very big stretch for that to happen, but an error message would be confusing from a user's perspective at least.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 47f27658eac1..808370ada888 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -3190,6 +3190,12 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans, return ret; } +static bool rescan_should_stop(struct btrfs_fs_info *fs_info) +{ + return btrfs_fs_closing(fs_info) || + test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); +} + static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) { struct btrfs_fs_info *fs_info = container_of(work, struct btrfs_fs_info, @@ -3198,6 +3204,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) struct btrfs_trans_handle *trans = NULL; int err = -ENOMEM; int ret = 0; + bool stopped = false; path = btrfs_alloc_path(); if (!path) @@ -3210,7 +3217,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) path->skip_locking = 1; err = 0; - while (!err && !btrfs_fs_closing(fs_info)) { + while (!err && !(stopped = rescan_should_stop(fs_info))) { trans = btrfs_start_transaction(fs_info->fs_root, 0); if (IS_ERR(trans)) { err = PTR_ERR(trans); @@ -3253,7 +3260,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) } mutex_lock(&fs_info->qgroup_rescan_lock); - if (!btrfs_fs_closing(fs_info)) + if (!stopped) fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN; if (trans) { ret = update_qgroup_status_item(trans); @@ -3272,7 +3279,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) btrfs_end_transaction(trans); - if (btrfs_fs_closing(fs_info)) { + if (stopped) { btrfs_info(fs_info, "qgroup scan paused"); } else if (err >= 0) { btrfs_info(fs_info, "qgroup scan completed%s", diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 022f20810089..b24fa62375e0 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1968,6 +1968,14 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) btrfs_scrub_cancel(fs_info); btrfs_pause_balance(fs_info); + /* + * Pause the qgroup rescan worker if it is running. We don't want + * it to be still running after we are in RO mode, as after that, + * by the time we unmount, it might have left a transaction open, + * so we would leak the transaction and/or crash. + */ + btrfs_qgroup_wait_for_completion(fs_info, false); + ret = btrfs_commit_super(fs_info); if (ret) goto restore;