* [PATCH 1/2] f2fs: fix iostat lock protection
@ 2023-03-31 9:26 Bo Ye
2023-03-31 9:26 ` [PATCH 2/2] f2fs: add __pack attribute for extent_info Bo Ye
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bo Ye @ 2023-03-31 9:26 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu, Matthias Brugger, AngeloGioacchino Del Regno
Cc: yongdong.zhang, peng.zhou, browse.zhang, light.hsieh, Qilin Tan,
linux-f2fs-devel, linux-kernel, linux-arm-kernel, linux-mediatek
From: Qilin Tan <qilin.tan@mediatek.com>
Made iostat lock irq safe to avoid potentinal deadlock.
Deadlock scenario:
f2fs_attr_store
-> f2fs_sbi_store
-> _sbi_store
-> spin_lock(sbi->iostat_lock)
<interrupt request>
-> scsi_end_request
-> bio_endio
-> f2fs_dio_read_end_io
-> f2fs_update_iostat
-> spin_lock_irqsave(sbi->iostat_lock) ===> Dead lock here
Signed-off-by: Qilin Tan <qilin.tan@mediatek.com>
---
fs/f2fs/sysfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 0b19163c90d4..fd238a68017e 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -575,9 +575,9 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
if (!strcmp(a->attr.name, "iostat_period_ms")) {
if (t < MIN_IOSTAT_PERIOD_MS || t > MAX_IOSTAT_PERIOD_MS)
return -EINVAL;
- spin_lock(&sbi->iostat_lock);
+ spin_lock_irq(&sbi->iostat_lock);
sbi->iostat_period_ms = (unsigned int)t;
- spin_unlock(&sbi->iostat_lock);
+ spin_unlock_irq(&sbi->iostat_lock);
return count;
}
#endif
--
2.17.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] f2fs: add __pack attribute for extent_info
2023-03-31 9:26 [PATCH 1/2] f2fs: fix iostat lock protection Bo Ye
@ 2023-03-31 9:26 ` Bo Ye
2023-04-01 1:00 ` Chao Yu
2023-04-01 0:53 ` [PATCH 1/2] f2fs: fix iostat lock protection Chao Yu
2023-04-03 18:30 ` [f2fs-dev] " patchwork-bot+f2fs
2 siblings, 1 reply; 6+ messages in thread
From: Bo Ye @ 2023-03-31 9:26 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu, Matthias Brugger,
AngeloGioacchino Del Regno, xiongping1, qixiaoyu1
Cc: yongdong.zhang, peng.zhou, browse.zhang, light.hsieh, Qilin Tan,
Bo Ye, linux-f2fs-devel, linux-kernel, linux-arm-kernel,
linux-mediatek
From: Qilin Tan <qilin.tan@mediatek.com>
Need add __pack for struct extent_info to align to memory
layout of struct rb_entry.
struct rb_entry {
struct rb_node rb_node; /* rb node located in rb-tree */
union {
struct {
unsigned int ofs; /* start offset of the entry */
unsigned int len; /* length of the entry */
};
unsigned long long key; /* 64-bits key */
} __packed;
};
struct extent_info {
unsigned int fofs; /* start offset in a file */
unsigned int len; /* length of the extent */
union {
...
/* block age extent_cache */
struct {
/* block age of the extent */
unsigned long long age;
/* last total blocks allocated */
unsigned long long last_blocks;
};
};
The new fields(age, last_blocks) are u64 in change 71644dff4811, it
cause the memory alignment based on 8 bytes in some complier. So the
field fofs and len are alloced with 8 bytes and using the last 4 byts.
Its memory is not aligned with struct rb_entry. the ofs of rb_entry
pointer to a invalid value and cause writing file failed.
struct extent_info's offset should be 12 rather than 16 from the
beginning of struct rb_entry.
The offset dump for the bad case as:
kworker/u16:6: [name:f2fs&]f2fs_lookup_rb_tree_for_insert:MTK_DEBUG: ino=1629 re=0xc675dc08 ofs=0 re->ofs=0, re->len=68 ei=0xc675dc18 ei.fofs=68 ei.len=277
Fixes: 71644dff4811 ("f2fs: add block_age-based extent cache")
Signed-off-by: Bo Ye <bo.ye@mediatek.com>
Signed-off-by: Qilin Tan <qilin.tan@mediatek.com>
---
fs/f2fs/f2fs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b0ab2062038a..7c690667a42f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -660,7 +660,7 @@ struct extent_info {
unsigned long long last_blocks;
};
};
-};
+} __packed;
struct extent_node {
struct rb_node rb_node; /* rb node located in rb-tree */
--
2.17.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] f2fs: fix iostat lock protection
2023-03-31 9:26 [PATCH 1/2] f2fs: fix iostat lock protection Bo Ye
2023-03-31 9:26 ` [PATCH 2/2] f2fs: add __pack attribute for extent_info Bo Ye
@ 2023-04-01 0:53 ` Chao Yu
2023-04-03 18:30 ` [f2fs-dev] " patchwork-bot+f2fs
2 siblings, 0 replies; 6+ messages in thread
From: Chao Yu @ 2023-04-01 0:53 UTC (permalink / raw)
To: Bo Ye, Jaegeuk Kim, Matthias Brugger, AngeloGioacchino Del Regno
Cc: yongdong.zhang, peng.zhou, browse.zhang, light.hsieh, Qilin Tan,
linux-f2fs-devel, linux-kernel, linux-arm-kernel, linux-mediatek
On 2023/3/31 17:26, Bo Ye wrote:
> From: Qilin Tan <qilin.tan@mediatek.com>
>
> Made iostat lock irq safe to avoid potentinal deadlock.
>
> Deadlock scenario:
> f2fs_attr_store
> -> f2fs_sbi_store
> -> _sbi_store
> -> spin_lock(sbi->iostat_lock)
> <interrupt request>
> -> scsi_end_request
> -> bio_endio
> -> f2fs_dio_read_end_io
> -> f2fs_update_iostat
> -> spin_lock_irqsave(sbi->iostat_lock) ===> Dead lock here
>
Fixes: 61803e984307 ("f2fs: fix iostat related lock protection")
Fixes: a1e09b03e6f5 ("f2fs: use iomap for direct I/O")
> Signed-off-by: Qilin Tan <qilin.tan@mediatek.com>
Reviewed-by: Chao Yu <chao@kernel.org>
Thanks,
> ---
> fs/f2fs/sysfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 0b19163c90d4..fd238a68017e 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -575,9 +575,9 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
> if (!strcmp(a->attr.name, "iostat_period_ms")) {
> if (t < MIN_IOSTAT_PERIOD_MS || t > MAX_IOSTAT_PERIOD_MS)
> return -EINVAL;
> - spin_lock(&sbi->iostat_lock);
> + spin_lock_irq(&sbi->iostat_lock);
> sbi->iostat_period_ms = (unsigned int)t;
> - spin_unlock(&sbi->iostat_lock);
> + spin_unlock_irq(&sbi->iostat_lock);
> return count;
> }
> #endif
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] f2fs: add __pack attribute for extent_info
2023-03-31 9:26 ` [PATCH 2/2] f2fs: add __pack attribute for extent_info Bo Ye
@ 2023-04-01 1:00 ` Chao Yu
2023-04-07 9:38 ` Bo Ye (叶波)
0 siblings, 1 reply; 6+ messages in thread
From: Chao Yu @ 2023-04-01 1:00 UTC (permalink / raw)
To: Bo Ye, Jaegeuk Kim, Matthias Brugger, AngeloGioacchino Del Regno,
xiongping1, qixiaoyu1
Cc: yongdong.zhang, peng.zhou, browse.zhang, light.hsieh, Qilin Tan,
linux-f2fs-devel, linux-kernel, linux-arm-kernel, linux-mediatek
On 2023/3/31 17:26, Bo Ye wrote:
> From: Qilin Tan <qilin.tan@mediatek.com>
>
> Need add __pack for struct extent_info to align to memory
> layout of struct rb_entry.
Jaegeuk has fixed this bug w/ below patchset, please check it:
https://lore.kernel.org/linux-f2fs-devel/20230313201216.924234-1-jaegeuk@kernel.org/
Thanks,
>
> struct rb_entry {
> struct rb_node rb_node; /* rb node located in rb-tree */
> union {
> struct {
> unsigned int ofs; /* start offset of the entry */
> unsigned int len; /* length of the entry */
> };
> unsigned long long key; /* 64-bits key */
> } __packed;
> };
>
> struct extent_info {
> unsigned int fofs; /* start offset in a file */
> unsigned int len; /* length of the extent */
> union {
> ...
> /* block age extent_cache */
> struct {
> /* block age of the extent */
> unsigned long long age;
> /* last total blocks allocated */
> unsigned long long last_blocks;
> };
> };
>
> The new fields(age, last_blocks) are u64 in change 71644dff4811, it
> cause the memory alignment based on 8 bytes in some complier. So the
> field fofs and len are alloced with 8 bytes and using the last 4 byts.
> Its memory is not aligned with struct rb_entry. the ofs of rb_entry
> pointer to a invalid value and cause writing file failed.
>
> struct extent_info's offset should be 12 rather than 16 from the
> beginning of struct rb_entry.
>
> The offset dump for the bad case as:
> kworker/u16:6: [name:f2fs&]f2fs_lookup_rb_tree_for_insert:MTK_DEBUG: ino=1629 re=0xc675dc08 ofs=0 re->ofs=0, re->len=68 ei=0xc675dc18 ei.fofs=68 ei.len=277
>
> Fixes: 71644dff4811 ("f2fs: add block_age-based extent cache")
> Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> Signed-off-by: Qilin Tan <qilin.tan@mediatek.com>
> ---
> fs/f2fs/f2fs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b0ab2062038a..7c690667a42f 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -660,7 +660,7 @@ struct extent_info {
> unsigned long long last_blocks;
> };
> };
> -};
> +} __packed;
>
> struct extent_node {
> struct rb_node rb_node; /* rb node located in rb-tree */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: fix iostat lock protection
2023-03-31 9:26 [PATCH 1/2] f2fs: fix iostat lock protection Bo Ye
2023-03-31 9:26 ` [PATCH 2/2] f2fs: add __pack attribute for extent_info Bo Ye
2023-04-01 0:53 ` [PATCH 1/2] f2fs: fix iostat lock protection Chao Yu
@ 2023-04-03 18:30 ` patchwork-bot+f2fs
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+f2fs @ 2023-04-03 18:30 UTC (permalink / raw)
To: Bo Ye
Cc: jaegeuk, chao, matthias.bgg, angelogioacchino.delregno,
qilin.tan, yongdong.zhang, linux-kernel, light.hsieh,
linux-f2fs-devel, linux-mediatek, browse.zhang, linux-arm-kernel,
peng.zhou
Hello:
This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:
On Fri, 31 Mar 2023 17:26:56 +0800 you wrote:
> From: Qilin Tan <qilin.tan@mediatek.com>
>
> Made iostat lock irq safe to avoid potentinal deadlock.
>
> Deadlock scenario:
> f2fs_attr_store
> -> f2fs_sbi_store
> -> _sbi_store
> -> spin_lock(sbi->iostat_lock)
> <interrupt request>
> -> scsi_end_request
> -> bio_endio
> -> f2fs_dio_read_end_io
> -> f2fs_update_iostat
> -> spin_lock_irqsave(sbi->iostat_lock) ===> Dead lock here
>
> [...]
Here is the summary with links:
- [f2fs-dev,1/2] f2fs: fix iostat lock protection
https://git.kernel.org/jaegeuk/f2fs/c/daa080db4e7e
- [f2fs-dev,2/2] f2fs: add __pack attribute for extent_info
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] f2fs: add __pack attribute for extent_info
2023-04-01 1:00 ` Chao Yu
@ 2023-04-07 9:38 ` Bo Ye (叶波)
0 siblings, 0 replies; 6+ messages in thread
From: Bo Ye (叶波) @ 2023-04-07 9:38 UTC (permalink / raw)
To: jaegeuk, matthias.bgg, xiongping1, angelogioacchino.delregno,
chao, qixiaoyu1
Cc: linux-kernel, linux-mediatek, Browse Zhang (张磊),
Peng Zhou (周鹏),
Light Hsieh (謝明燈),
Yongdong Zhang (张永东),
linux-arm-kernel, Qilin Tan (谭麒麟),
linux-f2fs-devel
On Sat, 2023-04-01 at 09:00 +0800, Chao Yu wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 2023/3/31 17:26, Bo Ye wrote:
> > From: Qilin Tan <qilin.tan@mediatek.com>
> >
> > Need add __pack for struct extent_info to align to memory
> > layout of struct rb_entry.
>
> Jaegeuk has fixed this bug w/ below patchset, please check it:
>
>
https://lore.kernel.org/linux-f2fs-devel/20230313201216.924234-1-jaegeuk@kernel.org/
>
> Thanks,
>
OK, under verifying, I will reply with test result.
Thanks a lot!
> >
> > struct rb_entry {
> > struct rb_node rb_node; /* rb node located in rb-tree */
> > union {
> > struct {
> > unsigned int ofs; /* start offset of the entry */
> > unsigned int len; /* length of the entry */
> > };
> > unsigned long long key; /* 64-bits key */
> > } __packed;
> > };
> >
> > struct extent_info {
> > unsigned int fofs; /* start offset in a file */
> > unsigned int len; /* length of the extent */
> > union {
> > ...
> > /* block age extent_cache */
> > struct {
> > /* block age of the extent */
> > unsigned long long age;
> > /* last total blocks allocated */
> > unsigned long long last_blocks;
> > };
> > };
> >
> > The new fields(age, last_blocks) are u64 in change 71644dff4811, it
> > cause the memory alignment based on 8 bytes in some complier. So
> > the
> > field fofs and len are alloced with 8 bytes and using the last 4
> > byts.
> > Its memory is not aligned with struct rb_entry. the ofs of rb_entry
> > pointer to a invalid value and cause writing file failed.
> >
> > struct extent_info's offset should be 12 rather than 16 from the
> > beginning of struct rb_entry.
> >
> > The offset dump for the bad case as:
> > kworker/u16:6:
> > [name:f2fs&]f2fs_lookup_rb_tree_for_insert:MTK_DEBUG: ino=1629
> > re=0xc675dc08 ofs=0 re->ofs=0, re->len=68 ei=0xc675dc18 ei.fofs=68
> > ei.len=277
> >
> > Fixes: 71644dff4811 ("f2fs: add block_age-based extent cache")
> > Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> > Signed-off-by: Qilin Tan <qilin.tan@mediatek.com>
> > ---
> > fs/f2fs/f2fs.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index b0ab2062038a..7c690667a42f 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -660,7 +660,7 @@ struct extent_info {
> > unsigned long long last_blocks;
> > };
> > };
> > -};
> > +} __packed;
> >
> > struct extent_node {
> > struct rb_node rb_node; /* rb node located in rb-tree
> > */
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-07 9:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-31 9:26 [PATCH 1/2] f2fs: fix iostat lock protection Bo Ye
2023-03-31 9:26 ` [PATCH 2/2] f2fs: add __pack attribute for extent_info Bo Ye
2023-04-01 1:00 ` Chao Yu
2023-04-07 9:38 ` Bo Ye (叶波)
2023-04-01 0:53 ` [PATCH 1/2] f2fs: fix iostat lock protection Chao Yu
2023-04-03 18:30 ` [f2fs-dev] " patchwork-bot+f2fs
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).