mbox series

[v2,0/4] btrfs: fix unexpected delayed iputs at umount time and cleanups

Message ID cover.1741198394.git.fdmanana@suse.com (mailing list archive)
Headers show
Series btrfs: fix unexpected delayed iputs at umount time and cleanups | expand

Message

Filipe Manana March 5, 2025, 6:17 p.m. UTC
From: Filipe Manana <fdmanana@suse.com>

Fix a couple races that can result in adding delayed iputs during umount after
we no longer expect to find any, triggering an assertion failure. Plus a couple
cleanups. Details in the change logs.

V2: Removed the NULL checks for the workqueues in patches 1 and 2, as they
    can never be NULL while at close_ctree() (they can only be NULL in error
    paths from open_ctree()).

Filipe Manana (4):
  btrfs: fix non-empty delayed iputs list on unmount due to endio workers
  btrfs: fix non-empty delayed iputs list on unmount due to compressed write workers
  btrfs: move __btrfs_bio_end_io() code into its single caller
  btrfs: move btrfs_cleanup_bio() code into its single caller

 fs/btrfs/bio.c     | 36 ++++++++++++++----------------------
 fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+), 22 deletions(-)

Comments

Qu Wenruo March 5, 2025, 10:54 p.m. UTC | #1
在 2025/3/6 04:47, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Fix a couple races that can result in adding delayed iputs during umount after
> we no longer expect to find any, triggering an assertion failure. Plus a couple
> cleanups. Details in the change logs.
> 
> V2: Removed the NULL checks for the workqueues in patches 1 and 2, as they
>      can never be NULL while at close_ctree() (they can only be NULL in error
>      paths from open_ctree()).

Commented on the first patch that the fixed tag should be the uncached 
io enablement.
Since before that we only handle data read operations in endio_workers, 
which should not get ordered extent involved at all.

(Thankfully we get that feature disabled for now)

Otherwise looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> 
> Filipe Manana (4):
>    btrfs: fix non-empty delayed iputs list on unmount due to endio workers
>    btrfs: fix non-empty delayed iputs list on unmount due to compressed write workers
>    btrfs: move __btrfs_bio_end_io() code into its single caller
>    btrfs: move btrfs_cleanup_bio() code into its single caller
> 
>   fs/btrfs/bio.c     | 36 ++++++++++++++----------------------
>   fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
>   2 files changed, 35 insertions(+), 22 deletions(-)
>
Qu Wenruo March 6, 2025, 10:09 a.m. UTC | #2
在 2025/3/6 04:47, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
>
> Fix a couple races that can result in adding delayed iputs during umount after
> we no longer expect to find any, triggering an assertion failure. Plus a couple
> cleanups. Details in the change logs.
>
> V2: Removed the NULL checks for the workqueues in patches 1 and 2, as they
>      can never be NULL while at close_ctree() (they can only be NULL in error
>      paths from open_ctree()).

BTW, even with this series applied, I can still trigger the delayed iput
ASSERT():

[ 5364.406135] BTRFS warning (device dm-10 state EA): direct IO failed
ino 259 op 0x0 offset 0x14800 len 18432 err no 10
[ 5364.406327] BTRFS warning (device dm-10 state EA): direct IO failed
ino 301 op 0x0 offset 0x112000 len 12288 err no 10
[ 5364.406443] BTRFS warning (device dm-10 state EA): direct IO failed
ino 284 op 0x0 offset 0x129000 len 40960 err no 10
[ 5364.408115] BTRFS warning (device dm-10 state EA): direct IO failed
ino 333 op 0x0 offset 0x43000 len 2048 err no 10
[ 5364.408350] BTRFS warning (device dm-10 state EA): direct IO failed
ino 7914 op 0x0 offset 0x34a000 len 43008 err no 10
[ 5364.636270] BTRFS info (device dm-10 state EA): last unmount of
filesystem 9c4c225e-d4c6-43d0-b8c9-4b3afb5cb3cc
[ 5364.639881] assertion failed: list_empty(&fs_info->delayed_iputs), in
fs/btrfs/disk-io.c:4419
[ 5364.641814] ------------[ cut here ]------------
[ 5364.642733] kernel BUG at fs/btrfs/disk-io.c:4419!
[ 5364.643712] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[ 5364.644880] CPU: 5 UID: 0 PID: 2672520 Comm: umount Tainted: G
W          6.14.0-rc5-custom+ #224
[ 5364.646787] Tainted: [W]=WARN

I have hit this at least twice, on x86_64 with the new experimental 2K
block size.

And this is the latest for-next, which does not have the uncached IO
patch at all.

Since I do not have compression enable for the mount options, I believe
there is some extra causes.

Thanks,
Qu

>
> Filipe Manana (4):
>    btrfs: fix non-empty delayed iputs list on unmount due to endio workers
>    btrfs: fix non-empty delayed iputs list on unmount due to compressed write workers
>    btrfs: move __btrfs_bio_end_io() code into its single caller
>    btrfs: move btrfs_cleanup_bio() code into its single caller
>
>   fs/btrfs/bio.c     | 36 ++++++++++++++----------------------
>   fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
>   2 files changed, 35 insertions(+), 22 deletions(-)
>
Filipe Manana March 6, 2025, 4 p.m. UTC | #3
On Thu, Mar 6, 2025 at 10:09 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2025/3/6 04:47, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > Fix a couple races that can result in adding delayed iputs during umount after
> > we no longer expect to find any, triggering an assertion failure. Plus a couple
> > cleanups. Details in the change logs.
> >
> > V2: Removed the NULL checks for the workqueues in patches 1 and 2, as they
> >      can never be NULL while at close_ctree() (they can only be NULL in error
> >      paths from open_ctree()).
>
> BTW, even with this series applied, I can still trigger the delayed iput
> ASSERT():
>
> [ 5364.406135] BTRFS warning (device dm-10 state EA): direct IO failed
> ino 259 op 0x0 offset 0x14800 len 18432 err no 10
> [ 5364.406327] BTRFS warning (device dm-10 state EA): direct IO failed
> ino 301 op 0x0 offset 0x112000 len 12288 err no 10
> [ 5364.406443] BTRFS warning (device dm-10 state EA): direct IO failed
> ino 284 op 0x0 offset 0x129000 len 40960 err no 10
> [ 5364.408115] BTRFS warning (device dm-10 state EA): direct IO failed
> ino 333 op 0x0 offset 0x43000 len 2048 err no 10
> [ 5364.408350] BTRFS warning (device dm-10 state EA): direct IO failed
> ino 7914 op 0x0 offset 0x34a000 len 43008 err no 10
> [ 5364.636270] BTRFS info (device dm-10 state EA): last unmount of
> filesystem 9c4c225e-d4c6-43d0-b8c9-4b3afb5cb3cc
> [ 5364.639881] assertion failed: list_empty(&fs_info->delayed_iputs), in
> fs/btrfs/disk-io.c:4419
> [ 5364.641814] ------------[ cut here ]------------
> [ 5364.642733] kernel BUG at fs/btrfs/disk-io.c:4419!
> [ 5364.643712] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [ 5364.644880] CPU: 5 UID: 0 PID: 2672520 Comm: umount Tainted: G
> W          6.14.0-rc5-custom+ #224
> [ 5364.646787] Tainted: [W]=WARN
>
> I have hit this at least twice, on x86_64 with the new experimental 2K
> block size.
>
> And this is the latest for-next, which does not have the uncached IO
> patch at all.
>
> Since I do not have compression enable for the mount options, I believe
> there is some extra causes.

There is, in case of IO errors we can add delayed iputs from another queue,
which makes sense since generic/648 exercises IO error simulation with dmerror,
and your dmesg also shows IO errors. Patch here:

https://lore.kernel.org/linux-btrfs/b07f13dbfadfdb5884b21b97bbf1316c45d06a32.1741272910.git.fdmanana@suse.com/

Thanks.

>
> Thanks,
> Qu
>
> >
> > Filipe Manana (4):
> >    btrfs: fix non-empty delayed iputs list on unmount due to endio workers
> >    btrfs: fix non-empty delayed iputs list on unmount due to compressed write workers
> >    btrfs: move __btrfs_bio_end_io() code into its single caller
> >    btrfs: move btrfs_cleanup_bio() code into its single caller
> >
> >   fs/btrfs/bio.c     | 36 ++++++++++++++----------------------
> >   fs/btrfs/disk-io.c | 21 +++++++++++++++++++++
> >   2 files changed, 35 insertions(+), 22 deletions(-)
> >
>