* [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
@ 2023-07-07 9:40 Christoph Hellwig
2023-07-10 21:22 ` Jaegeuk Kim
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2023-07-07 9:40 UTC (permalink / raw)
To: jaegeuk, chao; +Cc: linux-block, linux-f2fs-devel
f2fs_scan_devices reopens the main device since the very beginning, which
has always been useless, and also means that we don't pass the right
holder for the reopen, which now leads to a warning as the core super.c
holder ops aren't passed in for the reopen.
Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
Fixes: 0718afd47f70 ("block: introduce holder ops")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/f2fs/super.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index ca31163da00a55..8d11d4a5ec331d 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1560,7 +1560,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
{
int i;
- for (i = 0; i < sbi->s_ndevs; i++) {
+ kvfree(FDEV(0).blkz_seq);
+ for (i = 1; i < sbi->s_ndevs; i++) {
blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
#ifdef CONFIG_BLK_DEV_ZONED
kvfree(FDEV(i).blkz_seq);
@@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
sbi->aligned_blksize = true;
for (i = 0; i < max_devices; i++) {
-
- if (i > 0 && !RDEV(i).path[0])
+ if (i == 0)
+ FDEV(0).bdev = sbi->sb->s_bdev;
+ else if (!RDEV(i).path[0])
break;
- if (max_devices == 1) {
- /* Single zoned block device mount */
- FDEV(0).bdev =
- blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
- sbi->sb->s_type, NULL);
- } else {
+ if (max_devices > 1) {
/* Multi-device mount */
memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
FDEV(i).total_segments =
@@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
FDEV(i).end_blk = FDEV(i).start_blk +
(FDEV(i).total_segments <<
sbi->log_blocks_per_seg) - 1;
+ FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
+ mode, sbi->sb->s_type, NULL);
}
- FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
- sbi->sb->s_type,
- NULL);
}
if (IS_ERR(FDEV(i).bdev))
return PTR_ERR(FDEV(i).bdev);
--
2.39.2
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-07 9:40 [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices Christoph Hellwig
@ 2023-07-10 21:22 ` Jaegeuk Kim
2023-07-10 23:51 ` Damien Le Moal
2023-07-11 5:01 ` Christoph Hellwig
0 siblings, 2 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2023-07-10 21:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-f2fs-devel
Hit a kernel panic with single device.
[ 148.003511] BUG: kernel NULL pointer dereference, address: 0000000000000058
[ 148.005630] #PF: supervisor read access in kernel mode
[ 148.008179] #PF: error_code(0x0000) - not-present page
[ 148.010593] PGD 0 P4D 0
[ 148.011867] Oops: 0000 [#1] PREEMPT SMP PTI
[ 148.014619] CPU: 4 PID: 1905 Comm: umount Tainted: G OE 6.5.0-rc1-custom #19
[ 148.020358] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 148.024967] RIP: 0010:destroy_device_list+0x18/0x90 [f2fs]
[ 148.027688] Code: 1f 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 8b 87 40 0b 00 00 <48> 8b 78 58 e8 cf 3e 28 cf 41 83 bc 24 3c 0b 00 00 01 7e 4a 41 bd
[ 148.038517] RSP: 0018:ffffa24e80be3d28 EFLAGS: 00010202
[ 148.040978] RAX: 0000000000000000 RBX: ffff8bd5503bc800 RCX: 0000000080080006
[ 148.044292] RDX: 0000000080080007 RSI: ffffdcfe844da200 RDI: ffff8bd55368d000
[ 148.047688] RBP: ffffa24e80be3d40 R08: ffff8bd553688000 R09: 0000000080080006
[ 148.051317] R10: ffff8bd5580d4e80 R11: ffff8bd57bd00000 R12: ffff8bd55368d000
[ 148.054981] R13: 0000000000000000 R14: ffff8bd55368db18 R15: 0000000000000000
[ 148.058391] FS: 00007fc247124800(0000) GS:ffff8bd57bd00000(0000) knlGS:0000000000000000
[ 148.062549] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 148.065641] CR2: 0000000000000058 CR3: 0000000001120004 CR4: 0000000000370ee0
[ 148.069178] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 148.072651] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 148.076346] Call Trace:
[ 148.077641] <TASK>
[ 148.078839] ? show_regs+0x6a/0x80
[ 148.080475] ? __die+0x25/0x70
[ 148.082045] ? page_fault_oops+0x160/0x480
[ 148.084381] ? check_preempt_wakeup+0x192/0x2f0
[ 148.086840] ? do_user_addr_fault+0x313/0x680
[ 148.088999] ? exc_page_fault+0x79/0x180
[ 148.090899] ? asm_exc_page_fault+0x27/0x30
[ 148.093114] ? destroy_device_list+0x18/0x90 [f2fs]
[ 148.095448] f2fs_put_super+0x211/0x410 [f2fs]
[ 148.097871] ? fscrypt_destroy_keyring+0x110/0x170
[ 148.100313] generic_shutdown_super+0x84/0x1b0
[ 148.102582] kill_block_super+0x24/0x50
[ 148.104697] kill_f2fs_super+0x83/0x100 [f2fs]
[ 148.106974] deactivate_locked_super+0x35/0xb0
[ 148.109978] deactivate_super+0x44/0x50
[ 148.112235] cleanup_mnt+0x105/0x160
[ 148.114407] __cleanup_mnt+0x12/0x20
[ 148.116680] task_work_run+0x61/0x90
[ 148.118961] exit_to_user_mode_prepare+0x18f/0x1a0
[ 148.121812] syscall_exit_to_user_mode+0x26/0x50
[ 148.124595] do_syscall_64+0x69/0x90
[ 148.126616] ? exc_page_fault+0x8a/0x180
[ 148.128742] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 148.131521] RIP: 0033:0x7fc246f24a7b
On 07/07, Christoph Hellwig wrote:
> f2fs_scan_devices reopens the main device since the very beginning, which
> has always been useless, and also means that we don't pass the right
> holder for the reopen, which now leads to a warning as the core super.c
> holder ops aren't passed in for the reopen.
>
> Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
> Fixes: 0718afd47f70 ("block: introduce holder ops")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/f2fs/super.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index ca31163da00a55..8d11d4a5ec331d 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1560,7 +1560,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
> {
> int i;
>
> - for (i = 0; i < sbi->s_ndevs; i++) {
#ifdef CONFIG_BLK_DEV_ZONED
> + kvfree(FDEV(0).blkz_seq);
#endif
> + for (i = 1; i < sbi->s_ndevs; i++) {
> blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
> #ifdef CONFIG_BLK_DEV_ZONED
> kvfree(FDEV(i).blkz_seq);
> @@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> sbi->aligned_blksize = true;
>
> for (i = 0; i < max_devices; i++) {
> -
> - if (i > 0 && !RDEV(i).path[0])
> + if (i == 0)
> + FDEV(0).bdev = sbi->sb->s_bdev;
> + else if (!RDEV(i).path[0])
> break;
>
> - if (max_devices == 1) {
> - /* Single zoned block device mount */
> - FDEV(0).bdev =
> - blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
> - sbi->sb->s_type, NULL);
> - } else {
> + if (max_devices > 1) {
> /* Multi-device mount */
> memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
> FDEV(i).total_segments =
> @@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> FDEV(i).end_blk = FDEV(i).start_blk +
> (FDEV(i).total_segments <<
> sbi->log_blocks_per_seg) - 1;
> + FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
> + mode, sbi->sb->s_type, NULL);
> }
> - FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
> - sbi->sb->s_type,
> - NULL);
> }
> if (IS_ERR(FDEV(i).bdev))
> return PTR_ERR(FDEV(i).bdev);
> --
> 2.39.2
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-10 21:22 ` Jaegeuk Kim
@ 2023-07-10 23:51 ` Damien Le Moal
2023-07-11 1:52 ` Jaegeuk Kim
2023-07-11 5:01 ` Christoph Hellwig
1 sibling, 1 reply; 10+ messages in thread
From: Damien Le Moal @ 2023-07-10 23:51 UTC (permalink / raw)
To: Jaegeuk Kim, Christoph Hellwig; +Cc: linux-block, linux-f2fs-devel
On 7/11/23 06:22, Jaegeuk Kim wrote:
> Hit a kernel panic with single device.
>
> [ 148.003511] BUG: kernel NULL pointer dereference, address: 0000000000000058
> [ 148.005630] #PF: supervisor read access in kernel mode
> [ 148.008179] #PF: error_code(0x0000) - not-present page
> [ 148.010593] PGD 0 P4D 0
> [ 148.011867] Oops: 0000 [#1] PREEMPT SMP PTI
> [ 148.014619] CPU: 4 PID: 1905 Comm: umount Tainted: G OE 6.5.0-rc1-custom #19
> [ 148.020358] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> [ 148.024967] RIP: 0010:destroy_device_list+0x18/0x90 [f2fs]
> [ 148.027688] Code: 1f 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 8b 87 40 0b 00 00 <48> 8b 78 58 e8 cf 3e 28 cf 41 83 bc 24 3c 0b 00 00 01 7e 4a 41 bd
> [ 148.038517] RSP: 0018:ffffa24e80be3d28 EFLAGS: 00010202
> [ 148.040978] RAX: 0000000000000000 RBX: ffff8bd5503bc800 RCX: 0000000080080006
> [ 148.044292] RDX: 0000000080080007 RSI: ffffdcfe844da200 RDI: ffff8bd55368d000
> [ 148.047688] RBP: ffffa24e80be3d40 R08: ffff8bd553688000 R09: 0000000080080006
> [ 148.051317] R10: ffff8bd5580d4e80 R11: ffff8bd57bd00000 R12: ffff8bd55368d000
> [ 148.054981] R13: 0000000000000000 R14: ffff8bd55368db18 R15: 0000000000000000
> [ 148.058391] FS: 00007fc247124800(0000) GS:ffff8bd57bd00000(0000) knlGS:0000000000000000
> [ 148.062549] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 148.065641] CR2: 0000000000000058 CR3: 0000000001120004 CR4: 0000000000370ee0
> [ 148.069178] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 148.072651] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 148.076346] Call Trace:
> [ 148.077641] <TASK>
> [ 148.078839] ? show_regs+0x6a/0x80
> [ 148.080475] ? __die+0x25/0x70
> [ 148.082045] ? page_fault_oops+0x160/0x480
> [ 148.084381] ? check_preempt_wakeup+0x192/0x2f0
> [ 148.086840] ? do_user_addr_fault+0x313/0x680
> [ 148.088999] ? exc_page_fault+0x79/0x180
> [ 148.090899] ? asm_exc_page_fault+0x27/0x30
> [ 148.093114] ? destroy_device_list+0x18/0x90 [f2fs]
> [ 148.095448] f2fs_put_super+0x211/0x410 [f2fs]
> [ 148.097871] ? fscrypt_destroy_keyring+0x110/0x170
> [ 148.100313] generic_shutdown_super+0x84/0x1b0
> [ 148.102582] kill_block_super+0x24/0x50
> [ 148.104697] kill_f2fs_super+0x83/0x100 [f2fs]
> [ 148.106974] deactivate_locked_super+0x35/0xb0
> [ 148.109978] deactivate_super+0x44/0x50
> [ 148.112235] cleanup_mnt+0x105/0x160
> [ 148.114407] __cleanup_mnt+0x12/0x20
> [ 148.116680] task_work_run+0x61/0x90
> [ 148.118961] exit_to_user_mode_prepare+0x18f/0x1a0
> [ 148.121812] syscall_exit_to_user_mode+0x26/0x50
> [ 148.124595] do_syscall_64+0x69/0x90
> [ 148.126616] ? exc_page_fault+0x8a/0x180
> [ 148.128742] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> [ 148.131521] RIP: 0033:0x7fc246f24a7b
>
> On 07/07, Christoph Hellwig wrote:
>> f2fs_scan_devices reopens the main device since the very beginning, which
>> has always been useless, and also means that we don't pass the right
>> holder for the reopen, which now leads to a warning as the core super.c
>> holder ops aren't passed in for the reopen.
>>
>> Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
>> Fixes: 0718afd47f70 ("block: introduce holder ops")
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>> fs/f2fs/super.c | 20 ++++++++------------
>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index ca31163da00a55..8d11d4a5ec331d 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -1560,7 +1560,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
>> {
>> int i;
>>
>> - for (i = 0; i < sbi->s_ndevs; i++) {
>
> #ifdef CONFIG_BLK_DEV_ZONED
>
>> + kvfree(FDEV(0).blkz_seq);
>
> #endif
This should not be needed since for the !CONFIG_BLK_DEV_ZONED case,
FDEV(0).blkz_seq should always be NULL. However, what I think may be missing is
"FDEV(0).blkz_seq = NULL;" after the kvfree() call. No ?
>
>> + for (i = 1; i < sbi->s_ndevs; i++) {
>> blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
>> #ifdef CONFIG_BLK_DEV_ZONED
>> kvfree(FDEV(i).blkz_seq);
>> @@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
>> sbi->aligned_blksize = true;
>>
>> for (i = 0; i < max_devices; i++) {
>> -
>> - if (i > 0 && !RDEV(i).path[0])
>> + if (i == 0)
>> + FDEV(0).bdev = sbi->sb->s_bdev;
>> + else if (!RDEV(i).path[0])
>> break;
>>
>> - if (max_devices == 1) {
>> - /* Single zoned block device mount */
>> - FDEV(0).bdev =
>> - blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
>> - sbi->sb->s_type, NULL);
>> - } else {
>> + if (max_devices > 1) {
>> /* Multi-device mount */
>> memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
>> FDEV(i).total_segments =
>> @@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
>> FDEV(i).end_blk = FDEV(i).start_blk +
>> (FDEV(i).total_segments <<
>> sbi->log_blocks_per_seg) - 1;
>> + FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
>> + mode, sbi->sb->s_type, NULL);
>> }
>> - FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
>> - sbi->sb->s_type,
>> - NULL);
>> }
>> if (IS_ERR(FDEV(i).bdev))
>> return PTR_ERR(FDEV(i).bdev);
>> --
>> 2.39.2
--
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-10 23:51 ` Damien Le Moal
@ 2023-07-11 1:52 ` Jaegeuk Kim
2023-07-11 2:00 ` Damien Le Moal
0 siblings, 1 reply; 10+ messages in thread
From: Jaegeuk Kim @ 2023-07-11 1:52 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-block, Christoph Hellwig, linux-f2fs-devel
On 07/11, Damien Le Moal wrote:
> On 7/11/23 06:22, Jaegeuk Kim wrote:
> > Hit a kernel panic with single device.
> >
> > [ 148.003511] BUG: kernel NULL pointer dereference, address: 0000000000000058
> > [ 148.005630] #PF: supervisor read access in kernel mode
> > [ 148.008179] #PF: error_code(0x0000) - not-present page
> > [ 148.010593] PGD 0 P4D 0
> > [ 148.011867] Oops: 0000 [#1] PREEMPT SMP PTI
> > [ 148.014619] CPU: 4 PID: 1905 Comm: umount Tainted: G OE 6.5.0-rc1-custom #19
> > [ 148.020358] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> > [ 148.024967] RIP: 0010:destroy_device_list+0x18/0x90 [f2fs]
> > [ 148.027688] Code: 1f 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 8b 87 40 0b 00 00 <48> 8b 78 58 e8 cf 3e 28 cf 41 83 bc 24 3c 0b 00 00 01 7e 4a 41 bd
> > [ 148.038517] RSP: 0018:ffffa24e80be3d28 EFLAGS: 00010202
> > [ 148.040978] RAX: 0000000000000000 RBX: ffff8bd5503bc800 RCX: 0000000080080006
> > [ 148.044292] RDX: 0000000080080007 RSI: ffffdcfe844da200 RDI: ffff8bd55368d000
> > [ 148.047688] RBP: ffffa24e80be3d40 R08: ffff8bd553688000 R09: 0000000080080006
> > [ 148.051317] R10: ffff8bd5580d4e80 R11: ffff8bd57bd00000 R12: ffff8bd55368d000
> > [ 148.054981] R13: 0000000000000000 R14: ffff8bd55368db18 R15: 0000000000000000
> > [ 148.058391] FS: 00007fc247124800(0000) GS:ffff8bd57bd00000(0000) knlGS:0000000000000000
> > [ 148.062549] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 148.065641] CR2: 0000000000000058 CR3: 0000000001120004 CR4: 0000000000370ee0
> > [ 148.069178] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [ 148.072651] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [ 148.076346] Call Trace:
> > [ 148.077641] <TASK>
> > [ 148.078839] ? show_regs+0x6a/0x80
> > [ 148.080475] ? __die+0x25/0x70
> > [ 148.082045] ? page_fault_oops+0x160/0x480
> > [ 148.084381] ? check_preempt_wakeup+0x192/0x2f0
> > [ 148.086840] ? do_user_addr_fault+0x313/0x680
> > [ 148.088999] ? exc_page_fault+0x79/0x180
> > [ 148.090899] ? asm_exc_page_fault+0x27/0x30
> > [ 148.093114] ? destroy_device_list+0x18/0x90 [f2fs]
> > [ 148.095448] f2fs_put_super+0x211/0x410 [f2fs]
> > [ 148.097871] ? fscrypt_destroy_keyring+0x110/0x170
> > [ 148.100313] generic_shutdown_super+0x84/0x1b0
> > [ 148.102582] kill_block_super+0x24/0x50
> > [ 148.104697] kill_f2fs_super+0x83/0x100 [f2fs]
> > [ 148.106974] deactivate_locked_super+0x35/0xb0
> > [ 148.109978] deactivate_super+0x44/0x50
> > [ 148.112235] cleanup_mnt+0x105/0x160
> > [ 148.114407] __cleanup_mnt+0x12/0x20
> > [ 148.116680] task_work_run+0x61/0x90
> > [ 148.118961] exit_to_user_mode_prepare+0x18f/0x1a0
> > [ 148.121812] syscall_exit_to_user_mode+0x26/0x50
> > [ 148.124595] do_syscall_64+0x69/0x90
> > [ 148.126616] ? exc_page_fault+0x8a/0x180
> > [ 148.128742] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> > [ 148.131521] RIP: 0033:0x7fc246f24a7b
> >
> > On 07/07, Christoph Hellwig wrote:
> >> f2fs_scan_devices reopens the main device since the very beginning, which
> >> has always been useless, and also means that we don't pass the right
> >> holder for the reopen, which now leads to a warning as the core super.c
> >> holder ops aren't passed in for the reopen.
> >>
> >> Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
> >> Fixes: 0718afd47f70 ("block: introduce holder ops")
> >> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> ---
> >> fs/f2fs/super.c | 20 ++++++++------------
> >> 1 file changed, 8 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >> index ca31163da00a55..8d11d4a5ec331d 100644
> >> --- a/fs/f2fs/super.c
> >> +++ b/fs/f2fs/super.c
> >> @@ -1560,7 +1560,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
> >> {
> >> int i;
> >>
> >> - for (i = 0; i < sbi->s_ndevs; i++) {
> >
> > #ifdef CONFIG_BLK_DEV_ZONED
> >
> >> + kvfree(FDEV(0).blkz_seq);
> >
> > #endif
>
> This should not be needed since for the !CONFIG_BLK_DEV_ZONED case,
> FDEV(0).blkz_seq should always be NULL. However, what I think may be missing is
> "FDEV(0).blkz_seq = NULL;" after the kvfree() call. No ?
I was looking at a glance of this:
https://lore.kernel.org/linux-f2fs-devel/202307110542.NBAMyZxE-lkp@intel.com/T/#u
>
> >
> >> + for (i = 1; i < sbi->s_ndevs; i++) {
> >> blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
> >> #ifdef CONFIG_BLK_DEV_ZONED
> >> kvfree(FDEV(i).blkz_seq);
> >> @@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> >> sbi->aligned_blksize = true;
> >>
> >> for (i = 0; i < max_devices; i++) {
> >> -
> >> - if (i > 0 && !RDEV(i).path[0])
> >> + if (i == 0)
> >> + FDEV(0).bdev = sbi->sb->s_bdev;
> >> + else if (!RDEV(i).path[0])
> >> break;
> >>
> >> - if (max_devices == 1) {
> >> - /* Single zoned block device mount */
> >> - FDEV(0).bdev =
> >> - blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
> >> - sbi->sb->s_type, NULL);
> >> - } else {
> >> + if (max_devices > 1) {
> >> /* Multi-device mount */
> >> memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
> >> FDEV(i).total_segments =
> >> @@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> >> FDEV(i).end_blk = FDEV(i).start_blk +
> >> (FDEV(i).total_segments <<
> >> sbi->log_blocks_per_seg) - 1;
> >> + FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
> >> + mode, sbi->sb->s_type, NULL);
> >> }
> >> - FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
> >> - sbi->sb->s_type,
> >> - NULL);
> >> }
> >> if (IS_ERR(FDEV(i).bdev))
> >> return PTR_ERR(FDEV(i).bdev);
> >> --
> >> 2.39.2
>
> --
> Damien Le Moal
> Western Digital Research
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-11 1:52 ` Jaegeuk Kim
@ 2023-07-11 2:00 ` Damien Le Moal
0 siblings, 0 replies; 10+ messages in thread
From: Damien Le Moal @ 2023-07-11 2:00 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-block, Christoph Hellwig, linux-f2fs-devel
On 7/11/23 10:52, Jaegeuk Kim wrote:
>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>> index ca31163da00a55..8d11d4a5ec331d 100644
>>>> --- a/fs/f2fs/super.c
>>>> +++ b/fs/f2fs/super.c
>>>> @@ -1560,7 +1560,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
>>>> {
>>>> int i;
>>>>
>>>> - for (i = 0; i < sbi->s_ndevs; i++) {
>>>
>>> #ifdef CONFIG_BLK_DEV_ZONED
>>>
>>>> + kvfree(FDEV(0).blkz_seq);
>>>
>>> #endif
>>
>> This should not be needed since for the !CONFIG_BLK_DEV_ZONED case,
>> FDEV(0).blkz_seq should always be NULL. However, what I think may be missing is
>> "FDEV(0).blkz_seq = NULL;" after the kvfree() call. No ?
>
> I was looking at a glance of this:
> https://lore.kernel.org/linux-f2fs-devel/202307110542.NBAMyZxE-lkp@intel.com/T/#u
OK. Got it.
I still think that it may be safer to add the NULL affectation though.
--
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-10 21:22 ` Jaegeuk Kim
2023-07-10 23:51 ` Damien Le Moal
@ 2023-07-11 5:01 ` Christoph Hellwig
2023-07-11 6:19 ` Hannes Reinecke
` (2 more replies)
1 sibling, 3 replies; 10+ messages in thread
From: Christoph Hellwig @ 2023-07-11 5:01 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-block, Christoph Hellwig, linux-f2fs-devel
I think that's because it doesn't look at sbi->s_ndevs in
destroy_device_list. Let's try the variant below, which also fixes
the buildbot warning for non-zoned configfs:
---
From 645d8dceaa97b6ee73be067495b111b15b187498 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 7 Jul 2023 10:31:49 +0200
Subject: f2fs: don't reopen the main block device in f2fs_scan_devices
f2fs_scan_devices reopens the main device since the very beginning, which
has always been useless, and also means that we don't pass the right
holder for the reopen, which now leads to a warning as the core super.c
holder ops aren't passed in for the reopen.
Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
Fixes: 0718afd47f70 ("block: introduce holder ops")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-flush.c | 2 +-
fs/f2fs/super.c | 20 ++++++++------------
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index ca31163da00a55..30883beb750a59 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1561,7 +1561,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
int i;
for (i = 0; i < sbi->s_ndevs; i++) {
- blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
+ if (i > 0)
+ blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
#ifdef CONFIG_BLK_DEV_ZONED
kvfree(FDEV(i).blkz_seq);
#endif
@@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
sbi->aligned_blksize = true;
for (i = 0; i < max_devices; i++) {
-
- if (i > 0 && !RDEV(i).path[0])
+ if (i == 0)
+ FDEV(0).bdev = sbi->sb->s_bdev;
+ else if (!RDEV(i).path[0])
break;
- if (max_devices == 1) {
- /* Single zoned block device mount */
- FDEV(0).bdev =
- blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
- sbi->sb->s_type, NULL);
- } else {
+ if (max_devices > 1) {
/* Multi-device mount */
memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
FDEV(i).total_segments =
@@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
FDEV(i).end_blk = FDEV(i).start_blk +
(FDEV(i).total_segments <<
sbi->log_blocks_per_seg) - 1;
+ FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
+ mode, sbi->sb->s_type, NULL);
}
- FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
- sbi->sb->s_type,
- NULL);
}
if (IS_ERR(FDEV(i).bdev))
return PTR_ERR(FDEV(i).bdev);
--
2.39.2
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-11 5:01 ` Christoph Hellwig
@ 2023-07-11 6:19 ` Hannes Reinecke
2023-07-11 6:22 ` Christoph Hellwig
2023-07-11 16:17 ` Jaegeuk Kim
2023-07-17 15:16 ` Chao Yu
2 siblings, 1 reply; 10+ messages in thread
From: Hannes Reinecke @ 2023-07-11 6:19 UTC (permalink / raw)
To: Christoph Hellwig, Jaegeuk Kim; +Cc: linux-block, linux-f2fs-devel
On 7/11/23 07:01, Christoph Hellwig wrote:
> I think that's because it doesn't look at sbi->s_ndevs in
> destroy_device_list. Let's try the variant below, which also fixes
> the buildbot warning for non-zoned configfs:
>
> ---
> From 645d8dceaa97b6ee73be067495b111b15b187498 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 7 Jul 2023 10:31:49 +0200
> Subject: f2fs: don't reopen the main block device in f2fs_scan_devices
>
> f2fs_scan_devices reopens the main device since the very beginning, which
> has always been useless, and also means that we don't pass the right
> holder for the reopen, which now leads to a warning as the core super.c
> holder ops aren't passed in for the reopen.
>
> Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
> Fixes: 0718afd47f70 ("block: introduce holder ops")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/blk-flush.c | 2 +-
> fs/f2fs/super.c | 20 ++++++++------------
> 2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index ca31163da00a55..30883beb750a59 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1561,7 +1561,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
> int i;
>
> for (i = 0; i < sbi->s_ndevs; i++) {
> - blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
> + if (i > 0)
> + blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
You could have started the loop at '1', and avoid the curious 'if'
clause ...
> #ifdef CONFIG_BLK_DEV_ZONED
> kvfree(FDEV(i).blkz_seq);
> #endif
> @@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> sbi->aligned_blksize = true;
>
> for (i = 0; i < max_devices; i++) {
> -
> - if (i > 0 && !RDEV(i).path[0])
> + if (i == 0)
> + FDEV(0).bdev = sbi->sb->s_bdev;
> + else if (!RDEV(i).path[0])
> break;
>
> - if (max_devices == 1) {
> - /* Single zoned block device mount */
> - FDEV(0).bdev =
> - blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
> - sbi->sb->s_type, NULL);
> - } else {
> + if (max_devices > 1) {
> /* Multi-device mount */
> memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
> FDEV(i).total_segments =
Similar here; wouldn't it be better to unroll the loop, and have the
check for 'max_devices' outside of the loop?
Then the loop can be coded for the 'max_device > 1' case only, and avoid
all the special casing in the loop ...
> @@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> FDEV(i).end_blk = FDEV(i).start_blk +
> (FDEV(i).total_segments <<
> sbi->log_blocks_per_seg) - 1;
> + FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
> + mode, sbi->sb->s_type, NULL);
> }
> - FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
> - sbi->sb->s_type,
> - NULL);
> }
> if (IS_ERR(FDEV(i).bdev))
> return PTR_ERR(FDEV(i).bdev);
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-11 6:19 ` Hannes Reinecke
@ 2023-07-11 6:22 ` Christoph Hellwig
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2023-07-11 6:22 UTC (permalink / raw)
To: Hannes Reinecke
Cc: linux-block, Jaegeuk Kim, Christoph Hellwig, linux-f2fs-devel
On Tue, Jul 11, 2023 at 08:19:50AM +0200, Hannes Reinecke wrote:
>> for (i = 0; i < sbi->s_ndevs; i++) {
>> - blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
>> + if (i > 0)
>> + blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
> You could have started the loop at '1', and avoid the curious 'if' clause
That's what the previous version did, which caused a NULL pointer
dereference discussed in this thread, as well as a compile error for
non-zoned builds..
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-11 5:01 ` Christoph Hellwig
2023-07-11 6:19 ` Hannes Reinecke
@ 2023-07-11 16:17 ` Jaegeuk Kim
2023-07-17 15:16 ` Chao Yu
2 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2023-07-11 16:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-f2fs-devel
On 07/11, Christoph Hellwig wrote:
> I think that's because it doesn't look at sbi->s_ndevs in
> destroy_device_list. Let's try the variant below, which also fixes
> the buildbot warning for non-zoned configfs:
Thanks. At a glance, this looks better. Let me give it a try.
>
> ---
> >From 645d8dceaa97b6ee73be067495b111b15b187498 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 7 Jul 2023 10:31:49 +0200
> Subject: f2fs: don't reopen the main block device in f2fs_scan_devices
>
> f2fs_scan_devices reopens the main device since the very beginning, which
> has always been useless, and also means that we don't pass the right
> holder for the reopen, which now leads to a warning as the core super.c
> holder ops aren't passed in for the reopen.
>
> Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
> Fixes: 0718afd47f70 ("block: introduce holder ops")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/blk-flush.c | 2 +-
> fs/f2fs/super.c | 20 ++++++++------------
> 2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index ca31163da00a55..30883beb750a59 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1561,7 +1561,8 @@ static void destroy_device_list(struct f2fs_sb_info *sbi)
> int i;
>
> for (i = 0; i < sbi->s_ndevs; i++) {
> - blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
> + if (i > 0)
> + blkdev_put(FDEV(i).bdev, sbi->sb->s_type);
> #ifdef CONFIG_BLK_DEV_ZONED
> kvfree(FDEV(i).blkz_seq);
> #endif
> @@ -4190,16 +4191,12 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> sbi->aligned_blksize = true;
>
> for (i = 0; i < max_devices; i++) {
> -
> - if (i > 0 && !RDEV(i).path[0])
> + if (i == 0)
> + FDEV(0).bdev = sbi->sb->s_bdev;
> + else if (!RDEV(i).path[0])
> break;
>
> - if (max_devices == 1) {
> - /* Single zoned block device mount */
> - FDEV(0).bdev =
> - blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode,
> - sbi->sb->s_type, NULL);
> - } else {
> + if (max_devices > 1) {
> /* Multi-device mount */
> memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN);
> FDEV(i).total_segments =
> @@ -4215,10 +4212,9 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi)
> FDEV(i).end_blk = FDEV(i).start_blk +
> (FDEV(i).total_segments <<
> sbi->log_blocks_per_seg) - 1;
> + FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path,
> + mode, sbi->sb->s_type, NULL);
> }
> - FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode,
> - sbi->sb->s_type,
> - NULL);
> }
> if (IS_ERR(FDEV(i).bdev))
> return PTR_ERR(FDEV(i).bdev);
> --
> 2.39.2
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices
2023-07-11 5:01 ` Christoph Hellwig
2023-07-11 6:19 ` Hannes Reinecke
2023-07-11 16:17 ` Jaegeuk Kim
@ 2023-07-17 15:16 ` Chao Yu
2 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2023-07-17 15:16 UTC (permalink / raw)
To: Christoph Hellwig, Jaegeuk Kim; +Cc: linux-block, linux-f2fs-devel
On 2023/7/11 13:01, Christoph Hellwig wrote:
> I think that's because it doesn't look at sbi->s_ndevs in
> destroy_device_list. Let's try the variant below, which also fixes
> the buildbot warning for non-zoned configfs:
>
> ---
> From 645d8dceaa97b6ee73be067495b111b15b187498 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 7 Jul 2023 10:31:49 +0200
> Subject: f2fs: don't reopen the main block device in f2fs_scan_devices
>
> f2fs_scan_devices reopens the main device since the very beginning, which
> has always been useless, and also means that we don't pass the right
> holder for the reopen, which now leads to a warning as the core super.c
> holder ops aren't passed in for the reopen.
>
> Fixes: 3c62be17d4f5 ("f2fs: support multiple devices")
> Fixes: 0718afd47f70 ("block: introduce holder ops")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chao Yu <chao@kernel.org>
Thanks,
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-07-17 15:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-07 9:40 [f2fs-dev] [PATCH] f2fs: don't reopen the main block device in f2fs_scan_devices Christoph Hellwig
2023-07-10 21:22 ` Jaegeuk Kim
2023-07-10 23:51 ` Damien Le Moal
2023-07-11 1:52 ` Jaegeuk Kim
2023-07-11 2:00 ` Damien Le Moal
2023-07-11 5:01 ` Christoph Hellwig
2023-07-11 6:19 ` Hannes Reinecke
2023-07-11 6:22 ` Christoph Hellwig
2023-07-11 16:17 ` Jaegeuk Kim
2023-07-17 15:16 ` Chao Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).