btrfs: zoned: mark relocation as writing
Subsystems (3)
v2 cover btrfs: zoned: mark relocation as writing
There is a hung_task issue with running generic/068 on an SMR device. The hang occurs while a process is trying to thaw the filesystem. The process is trying to take sb->s_umount to thaw the FS. The lock is held by fsstress, which calls btrfs_sync_fs() and is waiting for an ordered extent to finish. However, as the FS is frozen, the ordered extent never finish.
Having an ordered extent while the FS is frozen is the root cause of the hang. The ordered extent is initiated from btrfs_relocate_chunk() which is called from btrfs_reclaim_bgs_work().
The first patch is a preparation patch to add asserting functions to check if sb_start_{write,pagefault,intwrite} is called.
The second patch adds sb_{start,end}_write and the assert function at proper places.
Changelog:
v2:
- Implement asserting functions not to directly touch the internal implementation
Naohiro Aota (2):
fs: add asserting functions for sb_start_{write,pagefault,intwrite}
btrfs: zoned: mark relocation as writing
fs/btrfs/block-group.c | 8 +++++++- fs/btrfs/volumes.c | 6 ++++++ include/linux/fs.h | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-)
Signature · 1 line
2.35.1
PATCH v2 1/2 fs: add asserting functions for sb_start_{write,pagefault,intwrite}
Add an assert function sb_assert_write_started() to check if sb_start_write() is properly called. It is used in the next commit.
Also, add the assert functions for sb_start_pagefault() and sb_start_intwrite().
--- include/linux/fs.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
include/linux/fs.h +20 -0
@@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)#define __sb_writers_release (sb, lev) \ percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_) +static inline void __sb_assert_write_started (struct super_block *sb,int level) +{ +lockdep_assert_held_read (sb->s_writers.rw_sem + level -1 ); +} +/** * sb_end_write - drop write access to a superblock * @sb: the super we wrote to
@@ -1885,6 +1890,11 @@ static inline bool sb_start_write_trylock(struct super_block *sb)return __sb_start_write_trylock (sb,SB_FREEZE_WRITE ); } +static inline void sb_assert_write_started (struct super_block *sb) +{ +__sb_assert_write_started (sb,SB_FREEZE_WRITE ); +} +/** * sb_start_pagefault - get write access to a superblock from a page fault * @sb: the super we write to
@@ -1909,6 +1919,11 @@ static inline void sb_start_pagefault(struct super_block *sb)__sb_start_write (sb,SB_FREEZE_PAGEFAULT ); } +static inline void sb_assert_pagefault_started (struct super_block *sb) +{ +__sb_assert_write_started (sb,SB_FREEZE_PAGEFAULT ); +} +/** * sb_start_intwrite - get write access to a superblock for internal fs purposes * @sb: the super we write to
@@ -1932,6 +1947,11 @@ static inline bool sb_start_intwrite_trylock(struct super_block *sb)return __sb_start_write_trylock (sb,SB_FREEZE_FS ); } +static inline void sb_assert_intwrite_started (struct super_block *sb) +{ +__sb_assert_write_started (sb,SB_FREEZE_FS ); +} +bool inode_owner_or_capable (struct user_namespace *mnt_userns,const struct inode *inode);
Signature · 1 line
2.35.1
Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}
Quoted 16 lines · Naohiro Aota
Add an assert function
sb_assert_write_started()to check ifsb_start_write()is properly called. It is used in the next commit.Also, add the assert functions for
sb_start_pagefault()andsb_start_intwrite().Signed-off-by Naohiro Aota <[email protected]>--- include/linux/fs.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)include/linux/fs.h +0
-0
include/linux/fs.h static inline bool __sb_start_write_trylock(struct super_block *sb, int level) · line 1827
Quoted 8 lines
include/linux/fs.h +5
-0@@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)#define __sb_writers_release (sb, lev) \ percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_) +static inline void __sb_assert_write_started (struct super_block *sb,int level) +{ +lockdep_assert_held_read (sb->s_writers.rw_sem + level -1 ); +} +
So this isn't an assert, it's a WARN_ON(). Asserts stop execution (i.e. kill the task) rather than just issue a warning, so let's not name a function that issues a warning "assert"...
Hence I'd much rather see this implemented as:
static inline bool __sb_write_held(struct super_block *sb, int level)
{
return lockdep_is_held_type(sb->s_writers.rw_sem + level - 1, 1);
}i.e. named similar to __sb_start_write/__sb_end_write, with similar wrappers for pagefault/intwrite, and it just returns a bool status that lets the caller do what it wants with the status (warn, bug, etc).
Then in the code that needs to check if the right freeze levels are held simply need to do:
WARN_ON(!sb_write_held(sb));
in which case it's self documenting in the code that cares about this and it's also obvious to anyone debugging such a message where it came from and what constraint got violated...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}
Quoted 30 lines · Dave Chinner
Quoted 16 lines · Naohiro Aota
Add an assert function
sb_assert_write_started()to check ifsb_start_write()is properly called. It is used in the next commit.Also, add the assert functions for
sb_start_pagefault()andsb_start_intwrite().Signed-off-by Naohiro Aota <[email protected]>--- include/linux/fs.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)include/linux/fs.h +0
-0include/linux/fs.h static inline bool __sb_start_write_trylock(struct super_block *sb, int level) · line 1827
Quoted 8 lines
include/linux/fs.h +5
-0@@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)#define __sb_writers_release (sb, lev) \ percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_) +static inline void __sb_assert_write_started (struct super_block *sb,int level) +{ +lockdep_assert_held_read (sb->s_writers.rw_sem + level -1 ); +} +So this isn't an assert, it's a
WARN_ON(). Asserts stop execution (i.e. kill the task) rather than just issue a warning, so let's not name a function that issues a warning "assert"...
Quoted 6 lines
Hence I'd much rather see this implemented as:
static inline bool
__sb_write_held(struct super_block *sb, int level){ return lockdep_is_held_type(sb->s_writers.rw_sem + level - 1, 1); }
Since this would be true when called in between __sb_start_write() and __sb_end_write(), what about calling it __sb_write_started() ? That disconnects from the fact that the implementation uses a sem.
Quoted 18 lines
i.e. named similar to __sb_start_write/__sb_end_write, with similar wrappers for pagefault/intwrite, and it just returns a bool status that lets the caller do what it wants with the status (warn, bug, etc).
Then in the code that needs to check if the right freeze levels are held simply need to do:
WARN_ON(!sb_write_held(sb));in which case it's self documenting in the code that cares about this and it's also obvious to anyone debugging such a message where it came from and what constraint got violated...
Cheers,
Dave.
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}
Quoted 33 lines · Damien Le Moal
Quoted 32 lines · Dave Chinner
Quoted 16 lines · Naohiro Aota
Add an assert function
sb_assert_write_started()to check ifsb_start_write()is properly called. It is used in the next commit.Also, add the assert functions for
sb_start_pagefault()andsb_start_intwrite().Signed-off-by Naohiro Aota <[email protected]>--- include/linux/fs.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)include/linux/fs.h +0
-0include/linux/fs.h static inline bool __sb_start_write_trylock(struct super_block *sb, int level) · line 1827
Quoted 8 lines
include/linux/fs.h +5
-0@@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)#define __sb_writers_release (sb, lev) \ percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_) +static inline void __sb_assert_write_started (struct super_block *sb,int level) +{ +lockdep_assert_held_read (sb->s_writers.rw_sem + level -1 ); +} +So this isn't an assert, it's a
WARN_ON(). Asserts stop execution (i.e. kill the task) rather than just issue a warning, so let's not name a function that issues a warning "assert"...Hence I'd much rather see this implemented as:
Quoted 8 lines
Quoted 4 lines
static inline bool
__sb_write_held(struct super_block *sb, int level){ return lockdep_is_held_type(sb->s_writers.rw_sem + level - 1, 1); }Since this would be true when called in between
__sb_start_write()and__sb_end_write(), what about calling it__sb_write_started()? That disconnects from the fact that the implementation uses a sem.
Makes no difference to me; I initially was going to suggest *_inprogress() but that seemed a bit verbose. We don't need to bikeshed this to death - all I want is it to be a check that can be used for generic purposes rather than being an explicit assert.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}
Quoted 39 lines · Dave Chinner
Quoted 38 lines · Damien Le Moal
Quoted 36 lines · Dave Chinner
Quoted 16 lines · Naohiro Aota
Add an assert function
sb_assert_write_started()to check ifsb_start_write()is properly called. It is used in the next commit.Also, add the assert functions for
sb_start_pagefault()andsb_start_intwrite().Signed-off-by Naohiro Aota <[email protected]>--- include/linux/fs.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)include/linux/fs.h +0
-0include/linux/fs.h static inline bool __sb_start_write_trylock(struct super_block *sb, int level) · line 1827
Quoted 8 lines
include/linux/fs.h +5
-0@@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)#define __sb_writers_release (sb, lev) \ percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_) +static inline void __sb_assert_write_started (struct super_block *sb,int level) +{ +lockdep_assert_held_read (sb->s_writers.rw_sem + level -1 ); +} +So this isn't an assert, it's a
WARN_ON(). Asserts stop execution (i.e. kill the task) rather than just issue a warning, so let's not name a function that issues a warning "assert"...Hence I'd much rather see this implemented as:
static inline bool
__sb_write_held(struct super_block *sb, int level){ return lockdep_is_held_type(sb->s_writers.rw_sem + level - 1, 1); }
Quoted 8 lines
Quoted 3 lines
Since this would be true when called in between
__sb_start_write()and__sb_end_write(), what about calling it__sb_write_started()? That disconnects from the fact that the implementation uses a sem.Makes no difference to me; I initially was going to suggest
*_inprogress()but that seemed a bit verbose. We don't need to bikeshed this to death - all I want is it to be a check that can be used for generic purposes rather than being an explicit assert.
agree.
Quoted 4 lines
Cheers,
Dave.
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}
In reply to Dave ChinnerQuoted 39 lines · Dave Chinner
Quoted 38 lines · Damien Le Moal
Quoted 36 lines · Dave Chinner
Quoted 16 lines · Naohiro Aota
Add an assert function
sb_assert_write_started()to check ifsb_start_write()is properly called. It is used in the next commit.Also, add the assert functions for
sb_start_pagefault()andsb_start_intwrite().Signed-off-by Naohiro Aota <[email protected]>--- include/linux/fs.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)include/linux/fs.h +0
-0include/linux/fs.h static inline bool __sb_start_write_trylock(struct super_block *sb, int level) · line 1827
Quoted 8 lines
include/linux/fs.h +5
-0@@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)#define __sb_writers_release (sb, lev) \ percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_) +static inline void __sb_assert_write_started (struct super_block *sb,int level) +{ +lockdep_assert_held_read (sb->s_writers.rw_sem + level -1 ); +} +So this isn't an assert, it's a
WARN_ON(). Asserts stop execution (i.e. kill the task) rather than just issue a warning, so let's not name a function that issues a warning "assert"...Hence I'd much rather see this implemented as:
static inline bool
__sb_write_held(struct super_block *sb, int level){ return lockdep_is_held_type(sb->s_writers.rw_sem + level - 1, 1); }
Quoted 8 lines
Quoted 3 lines
Since this would be true when called in between
__sb_start_write()and__sb_end_write(), what about calling it__sb_write_started()? That disconnects from the fact that the implementation uses a sem.Makes no difference to me; I initially was going to suggest
*_inprogress()but that seemed a bit verbose. We don't need to bikeshed this to death - all I want is it to be a check that can be used for generic purposes rather than being an explicit assert.
Agree. I'd like to use __sb_write_started() as it is conforming to other functions.
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH v2 2/2 btrfs: zoned: mark relocation as writing
In reply to Naohiro AotaThere is a hung_task issue with running generic/068 on an SMR device. The hang occurs while a process is trying to thaw the filesystem. The process is trying to take sb->s_umount to thaw the FS. The lock is held by fsstress, which calls btrfs_sync_fs() and is waiting for an ordered extent to finish. However, as the FS is frozen, the ordered extent never finish.
Having an ordered extent while the FS is frozen is the root cause of the hang. The ordered extent is initiated from btrfs_relocate_chunk() which is called from btrfs_reclaim_bgs_work().
This commit add sb_*_write() around btrfs_relocate_chunk() call site. For the usual "btrfs balance" command, we already call it with mnt_want_file() in btrfs_ioctl_balance().
Additionally, add an ASSERT in btrfs_relocate_chunk() to check it is properly called.
--- fs/btrfs/block-group.c | 8 +++++++- fs/btrfs/volumes.c | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-)
fs/btrfs/block-group.c +7 -1
@@ -1513,8 +1513,12 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)if (!test_bit (BTRFS_FS_OPEN , &fs_info->flags))return ; -if (!btrfs_exclop_start (fs_info,BTRFS_EXCLOP_BALANCE )) +sb_start_write (fs_info->sb); + +if (!btrfs_exclop_start (fs_info,BTRFS_EXCLOP_BALANCE )) { +sb_end_write (fs_info->sb);return ; + }/* * Long running balances can keep us blocked here for eternity, so
@@ -1522,6 +1526,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)*/ if (!mutex_trylock (&fs_info->reclaim_bgs_lock)) {btrfs_exclop_finish (fs_info); +sb_end_write (fs_info->sb);return ; }
@@ -1596,6 +1601,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)spin_unlock (&fs_info->unused_bgs_lock);mutex_unlock (&fs_info->reclaim_bgs_lock);btrfs_exclop_finish (fs_info); +sb_end_write (fs_info->sb); }void btrfs_reclaim_bgs (struct btrfs_fs_info *fs_info)
fs/btrfs/volumes.c +6 -0
@@ -3251,6 +3251,9 @@ int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset)u64 length;int ret; +/* Assert we called sb_start_write(), not to race with FS freezing */ +sb_assert_write_started (fs_info->sb); +/* * Prevent races with automatic removal of unused block groups. * After we relocate and before we remove the chunk with offset
@@ -8299,10 +8302,12 @@ static int relocating_repair_kthread(void *data) target = cache->start;btrfs_put_block_group (cache); +sb_start_write (fs_info->sb);if (!btrfs_exclop_start (fs_info,BTRFS_EXCLOP_BALANCE )) {btrfs_info (fs_info,"zoned: skip relocating block group %llu to repair: EBUSY" , target); +sb_end_write (fs_info->sb);return -EBUSY ; }
@@ -8330,6 +8335,7 @@ static int relocating_repair_kthread(void *data)btrfs_put_block_group (cache);mutex_unlock (&fs_info->reclaim_bgs_lock);btrfs_exclop_finish (fs_info); +sb_end_write (fs_info->sb);return ret; }
Signature · 1 line
2.35.1
Quoted 22 lines · Naohiro Aota
There is a
hung_taskissue with running generic/068 on an SMR device. The hang occurs while a process is trying to thaw the filesystem. The process is trying to takesb->s_umountto thaw the FS. The lock is held by fsstress, which callsbtrfs_sync_fs()and is waiting for an ordered extent to finish. However, as the FS is frozen, the ordered extent never finish.Having an ordered extent while the FS is frozen is the root cause of the hang. The ordered extent is initiated from
btrfs_relocate_chunk()which is called frombtrfs_reclaim_bgs_work().The first patch is a preparation patch to add asserting functions to check if sb_start_{write,pagefault,intwrite} is called.
The second patch adds sb_{start,end}_write and the assert function at proper places.
Changelog:
v2:
- Implement asserting functions not to directly touch the internal implementation
Quoted 9 lines
Naohiro Aota (2):
fs: add asserting functions for sb_start_{write,pagefault,intwrite}
btrfs: zoned: mark relocation as writingfs/btrfs/block-group.c | 8 +++++++- fs/btrfs/volumes.c | 6 ++++++ include/linux/fs.h | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-)
For the series:
Quoted 17 lines · Naohiro Aota
There is a
hung_taskissue with running generic/068 on an SMR device. The hang occurs while a process is trying to thaw the filesystem. The process is trying to takesb->s_umountto thaw the FS. The lock is held by fsstress, which callsbtrfs_sync_fs()and is waiting for an ordered extent to finish. However, as the FS is frozen, the ordered extent never finish.Having an ordered extent while the FS is frozen is the root cause of the hang. The ordered extent is initiated from
btrfs_relocate_chunk()which is called frombtrfs_reclaim_bgs_work().The first patch is a preparation patch to add asserting functions to check if sb_start_{write,pagefault,intwrite} is called.
The second patch adds sb_{start,end}_write and the assert function at proper places.
Quoted 8 lines
Changelog:
v2:
- Implement asserting functions not to directly touch the internal implementationNaohiro Aota (2):
fs: add asserting functions for sb_start_{write,pagefault,intwrite}
btrfs: zoned: mark relocation as writing
Added as topic branch to for-next. I'd appreciate acks for the sb_start_* helpers before the patches go to to Linus' tree.