| Message ID | 1541591010-29789-7-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix replace-start and replace-cancel racing | expand |
On 7.11.18 г. 13:43 ч., Anand Jain wrote: > + /* scrub for replace must not be running in suspended state */ > + if (btrfs_scrub_cancel(fs_info) != -ENOTCONN) > + ASSERT(0); ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN)
On 11/07/2018 08:19 PM, Nikolay Borisov wrote: > > > On 7.11.18 г. 13:43 ч., Anand Jain wrote: >> + /* scrub for replace must not be running in suspended state */ >> + if (btrfs_scrub_cancel(fs_info) != -ENOTCONN) >> + ASSERT(0); > > ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN) > There will be substantial difference in code when compiled with and without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info) won't be run at all, I would like to keep it as it is. [1] ------ ./fs/btrfs/ctree.h #ifdef CONFIG_BTRFS_ASSERT __cold static inline void assfail(const char *expr, const char *file, int line) { pr_err("assertion failed: %s, file: %s, line: %d\n", expr, file, line); BUG(); } #define ASSERT(expr) \ (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) #else #define ASSERT(expr) ((void)0) #endif ------- Thanks, Anand
On 8.11.18 г. 10:33 ч., Anand Jain wrote: > > > On 11/07/2018 08:19 PM, Nikolay Borisov wrote: >> >> >> On 7.11.18 г. 13:43 ч., Anand Jain wrote: >>> + /* scrub for replace must not be running in suspended state */ >>> + if (btrfs_scrub_cancel(fs_info) != -ENOTCONN) >>> + ASSERT(0); >> >> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN) >> > > There will be substantial difference in code when compiled with and > without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info) > won't be run at all, I would like to keep it as it is. Fair point, in that case do: ret = btrfs_scrub_cancel(fs_info); ASSERT(ret != -ENOTCONN); result > > [1] > ------ > ./fs/btrfs/ctree.h > #ifdef CONFIG_BTRFS_ASSERT > > __cold > static inline void assfail(const char *expr, const char *file, int line) > { > pr_err("assertion failed: %s, file: %s, line: %d\n", > expr, file, line); > BUG(); > } > > #define ASSERT(expr) \ > (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) > #else > #define ASSERT(expr) ((void)0) > #endif > ------- > > Thanks, Anand > >
On 11/08/2018 04:52 PM, Nikolay Borisov wrote: > > > On 8.11.18 г. 10:33 ч., Anand Jain wrote: >> >> >> On 11/07/2018 08:19 PM, Nikolay Borisov wrote: >>> >>> >>> On 7.11.18 г. 13:43 ч., Anand Jain wrote: >>>> + /* scrub for replace must not be running in suspended state */ >>>> + if (btrfs_scrub_cancel(fs_info) != -ENOTCONN) >>>> + ASSERT(0); >>> >>> ASSERT(btrfs_scrub_cancel(fs_info) == -ENOTCONN) >>> >> >> There will be substantial difference in code when compiled with and >> without CONFIG_BTRFS_ASSERT [1]. That is, btrfs_scrub_cancel(fs_info) >> won't be run at all, I would like to keep it as it is. > > Fair point, in that case do: > > ret = btrfs_scrub_cancel(fs_info); > ASSERT(ret != -ENOTCONN); Fixed. Thanks, Anand > result > > >> >> [1] >> ------ >> ./fs/btrfs/ctree.h >> #ifdef CONFIG_BTRFS_ASSERT >> >> __cold >> static inline void assfail(const char *expr, const char *file, int line) >> { >> pr_err("assertion failed: %s, file: %s, line: %d\n", >> expr, file, line); >> BUG(); >> } >> >> #define ASSERT(expr) \ >> (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) >> #else >> #define ASSERT(expr) ((void)0) >> #endif >> ------- >> >> Thanks, Anand >> >>
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index c092ed559714..dae2b920f1a9 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -831,7 +831,9 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info) btrfs_dev_replace_write_unlock(dev_replace); - btrfs_scrub_cancel(fs_info); + /* scrub for replace must not be running in suspended state */ + if (btrfs_scrub_cancel(fs_info) != -ENOTCONN) + ASSERT(0); trans = btrfs_start_transaction(root, 0); if (IS_ERR(trans)) {
When the replace state is placed in the suspended state, btrfs_scrub_cancel() should fail with -ENOTCONN as there is no scrub running, as a safety catch check if btrfs_scrub_cancel() returns -ENOTCONN and assert if it doesn't. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/dev-replace.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)