btrfs: qgroups, fix rescan worker running races
PATCH 1/3 btrfs: qgroups, fix rescan worker running races
From: Jeff Mahoney <[email protected]>
Commit d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization) fixed the issue with BTRFS_IOC_QUOTA_RESCAN_WAIT being racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.
The race this time is between qgroup_rescan_init setting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completion btrfs_ioctl_quota_rescan_wait waits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.
The right place to do it is when we're queuing the worker. The flag really just means that btrfs_ioctl_quota_rescan_wait should wait for a completion.
This patch introduces a new helper, queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.
While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex. btrfs_ioctl_quota_rescan_wait doesn't need to take the spinlock too.
--- fs/btrfs/ctree.h | 1 + fs/btrfs/qgroup.c | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 12 deletions(-)
fs/btrfs/ctree.h +1 -0
@@ -1045,6 +1045,7 @@ struct btrfs_fs_info {struct btrfs_workqueue *qgroup_rescan_workers;struct completion qgroup_rescan_completion;struct btrfs_work qgroup_rescan_work; +/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */ /* filesystem state */
fs/btrfs/qgroup.c +28 -12
@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; } +static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) { +btrfs_warn (fs_info,"rescan worker already queued" ); +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } + +/* +* Being queued is enough for btrfs_qgroup_wait_for_completion +* to need to wait. +*/ + fs_info->qgroup_rescan_running = true; +mutex_unlock (&fs_info->qgroup_rescan_lock); + +btrfs_queue_work (fs_info->qgroup_rescan_workers, + &fs_info->qgroup_rescan_work); +} +/* * called from commit_transaction. Writes all changed qgroups to disk. */
@@ -2123,8 +2147,7 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans, ret =qgroup_rescan_init (fs_info,0 ,1 );if (!ret) {qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); } ret =0 ; }
@@ -2713,7 +2736,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, sizeof(fs_info->qgroup_rescan_progress)); fs_info->qgroup_rescan_progress.objectid = progress_objectid;init_completion (&fs_info->qgroup_rescan_completion); - fs_info->qgroup_rescan_running = true;spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);
@@ -2785,9 +2807,7 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); - +queue_rescan_worker (fs_info);return 0 ; }
@@ -2798,9 +2818,7 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,int ret =0 ;mutex_lock (&fs_info->qgroup_rescan_lock); -spin_lock (&fs_info->qgroup_lock); running = fs_info->qgroup_rescan_running; -spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);if (!running)
@@ -2819,12 +2837,10 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,* this is only called from open_ctree where we're still single threaded, thus * locking is omitted here. */ -void -btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) +void btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) {if (fs_info->qgroup_flags &BTRFS_QGROUP_STATUS_FLAG_RESCAN ) -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); }/*
Signature · 1 line
2.12.3
PATCH 2/3 btrfs: qgroups, remove unnecessary memset before btrfs_init_work
From: Jeff Mahoney <[email protected]>
btrfs_init_work clears the work struct except for ->wq, so the memset before calling btrfs_init_work in qgroup_rescan_init is unnecessary.
We'll also initialize ->wq in btrfs_init_work so that it's obvious.
--- fs/btrfs/async-thread.c | 1 + fs/btrfs/qgroup.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)
fs/btrfs/async-thread.c +1 -0
@@ -354,6 +354,7 @@ void btrfs_init_work(struct btrfs_work *work, btrfs_work_func_t uniq_func,INIT_WORK (&work->normal_work, uniq_func);INIT_LIST_HEAD (&work->ordered_list); work->flags =0 ; + work->wq =NULL ; } static inline void __btrfs_queue_work(struct __btrfs_workqueue *wq,
fs/btrfs/qgroup.c +0 -2
@@ -2740,8 +2740,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock); -memset (&fs_info->qgroup_rescan_work,0 , -sizeof (fs_info->qgroup_rescan_work));btrfs_init_work (&fs_info->qgroup_rescan_work, btrfs_qgroup_rescan_helper, btrfs_qgroup_rescan_worker,NULL ,NULL );
Signature · 1 line
2.12.3
Re: [PATCH 2/3] btrfs: qgroups, remove unnecessary memset before btrfs_init_work
Quoted 8 lines · On 26.04.2018 22:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
btrfs_init_workclears the work struct except for ->wq, so the memset before callingbtrfs_init_workinqgroup_rescan_initis unnecessary.We'll also initialize ->wq in
btrfs_init_workso that it's obvious.Signed-off-by Jeff Mahoney <[email protected]>
Quoted 31 lines
--- fs/btrfs/async-thread.c | 1 + fs/btrfs/qgroup.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)fs/btrfs/async-thread.c +1
-0@@ -354,6 +354,7 @@ void btrfs_init_work(struct btrfs_work *work, btrfs_work_func_t uniq_func,INIT_WORK (&work->normal_work, uniq_func);INIT_LIST_HEAD (&work->ordered_list); work->flags =0 ; + work->wq =NULL ; } static inline void __btrfs_queue_work(struct __btrfs_workqueue *wq,fs/btrfs/qgroup.c +0
-2@@ -2740,8 +2740,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock); -memset (&fs_info->qgroup_rescan_work,0 , -sizeof (fs_info->qgroup_rescan_work));btrfs_init_work (&fs_info->qgroup_rescan_work, btrfs_qgroup_rescan_helper, btrfs_qgroup_rescan_worker,NULL ,NULL );
PATCH 3/3 btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
In reply to [email protected]From: Jeff Mahoney <[email protected]>
If we fail to allocate memory for a path, don't bother trying to insert the qgroup status item. We haven't done anything yet and it'll fail also. Just print an error and be done with it.
--- fs/btrfs/qgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/qgroup.c +1 -1
@@ -2648,7 +2648,6 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)btrfs_end_transaction (trans); } -out:btrfs_free_path (path);mutex_lock (&fs_info->qgroup_rescan_lock);
@@ -2688,6 +2687,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)btrfs_info (fs_info,"qgroup scan completed%s" , err >0 ?" (inconsistency flag cleared)" :"" ); }else { +out:btrfs_err (fs_info,"qgroup scan failed with %d" , err); }
Signature · 1 line
2.12.3
Re: [PATCH 3/3] btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
Quoted 7 lines · On 26.04.2018 22:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
If we fail to allocate memory for a path, don't bother trying to insert the qgroup status item. We haven't done anything yet and it'll fail also. Just print an error and be done with it.
Signed-off-by Jeff Mahoney <[email protected]>
nit: So the code is correct however, having the out label there is really ugly. What about on path alloc failure just have the print in the if branch do goto done?
Quoted 25 lines
--- fs/btrfs/qgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/qgroup.c +1
-1@@ -2648,7 +2648,6 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)btrfs_end_transaction (trans); } -out:btrfs_free_path (path);mutex_lock (&fs_info->qgroup_rescan_lock);@@ -2688,6 +2687,7 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)btrfs_info (fs_info,"qgroup scan completed%s" , err >0 ?" (inconsistency flag cleared)" :"" ); }else { +out:btrfs_err (fs_info,"qgroup scan failed with %d" , err); }
Re: [PATCH 3/3] btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
Quoted 7 lines · Nikolay Borisov
Quoted 6 lines · On 26.04.2018 22:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
If we fail to allocate memory for a path, don't bother trying to insert the qgroup status item. We haven't done anything yet and it'll fail also. Just print an error and be done with it.
Quoted 5 lines
Quoted 1 line
Signed-off-by Jeff Mahoney <[email protected]>nit: So the code is correct however, having the out label there is really ugly. What about on path alloc failure just have the print in the if branch do goto done?
Yeah, I don't like jumping to the inner blocks either. I saw this in the qgroup code so we should clean it up and not add new instances.
In this case, only the path allocation failure jumps to the out label, so printing the message and then jump to done makes sense to me. However, the message would have to be duplicated in the end, and I don't see a better way without further restructuring the code.
Re: [PATCH 3/3] btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
Quoted 14 lines · David Sterba
Quoted 7 lines · Nikolay Borisov
Quoted 6 lines · On 26.04.2018 22:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
If we fail to allocate memory for a path, don't bother trying to insert the qgroup status item. We haven't done anything yet and it'll fail also. Just print an error and be done with it.
Quoted 5 lines
Quoted 1 line
Signed-off-by Jeff Mahoney <[email protected]>nit: So the code is correct however, having the out label there is really ugly. What about on path alloc failure just have the print in the if branch do goto done?
Quoted 8 lines
Yeah, I don't like jumping to the inner blocks either. I saw this in the qgroup code so we should clean it up and not add new instances.
In this case, only the path allocation failure jumps to the out label, so printing the message and then jump to done makes sense to me. However, the message would have to be duplicated in the end, and I don't see a better way without further restructuring the code.
It doesn't require major surgery. The else can be disconnected.
-Jeff
Signature · 2 lines
Jeff Mahoney SUSE Labs
PATCH v2 1/1 btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
If we fail to allocate memory for a path, don't bother trying to insert the qgroup status item. We haven't done anything yet and it'll fail also. Just print an error and be done with it.
--- fs/btrfs/qgroup.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
fs/btrfs/qgroup.c +4 -5
@@ -2648,7 +2648,6 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)btrfs_end_transaction (trans); } -out:btrfs_free_path (path);mutex_lock (&fs_info->qgroup_rescan_lock);
@@ -2684,13 +2683,13 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)if (btrfs_fs_closing (fs_info)) {btrfs_info (fs_info,"qgroup scan paused" ); - }else if (err >=0 ) { + err =0 ; + }else if (err >=0 )btrfs_info (fs_info,"qgroup scan completed%s" , err >0 ?" (inconsistency flag cleared)" :"" ); - }else { +out: +if (err <0 )btrfs_err (fs_info,"qgroup scan failed with %d" , err); - } - done:mutex_lock (&fs_info->qgroup_rescan_lock); fs_info->qgroup_rescan_running = false;
Signature · 1 line
2.12.3
Re: [PATCH v2] btrfs: qgroup, don't try to insert status item after ENOMEM in rescan worker
Quoted 24 lines · Jeff Mahoney
If we fail to allocate memory for a path, don't bother trying to insert the qgroup status item. We haven't done anything yet and it'll fail also. Just print an error and be done with it.
Signed-off-by Jeff Mahoney <[email protected]>--- fs/btrfs/qgroup.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)fs/btrfs/qgroup.c +0
-1@@ -2648,7 +2648,6 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)btrfs_end_transaction (trans); } -out:btrfs_free_path (path);mutex_lock (&fs_info->qgroup_rescan_lock);@@ -2684,13 +2683,13 @@ static void btrfs_qgroup_rescan_worker(struct btrfs_work *work)if (btrfs_fs_closing (fs_info)) {
fs/btrfs/qgroup.c static void btrfs_qgroup_rescan_worker(struct btrfs_work *work) · line 2692
Quoted 10 lines
fs/btrfs/qgroup.c +4
-2btrfs_info (fs_info,"qgroup scan paused" ); - }else if (err >=0 ) { + err =0 ; + }else if (err >=0 )btrfs_info (fs_info,"qgroup scan completed%s" , err >0 ?" (inconsistency flag cleared)" :"" ); - }else { +out: +if (err <0 )btrfs_err (fs_info,"qgroup scan failed with %d" , err);
Ah right, with the err = 0 in the fs_closing check we won't see both messages reported, "qgroup scan paused" and "qgroup scan failed with %d".
Quoted 13 lines
fs/btrfs/qgroup.c +0
-2- } - done:mutex_lock (&fs_info->qgroup_rescan_lock); fs_info->qgroup_rescan_running = false;Signature · 7 lines
2.12.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 22 lines · On 26.04.2018 22:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.
Quoted 6 lines
While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>
LGTM.
Quoted 53 lines
--- fs/btrfs/ctree.h | 1 + fs/btrfs/qgroup.c | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +1
-0@@ -1045,6 +1045,7 @@ struct btrfs_fs_info {struct btrfs_workqueue *qgroup_rescan_workers;struct completion qgroup_rescan_completion;struct btrfs_work qgroup_rescan_work; +/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */ /* filesystem state */ fs/btrfs/qgroup.c +24
-0@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; } +static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) { +btrfs_warn (fs_info,"rescan worker already queued" ); +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } + +/* +* Being queued is enough for btrfs_qgroup_wait_for_completion +* to need to wait. +*/ + fs_info->qgroup_rescan_running = true; +mutex_unlock (&fs_info->qgroup_rescan_lock); + +btrfs_queue_work (fs_info->qgroup_rescan_workers, + &fs_info->qgroup_rescan_work); +} +/* * called from commit_transaction. Writes all changed qgroups to disk. */
fs/btrfs/qgroup.c int btrfs_run_qgroups(struct btrfs_trans_handle *trans, · line 2151
Quoted 7 lines
fs/btrfs/qgroup.c +1
-2@@ -2123,8 +2147,7 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans, ret =qgroup_rescan_init (fs_info,0 ,1 );if (!ret) {qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); }
So here it's not possible to race, since if qgroup_rescan_init returns 0 then we are guaranteed to queue the rescan.
Quoted 11 lines
fs/btrfs/qgroup.c +0
-1ret =0 ; }@@ -2713,7 +2736,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, sizeof(fs_info->qgroup_rescan_progress)); fs_info->qgroup_rescan_progress.objectid = progress_objectid;init_completion (&fs_info->qgroup_rescan_completion); - fs_info->qgroup_rescan_running = true;spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);
fs/btrfs/qgroup.c btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info) · line 2810
Quoted 7 lines
fs/btrfs/qgroup.c +1
-3@@ -2785,9 +2807,7 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); - +queue_rescan_worker (fs_info);
Which leaves this to be the only problematic case, in case transaction joining/commit fails, right?
Quoted 29 lines
fs/btrfs/qgroup.c +2
-6return 0 ; }@@ -2798,9 +2818,7 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,int ret =0 ;mutex_lock (&fs_info->qgroup_rescan_lock); -spin_lock (&fs_info->qgroup_lock); running = fs_info->qgroup_rescan_running; -spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);if (!running)@@ -2819,12 +2837,10 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,* this is only called from open_ctree where we're still single threaded, thus * locking is omitted here. */ -void -btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) +void btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) {if (fs_info->qgroup_flags &BTRFS_QGROUP_STATUS_FLAG_RESCAN ) -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); }/*
Quoted 19 lines · On Thu, Apr 26, 2018 at 8:23 PM, <[email protected]> wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.
Quoted 8 lines
This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)
The commit id and subjects don't match:
commit d2c609b834d62f1e91f1635a27dca29f7806d3d6
Author: Jeff Mahoney <[email protected]> Date: Mon Aug 15 12:10:33 2016 -0400
btrfs: properly track when rescan worker is running
Quoted 114 lines
Signed-off-by Jeff Mahoney <[email protected]>--- fs/btrfs/ctree.h | 1 + fs/btrfs/qgroup.c | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +1
-0@@ -1045,6 +1045,7 @@ struct btrfs_fs_info {struct btrfs_workqueue *qgroup_rescan_workers;struct completion qgroup_rescan_completion;struct btrfs_work qgroup_rescan_work; +/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */ /* filesystem state */ fs/btrfs/qgroup.c +28
-13@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; } +static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) { +btrfs_warn (fs_info,"rescan worker already queued" ); +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } + +/* +* Being queued is enough for btrfs_qgroup_wait_for_completion +* to need to wait. +*/ + fs_info->qgroup_rescan_running = true; +mutex_unlock (&fs_info->qgroup_rescan_lock); + +btrfs_queue_work (fs_info->qgroup_rescan_workers, + &fs_info->qgroup_rescan_work); +} +/* * called from commit_transaction. Writes all changed qgroups to disk. */ @@ -2123,8 +2147,7 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans, ret =qgroup_rescan_init (fs_info,0 ,1 );if (!ret) {qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); } ret =0 ; }@@ -2713,7 +2736,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, sizeof(fs_info->qgroup_rescan_progress)); fs_info->qgroup_rescan_progress.objectid = progress_objectid;init_completion (&fs_info->qgroup_rescan_completion); - fs_info->qgroup_rescan_running = true;spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);@@ -2785,9 +2807,7 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); - +queue_rescan_worker (fs_info);return 0 ; }@@ -2798,9 +2818,7 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,int ret =0 ;mutex_lock (&fs_info->qgroup_rescan_lock); -spin_lock (&fs_info->qgroup_lock); running = fs_info->qgroup_rescan_running; -spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);if (!running)@@ -2819,12 +2837,10 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,* this is only called from open_ctree where we're still single threaded, thus * locking is omitted here. */ -void -btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) +void btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) {if (fs_info->qgroup_flags &BTRFS_QGROUP_STATUS_FLAG_RESCAN ) -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); }/* -- 2.12.3
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
Quoted 29 lines · Filipe Manana
Quoted 19 lines · On Thu, Apr 26, 2018 at 8:23 PM, <[email protected]> wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.Quoted 8 lines
This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)
Quoted 8 lines
The commit id and subjects don't match:
commit
d2c609b834d62f1e91f1635a27dca29f7806d3d6Author: Jeff Mahoney <[email protected]> Date: Mon Aug 15 12:10:33 2016 -0400btrfs: properly track when rescan worker is running
Thanks. Fixed.
-Jeff
Signature · 2 lines
Jeff Mahoney SUSE Labs
Quoted 22 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.
Quoted 6 lines
While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>
I've added this to misc-next as I'd like to push it to the next rc. The Fixes is fixed.
fs/btrfs/ctree.h struct btrfs_fs_info { · line 1049
Quoted 2 lines
fs/btrfs/ctree.h +1
-0+/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */
Comments merged.
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2076
Quoted 11 lines
fs/btrfs/ctree.h +0
-0/* filesystem state */ fs/btrfs/qgroup.c +2
-0@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; } +static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{
And this had to be moved upwards as there was earlier use of btrfs_queue_work that matched following the hunk.
Quoted 15 lines
fs/btrfs/qgroup.c +3
-2+} +/* * called from commit_transaction. Writes all changed qgroups to disk. */ @@ -2123,8 +2147,7 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans, ret =qgroup_rescan_init (fs_info,0 ,1 );if (!ret) {qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); } ret =0 ; }
Quoted 30 lines · David Sterba
Quoted 22 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.Quoted 6 lines
While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>
Quoted 7 lines
I've added this to misc-next as I'd like to push it to the next rc. The Fixes is fixed.
fs/btrfs/ctree.h struct btrfs_fs_info { · line 1049
Quoted 2 lines
fs/btrfs/ctree.h +1
-0+/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */ Comments merged.
Thanks.
Quoted 9 lines
Quoted 9 lines
fs/btrfs/ctree.h +0
-0/* filesystem state */ fs/btrfs/qgroup.c +0
-0@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; }
Quoted 5 lines
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2076
Quoted 2 lines
fs/btrfs/qgroup.c +2
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{And this had to be moved upwards as there was earlier use of
btrfs_queue_workthat matched following the hunk.
Weird. That must be exactly the kind of mismerge artifact that we were talking about the other day. In my tree it's in the right spot.
-Jeff
Signature · 2 lines
Jeff Mahoney SUSE Labs
Quoted 8 lines · Jeff Mahoney
Quoted 5 lines
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2076
Quoted 2 lines
fs/btrfs/qgroup.c +2
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{And this had to be moved upwards as there was earlier use of
btrfs_queue_workthat matched following the hunk.Weird. That must be exactly the kind of mismerge artifact that we were talking about the other day. In my tree it's in the right spot.
I've tried current master, upcoming pull request queue (misc-4.17, one nonc-onflicting patch) and current misc-next. None of them applies the patch cleanly and the function is still added after the first use, so this would not compile.
The result can be found in
https://github.com/kdave/btrfs-devel/commits/ext/jeffm/qgroup-fixes
Quoted 10 lines · David Sterba
Quoted 8 lines · Jeff Mahoney
Quoted 5 lines
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2076
Quoted 2 lines
fs/btrfs/qgroup.c +2
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{And this had to be moved upwards as there was earlier use of
btrfs_queue_workthat matched following the hunk.Weird. That must be exactly the kind of mismerge artifact that we were talking about the other day. In my tree it's in the right spot.
Quoted 8 lines
I've tried current master, upcoming pull request queue (misc-4.17, one nonc-onflicting patch) and current misc-next. None of them applies the patch cleanly and the function is still added after the first use, so this would not compile.
The result can be found in
https://github.com/kdave/btrfs-devel/commits/ext/jeffm/qgroup-fixes
Thanks. The "Fixes" is incorrect there. I had the right commit message but not the right commit id. It should be:
8d9eddad1946 (Btrfs: fix qgroup rescan worker initialization)
-Jeff
Signature · 2 lines
Jeff Mahoney SUSE Labs
Quoted 16 lines · Jeff Mahoney
Quoted 15 lines · David Sterba
Quoted 8 lines · Jeff Mahoney
Quoted 5 lines
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2076
Quoted 2 lines
fs/btrfs/qgroup.c +2
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{And this had to be moved upwards as there was earlier use of
btrfs_queue_workthat matched following the hunk.Weird. That must be exactly the kind of mismerge artifact that we were talking about the other day. In my tree it's in the right spot.
I've tried current master, upcoming pull request queue (misc-4.17, one nonc-onflicting patch) and current misc-next. None of them applies the patch cleanly and the function is still added after the first use, so this would not compile.
Quoted 8 lines
Quoted 3 lines
The result can be found in
https://github.com/kdave/btrfs-devel/commits/ext/jeffm/qgroup-fixesThanks. The "Fixes" is incorrect there. I had the right commit message but not the right commit id. It should be:
8d9eddad1946(Btrfs: fix qgroup rescan worker initialization)
I've updated the wrong part, subject instead of the commit id. Now fixed.
Quoted 27 lines · David Sterba
Quoted 26 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.
Quoted 6 lines
Quoted 2 lines
Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>I've added this to misc-next as I'd like to push it to the next rc. The Fixes is fixed.
I don't see it pushed to misc-next yet, but based on f89fbcd776, could you update the reference in the first line of the commit to match the Fixes line?
Thanks,
Noah
Quoted 5 lines · Noah Massey
Quoted 4 lines · David Sterba
Quoted 3 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization)
...
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>I've added this to misc-next as I'd like to push it to the next rc. The Fixes is fixed.
I don't see it pushed to misc-next yet, but based on
f89fbcd776, could you update the reference in the first line of the commit to match the Fixes line?
Fixed, thanks for the notice.
Quoted 22 lines · On 2018年04月27日 03:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.
Quoted 6 lines
While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>
A little off-topic, (thanks Nikolay for reporting this) sometimes btrfs/017 could report qgroup corruption, and it turns out it's related to rescan racy, which double account existing tree blocks twice. (One by btrfs quota enable, another by btrfs quota rescan -w)
Would this patch help in such case?
Thanks,
Qu
Quoted 107 lines
--- fs/btrfs/ctree.h | 1 + fs/btrfs/qgroup.c | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +1
-0@@ -1045,6 +1045,7 @@ struct btrfs_fs_info {struct btrfs_workqueue *qgroup_rescan_workers;struct completion qgroup_rescan_completion;struct btrfs_work qgroup_rescan_work; +/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */ /* filesystem state */ fs/btrfs/qgroup.c +28
-12@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; } +static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) { +btrfs_warn (fs_info,"rescan worker already queued" ); +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } + +/* +* Being queued is enough for btrfs_qgroup_wait_for_completion +* to need to wait. +*/ + fs_info->qgroup_rescan_running = true; +mutex_unlock (&fs_info->qgroup_rescan_lock); + +btrfs_queue_work (fs_info->qgroup_rescan_workers, + &fs_info->qgroup_rescan_work); +} +/* * called from commit_transaction. Writes all changed qgroups to disk. */ @@ -2123,8 +2147,7 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans, ret =qgroup_rescan_init (fs_info,0 ,1 );if (!ret) {qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); } ret =0 ; }@@ -2713,7 +2736,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, sizeof(fs_info->qgroup_rescan_progress)); fs_info->qgroup_rescan_progress.objectid = progress_objectid;init_completion (&fs_info->qgroup_rescan_completion); - fs_info->qgroup_rescan_running = true;spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);@@ -2785,9 +2807,7 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); - +queue_rescan_worker (fs_info);return 0 ; }@@ -2798,9 +2818,7 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,int ret =0 ;mutex_lock (&fs_info->qgroup_rescan_lock); -spin_lock (&fs_info->qgroup_lock); running = fs_info->qgroup_rescan_running; -spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);if (!running)@@ -2819,12 +2837,10 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,* this is only called from open_ctree where we're still single threaded, thus * locking is omitted here. */ -void -btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) +void btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) {if (fs_info->qgroup_flags &BTRFS_QGROUP_STATUS_FLAG_RESCAN ) -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); }/*
Quoted 32 lines · Qu Wenruo
Quoted 22 lines · On 2018年04月27日 03:23, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
Commit
d2c609b834d6(Btrfs: fix qgroup rescan worker initialization) fixed the issue withBTRFS_IOC_QUOTA_RESCAN_WAITbeing racy, but ended up reintroducing the hang-on-unmount bug that the commit it intended to fix addressed.The race this time is between
qgroup_rescan_initsetting ->qgroup_rescan_running = true and the worker starting. There are many scenarios where we initialize the worker and never start it. The completionbtrfs_ioctl_quota_rescan_waitwaits for will never come. This can happen even without involving error handling, since mounting the file system read-only returns between initializing the worker and queueing it.The right place to do it is when we're queuing the worker. The flag really just means that
btrfs_ioctl_quota_rescan_waitshould wait for a completion.This patch introduces a new helper,
queue_rescan_worker, that handles the ->qgroup_rescan_running flag, including any races with umount.Quoted 6 lines
While we're at it, ->qgroup_rescan_running is protected only by the ->qgroup_rescan_mutex.
btrfs_ioctl_quota_rescan_waitdoesn't need to take the spinlock too.Fixes d2c609b834d6 (Btrfs: fix qgroup rescan worker initialization)Signed-off-by Jeff Mahoney <[email protected]>
Quoted 6 lines
A little off-topic, (thanks Nikolay for reporting this) sometimes btrfs/017 could report qgroup corruption, and it turns out it's related to rescan racy, which double account existing tree blocks twice. (One by btrfs quota enable, another by btrfs quota rescan -w)
Would this patch help in such case?
It shouldn't. This only fixes races between the rescan worker getting initialized and running vs waiting for it to complete.
-Jeff
Quoted 107 lines
Quoted 106 lines
fs/btrfs/ctree.h | 1 + fs/btrfs/qgroup.c | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +1
-0@@ -1045,6 +1045,7 @@ struct btrfs_fs_info {struct btrfs_workqueue *qgroup_rescan_workers;struct completion qgroup_rescan_completion;struct btrfs_work qgroup_rescan_work; +/* qgroup rescan worker is running or queued to run */ bool qgroup_rescan_running;/* protected by qgroup_rescan_lock */ /* filesystem state */ fs/btrfs/qgroup.c +28
-12@@ -2072,6 +2072,30 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans,return ret; } +static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) { +btrfs_warn (fs_info,"rescan worker already queued" ); +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } + +/* +* Being queued is enough for btrfs_qgroup_wait_for_completion +* to need to wait. +*/ + fs_info->qgroup_rescan_running = true; +mutex_unlock (&fs_info->qgroup_rescan_lock); + +btrfs_queue_work (fs_info->qgroup_rescan_workers, + &fs_info->qgroup_rescan_work); +} +/* * called from commit_transaction. Writes all changed qgroups to disk. */ @@ -2123,8 +2147,7 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans, ret =qgroup_rescan_init (fs_info,0 ,1 );if (!ret) {qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); } ret =0 ; }@@ -2713,7 +2736,6 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid, sizeof(fs_info->qgroup_rescan_progress)); fs_info->qgroup_rescan_progress.objectid = progress_objectid;init_completion (&fs_info->qgroup_rescan_completion); - fs_info->qgroup_rescan_running = true;spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);@@ -2785,9 +2807,7 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)qgroup_rescan_zero_tracking (fs_info); -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); - +queue_rescan_worker (fs_info);return 0 ; }@@ -2798,9 +2818,7 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,int ret =0 ;mutex_lock (&fs_info->qgroup_rescan_lock); -spin_lock (&fs_info->qgroup_lock); running = fs_info->qgroup_rescan_running; -spin_unlock (&fs_info->qgroup_lock);mutex_unlock (&fs_info->qgroup_rescan_lock);if (!running)@@ -2819,12 +2837,10 @@ int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,* this is only called from open_ctree where we're still single threaded, thus * locking is omitted here. */ -void -btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) +void btrfs_qgroup_rescan_resume (struct btrfs_fs_info *fs_info) {if (fs_info->qgroup_flags &BTRFS_QGROUP_STATUS_FLAG_RESCAN ) -btrfs_queue_work (fs_info->qgroup_rescan_workers, - &fs_info->qgroup_rescan_work); +queue_rescan_worker (fs_info); }/*
Signature · 2 lines
Jeff Mahoney SUSE Labs
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2082
Quoted 9 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
fs/btrfs/qgroup.c +8
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) {
The warning is quite noisy, I see it after tests btrfs/ 017, 022, 124, 139, 153. Is it necessary for non-debugging builds?
The tested branch was full for-next so it could be your patchset interacting with other fixes, but the warning noise level question still stands.
Quoted 15 lines
fs/btrfs/qgroup.c +15
-0+btrfs_warn (fs_info,"rescan worker already queued" ); +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } + +/* +* Being queued is enough for btrfs_qgroup_wait_for_completion +* to need to wait. +*/ + fs_info->qgroup_rescan_running = true; +mutex_unlock (&fs_info->qgroup_rescan_lock); + +btrfs_queue_work (fs_info->qgroup_rescan_workers, + &fs_info->qgroup_rescan_work); +}
Quoted 14 lines · David Sterba
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2082
Quoted 9 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
fs/btrfs/qgroup.c +8
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) {The warning is quite noisy, I see it after tests btrfs/ 017, 022, 124, 139, 153. Is it necessary for non-debugging builds?
Quoted 3 lines
The tested branch was full for-next so it could be your patchset interacting with other fixes, but the warning noise level question still stands.
So it must be something with the rest of misc-next or for-next patches, current for 4.17 queue does show the warning at all, and the patch is ok for merge.
Quoted 15 lines · David Sterba
Quoted 14 lines · David Sterba
fs/btrfs/qgroup.c int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans, · line 2082
Quoted 9 lines · On Thu, Apr 26, 2018 at 03:23:49PM -0400, [email protected] wrote:
From: Jeff Mahoney <[email protected]>
fs/btrfs/qgroup.c +8
-0+static void queue_rescan_worker (struct btrfs_fs_info *fs_info) +{ +mutex_lock (&fs_info->qgroup_rescan_lock); +if (btrfs_fs_closing (fs_info)) { +mutex_unlock (&fs_info->qgroup_rescan_lock); +return ; + } +if (WARN_ON (fs_info->qgroup_rescan_running)) {The warning is quite noisy, I see it after tests btrfs/ 017, 022, 124, 139, 153. Is it necessary for non-debugging builds?
Quoted 8 lines
Quoted 3 lines
The tested branch was full for-next so it could be your patchset interacting with other fixes, but the warning noise level question still stands.
So it must be something with the rest of misc-next or for-next patches, current for 4.17 queue does show the warning at all, and the patch is ok for merge.
You might have something that causes it to be more noisy but it looks like it should be possible to hit on 4.16. The warning is supposed to detect and complain about multiple rescan threads starting. What I think it's doing here is (correctly) identifying a different race: at the end of btrfs_qgroup_rescan_worker, we clear the rescan status flag, drop the lock, commit the status item transaction, and then update ->qgroup_rescan_running. If a rescan is requested before the lock is reacquired, we'll try to start it up and then hit that warning.
So, the warning is doing its job. Please hold off on merging this patch.
IMO the root cause is overloading fs_info->qgroup_flags to correspond to the on-disk item and control runtime behavior. I've been meaning to fix that for a while, so I'll do that now.
-Jeff
Signature · 2 lines
Jeff Mahoney SUSE Labs