Re: [f2fs-dev] [PATCH V3] f2fs: unify the error handling of f2fs_is_valid_blkaddr
Dear Chao,
Do you have any other suggestions about this patch version?
thanks!
On Tue, Dec 19, 2023 at 11:57 AM Zhiguo Niu wrote:
>
> unify the error handling of ERROR_INVALID_BLKADDR in f2fs_is_valid_blkaddr
> and remove some redundant codes in f2fs_cache_compressed_page.
>
> Signed-off-by: Zhiguo Niu
> ---
> changes of v2: improve patch according Chao's suggestions.
> changes of v3:
> -rebase patch to dev-test
> -correct return value for some f2fs_is_valid_blkaddr error case
> ---
> ---
> fs/f2fs/checkpoint.c | 39 ---
> fs/f2fs/compress.c | 4
> fs/f2fs/data.c | 24
> fs/f2fs/extent_cache.c | 7 ++-
> fs/f2fs/file.c | 12 ++--
> fs/f2fs/gc.c | 2 --
> fs/f2fs/node.c | 2 +-
> fs/f2fs/recovery.c | 4
> fs/f2fs/segment.c | 2 --
> 9 files changed, 29 insertions(+), 67 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index b0597a5..83119aa 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -154,19 +154,17 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi,
> block_t blkaddr,
> if (unlikely(f2fs_cp_error(sbi)))
> return exist;
>
> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> -blkaddr, exist);
> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> - return exist;
> - }
> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
> + (!exist && type == DATA_GENERIC_ENHANCE))
> + goto err;
>
> - if (!exist && type == DATA_GENERIC_ENHANCE) {
> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> -blkaddr, exist);
> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> - dump_stack();
> - }
> + return exist;
> +err:
> + f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> + blkaddr, exist);
> + set_sbi_flag(sbi, SBI_NEED_FSCK);
> + dump_stack();
> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> return exist;
> }
>
> @@ -174,29 +172,29 @@ bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> block_t blkaddr, int type)
> {
> if (time_to_inject(sbi, FAULT_BLKADDR))
> - return false;
> + goto err;
>
> switch (type) {
> case META_NAT:
> break;
> case META_SIT:
> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
> - return false;
> + goto err;
> break;
> case META_SSA:
> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
> blkaddr < SM_I(sbi)->ssa_blkaddr))
> - return false;
> + goto err;
> break;
> case META_CP:
> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
> blkaddr < __start_cp_addr(sbi)))
> - return false;
> + goto err;
> break;
> case META_POR:
> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> blkaddr < MAIN_BLKADDR(sbi)))
> - return false;
> + goto err;
> break;
> case DATA_GENERIC:
> case DATA_GENERIC_ENHANCE:
> @@ -213,7 +211,7 @@ bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> blkaddr);
> set_sbi_flag(sbi, SBI_NEED_FSCK);
> dump_stack();
> - return false;
> + goto err;
> } else {
> return __is_bitmap_valid(sbi, blkaddr, type);
> }
> @@ -221,13 +219,16 @@ bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> case META_GENERIC:
> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
> blkaddr >= MAIN_BLKADDR(sbi)))
> - return false;
> + goto err;
> break;
> default:
> BUG();
> }
>
> return true;
> +err:
> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> + return false;
> }
>
> /*
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index c5a4364..cf075ca 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1878,12 +1878,8 @@ void f2fs_cache_compressed_page(struct f2fs_sb_info
> *sbi, struct page *page,
>
> set_page_private_data(cpage, ino);
>
> - if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE_READ))
> - goto out;
> -
> memcpy(page_address(c
Re: [f2fs-dev] [syzbot] [reiserfs?] possible deadlock in super_lock
syzbot suspects this issue was fixed by commit: commit fd1464105cb37a3b50a72c1d2902e97a71950af8 Author: Jan Kara Date: Wed Oct 18 15:29:24 2023 + fs: Avoid grabbing sb->s_umount under bdev->bd_holder_lock bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14639595e8 start commit: 2cf0f7156238 Merge tag 'nfs-for-6.6-2' of git://git.linux-.. git tree: upstream kernel config: https://syzkaller.appspot.com/x/.config?x=710dc49bece494df dashboard link: https://syzkaller.appspot.com/bug?extid=062317ea1d0a6d5e29e7 syz repro: https://syzkaller.appspot.com/x/repro.syz?x=107e951868 If the result looks correct, please mark the issue as fixed by replying with: #syz fix: fs: Avoid grabbing sb->s_umount under bdev->bd_holder_lock For information about bisection process see: https://goo.gl/tpsmEJ#bisection ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: Use wait_event_freezable_timeout() for freezable kthread
On 2023/12/21 14:49, Kevin Hao wrote: A freezable kernel thread can enter frozen state during freezing by either calling try_to_freeze() or using wait_event_freezable() and its variants. So for the following snippet of code in a kernel thread loop: wait_event_interruptible_timeout(); try_to_freeze(); We can change it to a simple wait_event_freezable_timeout() and then eliminate the function calls to try_to_freeze() and freezing(). Signed-off-by: Kevin Hao Reviewed-by: Chao Yu Thanks, ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2 4/6] f2fs: compress: fix to avoid inconsistent bewteen i_blocks and dnode
In reserve_compress_blocks(), we update blkaddrs of dnode in prior to
inc_valid_block_count(), it may cause inconsistent status bewteen
i_blocks and blkaddrs once inc_valid_block_count() fails.
To fix this issue, it needs to reverse their invoking order.
Fixes: c75488fb4d82 ("f2fs: introduce F2FS_IOC_RESERVE_COMPRESS_BLOCKS")
Signed-off-by: Chao Yu
---
v2:
- rebase code to last dev-test branch
fs/f2fs/data.c| 5 +++--
fs/f2fs/f2fs.h| 7 ++-
fs/f2fs/file.c| 26 ++
fs/f2fs/segment.c | 2 +-
4 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 263053219b28..b6e35e601e24 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1224,7 +1224,8 @@ int f2fs_reserve_new_blocks(struct dnode_of_data *dn,
blkcnt_t count)
if (unlikely(is_inode_flag_set(dn->inode, FI_NO_ALLOC)))
return -EPERM;
- if (unlikely((err = inc_valid_block_count(sbi, dn->inode, &count
+ err = inc_valid_block_count(sbi, dn->inode, &count, true);
+ if (unlikely(err))
return err;
trace_f2fs_reserve_new_blocks(dn->inode, dn->nid,
@@ -1481,7 +1482,7 @@ static int __allocate_data_block(struct dnode_of_data
*dn, int seg_type)
dn->data_blkaddr = f2fs_data_blkaddr(dn);
if (dn->data_blkaddr == NULL_ADDR) {
- err = inc_valid_block_count(sbi, dn->inode, &count);
+ err = inc_valid_block_count(sbi, dn->inode, &count, true);
if (unlikely(err))
return err;
}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7d0c2b05c5a8..dc1feafb4973 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2251,7 +2251,7 @@ static inline bool __allow_reserved_blocks(struct
f2fs_sb_info *sbi,
static inline void f2fs_i_blocks_write(struct inode *, block_t, bool, bool);
static inline int inc_valid_block_count(struct f2fs_sb_info *sbi,
-struct inode *inode, blkcnt_t *count)
+struct inode *inode, blkcnt_t *count, bool
partial)
{
blkcnt_t diff = 0, release = 0;
block_t avail_user_block_count;
@@ -2291,6 +2291,11 @@ static inline int inc_valid_block_count(struct
f2fs_sb_info *sbi,
avail_user_block_count = 0;
}
if (unlikely(sbi->total_valid_block_count > avail_user_block_count)) {
+ if (!partial) {
+ spin_unlock(&sbi->stat_lock);
+ goto enospc;
+ }
+
diff = sbi->total_valid_block_count - avail_user_block_count;
if (diff > *count)
diff = *count;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 782ae3be48f6..9f4e21b5916c 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3614,14 +3614,16 @@ static int reserve_compress_blocks(struct dnode_of_data
*dn, pgoff_t count)
blkcnt_t reserved;
int ret;
- for (i = 0; i < cluster_size; i++, dn->ofs_in_node++) {
- blkaddr = f2fs_data_blkaddr(dn);
+ for (i = 0; i < cluster_size; i++) {
+ blkaddr = data_blkaddr(dn->inode, dn->node_page,
+ dn->ofs_in_node + i);
if (i == 0) {
- if (blkaddr == COMPRESS_ADDR)
- continue;
- dn->ofs_in_node += cluster_size;
- goto next;
+ if (blkaddr != COMPRESS_ADDR) {
+ dn->ofs_in_node += cluster_size;
+ goto next;
+ }
+ continue;
}
/*
@@ -3637,20 +3639,20 @@ static int reserve_compress_blocks(struct dnode_of_data
*dn, pgoff_t count)
compr_blocks++;
continue;
}
-
- f2fs_set_data_blkaddr(dn, NEW_ADDR);
}
reserved = cluster_size - compr_blocks;
if (!reserved)
goto next;
- ret = inc_valid_block_count(sbi, dn->inode, &reserved);
- if (ret)
+ ret = inc_valid_block_count(sbi, dn->inode, &reserved, false);
+ if (unlikely(ret))
return ret;
- if (reserved != cluster_size - compr_blocks)
- return -ENOSPC;
+ for (i = 0; i < cluster_size; i++, dn->ofs_in_node++) {
+ if (f2fs_data_blkaddr(dn) == NULL_ADDR)
+ f2fs_set_data_blkaddr(dn, NEW_ADDR);
+ }
f2fs_i_compr_blocks_update(dn->inode, compr_blocks, true);
diff --gi
