[PATCH v2 0/2] btrfs: zoned: mark relocation as writing

[PATCH v2 0/2] btrfs: zoned: mark relocation as writing

[Naohiro Aota]

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(-)

-- 
2.35.1

[PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Naohiro Aota]

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().

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 include/linux/fs.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbf812ce89a8..5d5dc9a276d9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -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);
 
-- 
2.35.1

Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Dave Chinner]

On Thu, Feb 10, 2022 at 02:59:04PM +0900, Naohiro Aota wrote:
> 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().
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  include/linux/fs.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbf812ce89a8..5d5dc9a276d9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Damien Le Moal]

On 2/15/22 06:35, Dave Chinner wrote:
> On Thu, Feb 10, 2022 at 02:59:04PM +0900, Naohiro Aota wrote:
>> 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().
>>
>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>> ---
>>  include/linux/fs.h | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index bbf812ce89a8..5d5dc9a276d9 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -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);
> }

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.

> 
> 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.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Dave Chinner]

On Tue, Feb 15, 2022 at 07:49:27AM +0900, Damien Le Moal wrote:
> On 2/15/22 06:35, Dave Chinner wrote:
> > On Thu, Feb 10, 2022 at 02:59:04PM +0900, Naohiro Aota wrote:
> >> 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().
> >>
> >> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> >> ---
> >>  include/linux/fs.h | 20 ++++++++++++++++++++
> >>  1 file changed, 20 insertions(+)
> >>
> >> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >> index bbf812ce89a8..5d5dc9a276d9 100644
> >> --- a/include/linux/fs.h
> >> +++ b/include/linux/fs.h
> >> @@ -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);
> > }
> 
> 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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Damien Le Moal]

On 2/15/22 09:05, Dave Chinner wrote:
> On Tue, Feb 15, 2022 at 07:49:27AM +0900, Damien Le Moal wrote:
>> On 2/15/22 06:35, Dave Chinner wrote:
>>> On Thu, Feb 10, 2022 at 02:59:04PM +0900, Naohiro Aota wrote:
>>>> 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().
>>>>
>>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>>> ---
>>>>  include/linux/fs.h | 20 ++++++++++++++++++++
>>>>  1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>> index bbf812ce89a8..5d5dc9a276d9 100644
>>>> --- a/include/linux/fs.h
>>>> +++ b/include/linux/fs.h
>>>> @@ -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);
>>> }
>>
>> 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.

> 
> Cheers,
> 
> Dave.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Naohiro Aota]

On Tue, Feb 15, 2022 at 11:05:15AM +1100, Dave Chinner wrote:
> On Tue, Feb 15, 2022 at 07:49:27AM +0900, Damien Le Moal wrote:
> > On 2/15/22 06:35, Dave Chinner wrote:
> > > On Thu, Feb 10, 2022 at 02:59:04PM +0900, Naohiro Aota wrote:
> > >> 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().
> > >>
> > >> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > >> ---
> > >>  include/linux/fs.h | 20 ++++++++++++++++++++
> > >>  1 file changed, 20 insertions(+)
> > >>
> > >> diff --git a/include/linux/fs.h b/include/linux/fs.h
> > >> index bbf812ce89a8..5d5dc9a276d9 100644
> > >> --- a/include/linux/fs.h
> > >> +++ b/include/linux/fs.h
> > >> @@ -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);
> > > }
> > 
> > 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.

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH v2 2/2] btrfs: zoned: mark relocation as writing

[Naohiro Aota]

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().

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.

Fixes: 18bb8bbf13c1 ("btrfs: zoned: automatically reclaim zones")
Cc: stable@vger.kernel.org # 5.13+
Link: https://github.com/naota/linux/issues/56
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/block-group.c | 8 +++++++-
 fs/btrfs/volumes.c     | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 68feabc83a27..f9ca58a90630 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -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)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b07d382d53a8..e415fb4a698e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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;
 }
-- 
2.35.1

Re: [PATCH v2 0/2] btrfs: zoned: mark relocation as writing

[Johannes Thumshirn]

On 10/02/2022 06:59, Naohiro Aota wrote:
> 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(-)
> 

For the series:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 0/2] btrfs: zoned: mark relocation as writing

[David Sterba]

On Thu, Feb 10, 2022 at 02:59:03PM +0900, Naohiro Aota wrote:
> 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

Added as topic branch to for-next. I'd appreciate acks for the
sb_start_* helpers before the patches go to to Linus' tree.