| Message ID | 95ce11234dd6911a433b1a016e4d4194856212b5.1638523623.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: free exchange changeset on failures | expand |
On Fri, Dec 03, 2021 at 02:55:33AM -0800, Johannes Thumshirn wrote: > Fstests runs on my VMs have show several kmemleak reports like the following. > > unreferenced object 0xffff88811ae59080 (size 64): > comm "xfs_io", pid 12124, jiffies 4294987392 (age 6.368s) > hex dump (first 32 bytes): > 00 c0 1c 00 00 00 00 00 ff cf 1c 00 00 00 00 00 ................ > 90 97 e5 1a 81 88 ff ff 90 97 e5 1a 81 88 ff ff ................ > backtrace: > [<00000000ac0176d2>] ulist_add_merge+0x60/0x150 [btrfs] > [<0000000076e9f312>] set_state_bits+0x86/0xc0 [btrfs] > [<0000000014fe73d6>] set_extent_bit+0x270/0x690 [btrfs] > [<000000004f675208>] set_record_extent_bits+0x19/0x20 [btrfs] > [<00000000b96137b1>] qgroup_reserve_data+0x274/0x310 [btrfs] > [<0000000057e9dcbb>] btrfs_check_data_free_space+0x5c/0xa0 [btrfs] > [<0000000019c4511d>] btrfs_delalloc_reserve_space+0x1b/0xa0 [btrfs] > [<000000006d37e007>] btrfs_dio_iomap_begin+0x415/0x970 [btrfs] > [<00000000fb8a74b8>] iomap_iter+0x161/0x1e0 > [<0000000071dff6ff>] __iomap_dio_rw+0x1df/0x700 > [<000000002567ba53>] iomap_dio_rw+0x5/0x20 > [<0000000072e555f8>] btrfs_file_write_iter+0x290/0x530 [btrfs] > [<000000005eb3d845>] new_sync_write+0x106/0x180 > [<000000003fb505bf>] vfs_write+0x24d/0x2f0 > [<000000009bb57d37>] __x64_sys_pwrite64+0x69/0xa0 > [<000000003eba3fdf>] do_syscall_64+0x43/0x90 > > In case brtfs_qgroup_reserve_data() or btrfs_delalloc_reserve_metadata() > fail the allocated extent_changeset will not be freed. > > So in btrfs_check_data_free_space() and btrfs_delalloc_reserve_space() > free the allocated extent_changeset to get rid of the allocated memory. > > Cc: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Looks good, and it ran successfully on my fstests run with kmemleak as well. Just a note worth adding, is that the issue currently only happens in the direct IO write path, but only after my change "btrfs: fix ENOSPC failure when attempting direct IO write into NOCOW range", and also at defrag_one_locked_target() (haven't checked since when). Every other place is always calling extent_changeset_free() even if its call to btrfs_delalloc_reserve_space() or btrfs_check_data_free_space() has failed. With that, which probably David can add, it looks good: Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > --- > fs/btrfs/delalloc-space.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/delalloc-space.c b/fs/btrfs/delalloc-space.c > index bca438c7c972..fb46a28f5065 100644 > --- a/fs/btrfs/delalloc-space.c > +++ b/fs/btrfs/delalloc-space.c > @@ -143,10 +143,13 @@ int btrfs_check_data_free_space(struct btrfs_inode *inode, > > /* Use new btrfs_qgroup_reserve_data to reserve precious data space. */ > ret = btrfs_qgroup_reserve_data(inode, reserved, start, len); > - if (ret < 0) > + if (ret < 0) { > btrfs_free_reserved_data_space_noquota(fs_info, len); > - else > + extent_changeset_free(*reserved); > + *reserved = NULL; > + } else { > ret = 0; > + } > return ret; > } > > @@ -452,8 +455,11 @@ int btrfs_delalloc_reserve_space(struct btrfs_inode *inode, > if (ret < 0) > return ret; > ret = btrfs_delalloc_reserve_metadata(inode, len); > - if (ret < 0) > + if (ret < 0) { > btrfs_free_reserved_data_space(inode, *reserved, start, len); > + extent_changeset_free(*reserved); > + *reserved = NULL; > + } > return ret; > } > > -- > 2.31.1 >
On Fri, Dec 03, 2021 at 02:55:33AM -0800, Johannes Thumshirn wrote: > Fstests runs on my VMs have show several kmemleak reports like the following. > > unreferenced object 0xffff88811ae59080 (size 64): > comm "xfs_io", pid 12124, jiffies 4294987392 (age 6.368s) > hex dump (first 32 bytes): > 00 c0 1c 00 00 00 00 00 ff cf 1c 00 00 00 00 00 ................ > 90 97 e5 1a 81 88 ff ff 90 97 e5 1a 81 88 ff ff ................ > backtrace: > [<00000000ac0176d2>] ulist_add_merge+0x60/0x150 [btrfs] > [<0000000076e9f312>] set_state_bits+0x86/0xc0 [btrfs] > [<0000000014fe73d6>] set_extent_bit+0x270/0x690 [btrfs] > [<000000004f675208>] set_record_extent_bits+0x19/0x20 [btrfs] > [<00000000b96137b1>] qgroup_reserve_data+0x274/0x310 [btrfs] > [<0000000057e9dcbb>] btrfs_check_data_free_space+0x5c/0xa0 [btrfs] > [<0000000019c4511d>] btrfs_delalloc_reserve_space+0x1b/0xa0 [btrfs] > [<000000006d37e007>] btrfs_dio_iomap_begin+0x415/0x970 [btrfs] > [<00000000fb8a74b8>] iomap_iter+0x161/0x1e0 > [<0000000071dff6ff>] __iomap_dio_rw+0x1df/0x700 > [<000000002567ba53>] iomap_dio_rw+0x5/0x20 > [<0000000072e555f8>] btrfs_file_write_iter+0x290/0x530 [btrfs] > [<000000005eb3d845>] new_sync_write+0x106/0x180 > [<000000003fb505bf>] vfs_write+0x24d/0x2f0 > [<000000009bb57d37>] __x64_sys_pwrite64+0x69/0xa0 > [<000000003eba3fdf>] do_syscall_64+0x43/0x90 > > In case brtfs_qgroup_reserve_data() or btrfs_delalloc_reserve_metadata() > fail the allocated extent_changeset will not be freed. > > So in btrfs_check_data_free_space() and btrfs_delalloc_reserve_space() > free the allocated extent_changeset to get rid of the allocated memory. > > Cc: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Added to devel with Filipe's comment, thanks.
diff --git a/fs/btrfs/delalloc-space.c b/fs/btrfs/delalloc-space.c index bca438c7c972..fb46a28f5065 100644 --- a/fs/btrfs/delalloc-space.c +++ b/fs/btrfs/delalloc-space.c @@ -143,10 +143,13 @@ int btrfs_check_data_free_space(struct btrfs_inode *inode, /* Use new btrfs_qgroup_reserve_data to reserve precious data space. */ ret = btrfs_qgroup_reserve_data(inode, reserved, start, len); - if (ret < 0) + if (ret < 0) { btrfs_free_reserved_data_space_noquota(fs_info, len); - else + extent_changeset_free(*reserved); + *reserved = NULL; + } else { ret = 0; + } return ret; } @@ -452,8 +455,11 @@ int btrfs_delalloc_reserve_space(struct btrfs_inode *inode, if (ret < 0) return ret; ret = btrfs_delalloc_reserve_metadata(inode, len); - if (ret < 0) + if (ret < 0) { btrfs_free_reserved_data_space(inode, *reserved, start, len); + extent_changeset_free(*reserved); + *reserved = NULL; + } return ret; }
Fstests runs on my VMs have show several kmemleak reports like the following. unreferenced object 0xffff88811ae59080 (size 64): comm "xfs_io", pid 12124, jiffies 4294987392 (age 6.368s) hex dump (first 32 bytes): 00 c0 1c 00 00 00 00 00 ff cf 1c 00 00 00 00 00 ................ 90 97 e5 1a 81 88 ff ff 90 97 e5 1a 81 88 ff ff ................ backtrace: [<00000000ac0176d2>] ulist_add_merge+0x60/0x150 [btrfs] [<0000000076e9f312>] set_state_bits+0x86/0xc0 [btrfs] [<0000000014fe73d6>] set_extent_bit+0x270/0x690 [btrfs] [<000000004f675208>] set_record_extent_bits+0x19/0x20 [btrfs] [<00000000b96137b1>] qgroup_reserve_data+0x274/0x310 [btrfs] [<0000000057e9dcbb>] btrfs_check_data_free_space+0x5c/0xa0 [btrfs] [<0000000019c4511d>] btrfs_delalloc_reserve_space+0x1b/0xa0 [btrfs] [<000000006d37e007>] btrfs_dio_iomap_begin+0x415/0x970 [btrfs] [<00000000fb8a74b8>] iomap_iter+0x161/0x1e0 [<0000000071dff6ff>] __iomap_dio_rw+0x1df/0x700 [<000000002567ba53>] iomap_dio_rw+0x5/0x20 [<0000000072e555f8>] btrfs_file_write_iter+0x290/0x530 [btrfs] [<000000005eb3d845>] new_sync_write+0x106/0x180 [<000000003fb505bf>] vfs_write+0x24d/0x2f0 [<000000009bb57d37>] __x64_sys_pwrite64+0x69/0xa0 [<000000003eba3fdf>] do_syscall_64+0x43/0x90 In case brtfs_qgroup_reserve_data() or btrfs_delalloc_reserve_metadata() fail the allocated extent_changeset will not be freed. So in btrfs_check_data_free_space() and btrfs_delalloc_reserve_space() free the allocated extent_changeset to get rid of the allocated memory. Cc: Filipe Manana <fdmanana@suse.com> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/delalloc-space.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)