As Jungyeon reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=203229
- Overview
When mounting the attached crafted image, following errors are reported.
Additionally, it hangs on sync after trying to mount it.
The image is intentionally fuzzed from a normal f2fs image for testing.
Compile options for F2FS are as follows.
CONFIG_F2FS_FS=y
CONFIG_F2FS_STAT_FS=y
CONFIG_F2FS_FS_XATTR=y
CONFIG_F2FS_FS_POSIX_ACL=y
CONFIG_F2FS_CHECK_FS=y
- Reproduces
mkdir test
mount -t f2fs tmp.img test
sync
- Kernel message
kernel BUG at fs/f2fs/recovery.c:591!
RIP: 0010:recover_data+0x12d8/0x1780
Call Trace:
f2fs_recover_fsync_data+0x613/0x710
f2fs_fill_super+0x1043/0x1aa0
mount_bdev+0x16d/0x1a0
mount_fs+0x4a/0x170
vfs_kern_mount+0x5d/0x100
do_mount+0x200/0xcf0
ksys_mount+0x79/0xc0
__x64_sys_mount+0x1c/0x20
do_syscall_64+0x43/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
With corrupted image wihch has out-of-range valid node/block count, during
recovery, once we failed due to no free space, it will trigger kernel
panic.
Adding sanity check on valid node/block count in f2fs_sanity_check_ckpt()
to detect such condition, so that potential panic can be avoided.
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/super.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 2cd78583218a..cbbb1e35070d 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2592,7 +2592,8 @@ int f2fs_sanity_check_ckpt(struct f2fs_sb_info *sbi)
unsigned int log_blocks_per_seg;
unsigned int segment_count_main;
unsigned int cp_pack_start_sum, cp_payload;
- block_t user_block_count;
+ block_t user_block_count, valid_user_blocks;
+ block_t avail_node_count, valid_node_count;
int i, j;
total = le32_to_cpu(raw_super->segment_count);
@@ -2627,6 +2628,24 @@ int f2fs_sanity_check_ckpt(struct f2fs_sb_info *sbi)
return 1;
}
+ valid_user_blocks = le64_to_cpu(ckpt->valid_block_count);
+ if (valid_user_blocks > user_block_count) {
+ f2fs_msg(sbi->sb, KERN_ERR,
+ "Wrong valid_user_blocks: %u, user_block_count: %u",
+ valid_user_blocks, user_block_count);
+ return 1;
+ }
+
+ valid_node_count = le32_to_cpu(ckpt->valid_node_count);
+ avail_node_count = sbi->total_node_count - sbi->nquota_files -
+ F2FS_RESERVED_NODE_NUM;
+ if (valid_node_count > avail_node_count) {
+ f2fs_msg(sbi->sb, KERN_ERR,
+ "Wrong valid_node_count: %u, avail_node_count: %u",
+ valid_node_count, avail_node_count);
+ return 1;
+ }
+
main_segs = le32_to_cpu(raw_super->segment_count_main);
blocks_per_seg = sbi->blocks_per_seg;
--
2.18.0.rc1
As Jungyeon reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=203233
- Overview
When mounting the attached crafted image and running program, following errors are reported.
Additionally, it hangs on sync after running program.
The image is intentionally fuzzed from a normal f2fs image for testing.
Compile options for F2FS are as follows.
CONFIG_F2FS_FS=y
CONFIG_F2FS_STAT_FS=y
CONFIG_F2FS_FS_XATTR=y
CONFIG_F2FS_FS_POSIX_ACL=y
CONFIG_F2FS_CHECK_FS=y
- Reproduces
cc poc_13.c
mkdir test
mount -t f2fs tmp.img test
cp a.out test
cd test
sudo ./a.out
sync
- Kernel messages
F2FS-fs (sdb): Bitmap was wrongly set, blk:4608
kernel BUG at fs/f2fs/segment.c:2102!
RIP: 0010:update_sit_entry+0x394/0x410
Call Trace:
f2fs_allocate_data_block+0x16f/0x660
do_write_page+0x62/0x170
f2fs_do_write_node_page+0x33/0xa0
__write_node_page+0x270/0x4e0
f2fs_sync_node_pages+0x5df/0x670
f2fs_write_checkpoint+0x372/0x1400
f2fs_sync_fs+0xa3/0x130
f2fs_do_sync_file+0x1a6/0x810
do_fsync+0x33/0x60
__x64_sys_fsync+0xb/0x10
do_syscall_64+0x43/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
sit.vblocks and sum valid block count in sit.valid_map may be
inconsistent, segment w/ zero vblocks will be treated as free
segment, while allocating in free segment, we may allocate a
free block, if its bitmap is valid previously, it can cause
kernel crash due to bitmap verification failure.
Anyway, to avoid further serious metadata inconsistence and
corruption, it is necessary and worth to detect SIT
inconsistence. So let's enable check_block_count() to verify
vblocks and valid_map all the time rather than do it only
CONFIG_F2FS_CHECK_FS is enabled.
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/segment.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index b333ecca6ed6..429007b8036e 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -673,7 +673,6 @@ static inline void verify_fio_blkaddr(struct f2fs_io_info *fio)
static inline int check_block_count(struct f2fs_sb_info *sbi,
int segno, struct f2fs_sit_entry *raw_sit)
{
-#ifdef CONFIG_F2FS_CHECK_FS
bool is_valid = test_bit_le(0, raw_sit->valid_map) ? true : false;
int valid_blocks = 0;
int cur_pos = 0, next_pos;
@@ -700,7 +699,7 @@ static inline int check_block_count(struct f2fs_sb_info *sbi,
set_sbi_flag(sbi, SBI_NEED_FSCK);
return -EINVAL;
}
-#endif
+
/* check segment usage, and check boundary of a given segment number */
if (unlikely(GET_SIT_VBLOCKS(raw_sit) > sbi->blocks_per_seg
|| segno > TOTAL_SEGS(sbi) - 1)) {
--
2.18.0.rc1
This patch fixes to set FI_UPDATE_WRITE only if in-place IO was issued.
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/data.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index ee5c7962b0f3..f6191b5a0e48 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1879,9 +1879,10 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
true);
if (PageWriteback(page))
end_page_writeback(page);
+ } else {
+ set_inode_flag(inode, FI_UPDATE_WRITE);
}
trace_f2fs_do_write_data_page(fio->page, IPU);
- set_inode_flag(inode, FI_UPDATE_WRITE);
return err;
}
--
2.18.0.rc1
As JuHyung Park reported in mailing list:
https://sourceforge.net/p/linux-f2fs/mailman/message/36639787/
generic_make_request: Trying to write to read-only block-device loop0 (partno 0)
WARNING: CPU: 0 PID: 23437 at block/blk-core.c:2174 generic_make_request_checks+0x594/0x630
generic_make_request+0x46/0x3d0
submit_bio+0x30/0x110
__submit_merged_bio+0x68/0x390
f2fs_submit_page_write+0x1bb/0x7f0
f2fs_do_write_meta_page+0x7f/0x160
__f2fs_write_meta_page+0x70/0x140
f2fs_sync_meta_pages+0x140/0x250
f2fs_write_checkpoint+0x5c5/0x17b0
f2fs_sync_fs+0x9c/0x110
sync_filesystem+0x66/0x80
f2fs_recover_fsync_data+0x790/0xa30
f2fs_fill_super+0xe4e/0x1980
mount_bdev+0x518/0x610
mount_fs+0x34/0x13f
vfs_kern_mount.part.11+0x4f/0x120
do_mount+0x2d1/0xe40
__x64_sys_mount+0xbf/0xe0
do_syscall_64+0x4a/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
print_req_error: I/O error, dev loop0, sector 4096
If block device is readonly, we should never trigger write IO from
filesystem layer, but previously, orphan recovery didn't consider
such condition, result in triggering above warning, fix it.
Reported-by: JuHyung Park <[email protected]>
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/checkpoint.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index a7ad1b1e5750..90e1bab86269 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -674,6 +674,12 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
if (!is_set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG))
return 0;
+ if (bdev_read_only(sbi->sb->s_bdev)) {
+ f2fs_msg(sbi->sb, KERN_INFO, "write access "
+ "unavailable, skipping orphan cleanup");
+ return 0;
+ }
+
if (s_flags & SB_RDONLY) {
f2fs_msg(sbi->sb, KERN_INFO, "orphan cleanup on readonly fs");
sbi->sb->s_flags &= ~SB_RDONLY;
--
2.18.0.rc1
As Jungyeon reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=203239
- Overview
When mounting the attached crafted image and running program, following errors are reported.
Additionally, it hangs on sync after running program.
The image is intentionally fuzzed from a normal f2fs image for testing.
Compile options for F2FS are as follows.
CONFIG_F2FS_FS=y
CONFIG_F2FS_STAT_FS=y
CONFIG_F2FS_FS_XATTR=y
CONFIG_F2FS_FS_POSIX_ACL=y
CONFIG_F2FS_CHECK_FS=y
- Reproduces
cc poc_15.c
./run.sh f2fs
sync
- Kernel messages
------------[ cut here ]------------
kernel BUG at fs/f2fs/segment.c:3162!
RIP: 0010:f2fs_inplace_write_data+0x12d/0x160
Call Trace:
f2fs_do_write_data_page+0x3c1/0x820
__write_data_page+0x156/0x720
f2fs_write_cache_pages+0x20d/0x460
f2fs_write_data_pages+0x1b4/0x300
do_writepages+0x15/0x60
__filemap_fdatawrite_range+0x7c/0xb0
file_write_and_wait_range+0x2c/0x80
f2fs_do_sync_file+0x102/0x810
do_fsync+0x33/0x60
__x64_sys_fsync+0xb/0x10
do_syscall_64+0x43/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
The reason is f2fs_inplace_write_data() will trigger kernel panic due
to data block locates in node type segment.
To avoid panic, let's just return error code and set SBI_NEED_FSCK to
give a hint to fsck for latter repairing.
Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/segment.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index b59d1c85863b..8388d2abacb5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3184,13 +3184,18 @@ int f2fs_inplace_write_data(struct f2fs_io_info *fio)
{
int err;
struct f2fs_sb_info *sbi = fio->sbi;
+ unsigned int segno;
fio->new_blkaddr = fio->old_blkaddr;
/* i/o temperature is needed for passing down write hints */
__get_segment_type(fio);
- f2fs_bug_on(sbi, !IS_DATASEG(get_seg_entry(sbi,
- GET_SEGNO(sbi, fio->new_blkaddr))->type));
+ segno = GET_SEGNO(sbi, fio->new_blkaddr);
+
+ if (!IS_DATASEG(get_seg_entry(sbi, segno)->type)) {
+ set_sbi_flag(sbi, SBI_NEED_FSCK);
+ return -EFAULT;
+ }
stat_inc_inplace_blocks(fio->sbi);
--
2.18.0.rc1
Hi,
Thanks for the fix. I'll try this sooner than later.
One minor request though, can you change
"JuHyung Park <[email protected]>"
to
"Park Ju Hyung <[email protected]>"?
That's my preference and I'd like to avoid any inconsistencies.
One additional question from reviewing the code surrounding it:
does it really makes sense to cleanup orphan inodes even when the "ro"
mount option is passed?
It's an explicit request from the user not to write to the block device/image.
Thanks.
On Mon, Apr 15, 2019 at 4:31 PM Chao Yu <[email protected]> wrote:
>
> As JuHyung Park reported in mailing list:
>
> https://sourceforge.net/p/linux-f2fs/mailman/message/36639787/
>
> generic_make_request: Trying to write to read-only block-device loop0 (partno 0)
> WARNING: CPU: 0 PID: 23437 at block/blk-core.c:2174 generic_make_request_checks+0x594/0x630
>
> generic_make_request+0x46/0x3d0
> submit_bio+0x30/0x110
> __submit_merged_bio+0x68/0x390
> f2fs_submit_page_write+0x1bb/0x7f0
> f2fs_do_write_meta_page+0x7f/0x160
> __f2fs_write_meta_page+0x70/0x140
> f2fs_sync_meta_pages+0x140/0x250
> f2fs_write_checkpoint+0x5c5/0x17b0
> f2fs_sync_fs+0x9c/0x110
> sync_filesystem+0x66/0x80
> f2fs_recover_fsync_data+0x790/0xa30
> f2fs_fill_super+0xe4e/0x1980
> mount_bdev+0x518/0x610
> mount_fs+0x34/0x13f
> vfs_kern_mount.part.11+0x4f/0x120
> do_mount+0x2d1/0xe40
> __x64_sys_mount+0xbf/0xe0
> do_syscall_64+0x4a/0xf0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> print_req_error: I/O error, dev loop0, sector 4096
>
> If block device is readonly, we should never trigger write IO from
> filesystem layer, but previously, orphan recovery didn't consider
> such condition, result in triggering above warning, fix it.
>
> Reported-by: JuHyung Park <[email protected]>
> Signed-off-by: Chao Yu <[email protected]>
> ---
> fs/f2fs/checkpoint.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index a7ad1b1e5750..90e1bab86269 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -674,6 +674,12 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
> if (!is_set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG))
> return 0;
>
> + if (bdev_read_only(sbi->sb->s_bdev)) {
> + f2fs_msg(sbi->sb, KERN_INFO, "write access "
> + "unavailable, skipping orphan cleanup");
> + return 0;
> + }
> +
> if (s_flags & SB_RDONLY) {
> f2fs_msg(sbi->sb, KERN_INFO, "orphan cleanup on readonly fs");
> sbi->sb->s_flags &= ~SB_RDONLY;
> --
> 2.18.0.rc1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2019/4/15 16:10, Ju Hyung Park wrote:
> Hi,
>
> Thanks for the fix. I'll try this sooner than later.
>
> One minor request though, can you change
> "JuHyung Park <[email protected]>"
> to
> "Park Ju Hyung <[email protected]>"?
>
> That's my preference and I'd like to avoid any inconsistencies.
Sure, will update it in next version. :)
>
> One additional question from reviewing the code surrounding it:
> does it really makes sense to cleanup orphan inodes even when the "ro"
> mount option is passed?
> It's an explicit request from the user not to write to the block device/image.
Now, f2fs follows the rule that ext4 kept, you can check codes in
ext4_orphan_cleanup()
if (bdev_read_only(sb->s_bdev)) {
ext4_msg(sb, KERN_ERR, "write access "
"unavailable, skipping orphan cleanup");
return;
}
...
if (s_flags & SB_RDONLY) {
ext4_msg(sb, KERN_INFO, "orphan cleanup on readonly fs");
sb->s_flags &= ~SB_RDONLY;
}
There are two points in above codes:
- if block device is readonly, filesystem should not execute any recovery flow
which can trigger write IO.
- if filesystem was mounted as readonly one, and recovery is needed, it will
ignore readonly flag and update data in device for journal recovery during mount.
So IMO, readonly mountoption sematics is only try to restrict data/meta update
behavior that is triggered by user from mountpoint, but filesystem still can do
any updates on a writable device if it needs, mostly like recovery flow.
Anyway, if you want to limit any updates on block device, making it readonly
will be a good choice. :)
Thanks,
>
> Thanks.
>
> On Mon, Apr 15, 2019 at 4:31 PM Chao Yu <[email protected]> wrote:
>>
>> As JuHyung Park reported in mailing list:
>>
>> https://sourceforge.net/p/linux-f2fs/mailman/message/36639787/
>>
>> generic_make_request: Trying to write to read-only block-device loop0 (partno 0)
>> WARNING: CPU: 0 PID: 23437 at block/blk-core.c:2174 generic_make_request_checks+0x594/0x630
>>
>> generic_make_request+0x46/0x3d0
>> submit_bio+0x30/0x110
>> __submit_merged_bio+0x68/0x390
>> f2fs_submit_page_write+0x1bb/0x7f0
>> f2fs_do_write_meta_page+0x7f/0x160
>> __f2fs_write_meta_page+0x70/0x140
>> f2fs_sync_meta_pages+0x140/0x250
>> f2fs_write_checkpoint+0x5c5/0x17b0
>> f2fs_sync_fs+0x9c/0x110
>> sync_filesystem+0x66/0x80
>> f2fs_recover_fsync_data+0x790/0xa30
>> f2fs_fill_super+0xe4e/0x1980
>> mount_bdev+0x518/0x610
>> mount_fs+0x34/0x13f
>> vfs_kern_mount.part.11+0x4f/0x120
>> do_mount+0x2d1/0xe40
>> __x64_sys_mount+0xbf/0xe0
>> do_syscall_64+0x4a/0xf0
>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> print_req_error: I/O error, dev loop0, sector 4096
>>
>> If block device is readonly, we should never trigger write IO from
>> filesystem layer, but previously, orphan recovery didn't consider
>> such condition, result in triggering above warning, fix it.
>>
>> Reported-by: JuHyung Park <[email protected]>
>> Signed-off-by: Chao Yu <[email protected]>
>> ---
>> fs/f2fs/checkpoint.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index a7ad1b1e5750..90e1bab86269 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -674,6 +674,12 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
>> if (!is_set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG))
>> return 0;
>>
>> + if (bdev_read_only(sbi->sb->s_bdev)) {
>> + f2fs_msg(sbi->sb, KERN_INFO, "write access "
>> + "unavailable, skipping orphan cleanup");
>> + return 0;
>> + }
>> +
>> if (s_flags & SB_RDONLY) {
>> f2fs_msg(sbi->sb, KERN_INFO, "orphan cleanup on readonly fs");
>> sbi->sb->s_flags &= ~SB_RDONLY;
>> --
>> 2.18.0.rc1
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
Thanks for the explanation.
And yes, this patch fixed it, the kernel log is now clean.
Thanks!
[ 22.506553] F2FS-fs (loop0): write access unavailable, skipping
orphan cleanup
[ 22.506555] F2FS-fs (loop0): recover fsync data on readonly fs
[ 22.506556] F2FS-fs (loop0): quota file may be corrupted, skip loading it
[ 22.507015] F2FS-fs (loop0): Mounted with checkpoint version = 26e7ba3e
On Mon, Apr 15, 2019 at 5:57 PM Chao Yu <[email protected]> wrote:
>
> On 2019/4/15 16:10, Ju Hyung Park wrote:
> > Hi,
> >
> > Thanks for the fix. I'll try this sooner than later.
> >
> > One minor request though, can you change
> > "JuHyung Park <[email protected]>"
> > to
> > "Park Ju Hyung <[email protected]>"?
> >
> > That's my preference and I'd like to avoid any inconsistencies.
>
> Sure, will update it in next version. :)
>
> >
> > One additional question from reviewing the code surrounding it:
> > does it really makes sense to cleanup orphan inodes even when the "ro"
> > mount option is passed?
> > It's an explicit request from the user not to write to the block device/image.
>
> Now, f2fs follows the rule that ext4 kept, you can check codes in
> ext4_orphan_cleanup()
>
> if (bdev_read_only(sb->s_bdev)) {
> ext4_msg(sb, KERN_ERR, "write access "
> "unavailable, skipping orphan cleanup");
> return;
> }
> ...
> if (s_flags & SB_RDONLY) {
> ext4_msg(sb, KERN_INFO, "orphan cleanup on readonly fs");
> sb->s_flags &= ~SB_RDONLY;
> }
>
> There are two points in above codes:
> - if block device is readonly, filesystem should not execute any recovery flow
> which can trigger write IO.
> - if filesystem was mounted as readonly one, and recovery is needed, it will
> ignore readonly flag and update data in device for journal recovery during mount.
>
> So IMO, readonly mountoption sematics is only try to restrict data/meta update
> behavior that is triggered by user from mountpoint, but filesystem still can do
> any updates on a writable device if it needs, mostly like recovery flow.
>
> Anyway, if you want to limit any updates on block device, making it readonly
> will be a good choice. :)
>
> Thanks,
>
> >
> > Thanks.
> >
> > On Mon, Apr 15, 2019 at 4:31 PM Chao Yu <[email protected]> wrote:
> >>
> >> As JuHyung Park reported in mailing list:
> >>
> >> https://sourceforge.net/p/linux-f2fs/mailman/message/36639787/
> >>
> >> generic_make_request: Trying to write to read-only block-device loop0 (partno 0)
> >> WARNING: CPU: 0 PID: 23437 at block/blk-core.c:2174 generic_make_request_checks+0x594/0x630
> >>
> >> generic_make_request+0x46/0x3d0
> >> submit_bio+0x30/0x110
> >> __submit_merged_bio+0x68/0x390
> >> f2fs_submit_page_write+0x1bb/0x7f0
> >> f2fs_do_write_meta_page+0x7f/0x160
> >> __f2fs_write_meta_page+0x70/0x140
> >> f2fs_sync_meta_pages+0x140/0x250
> >> f2fs_write_checkpoint+0x5c5/0x17b0
> >> f2fs_sync_fs+0x9c/0x110
> >> sync_filesystem+0x66/0x80
> >> f2fs_recover_fsync_data+0x790/0xa30
> >> f2fs_fill_super+0xe4e/0x1980
> >> mount_bdev+0x518/0x610
> >> mount_fs+0x34/0x13f
> >> vfs_kern_mount.part.11+0x4f/0x120
> >> do_mount+0x2d1/0xe40
> >> __x64_sys_mount+0xbf/0xe0
> >> do_syscall_64+0x4a/0xf0
> >> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >>
> >> print_req_error: I/O error, dev loop0, sector 4096
> >>
> >> If block device is readonly, we should never trigger write IO from
> >> filesystem layer, but previously, orphan recovery didn't consider
> >> such condition, result in triggering above warning, fix it.
> >>
> >> Reported-by: JuHyung Park <[email protected]>
> >> Signed-off-by: Chao Yu <[email protected]>
> >> ---
> >> fs/f2fs/checkpoint.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >> index a7ad1b1e5750..90e1bab86269 100644
> >> --- a/fs/f2fs/checkpoint.c
> >> +++ b/fs/f2fs/checkpoint.c
> >> @@ -674,6 +674,12 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
> >> if (!is_set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG))
> >> return 0;
> >>
> >> + if (bdev_read_only(sbi->sb->s_bdev)) {
> >> + f2fs_msg(sbi->sb, KERN_INFO, "write access "
> >> + "unavailable, skipping orphan cleanup");
> >> + return 0;
> >> + }
> >> +
> >> if (s_flags & SB_RDONLY) {
> >> f2fs_msg(sbi->sb, KERN_INFO, "orphan cleanup on readonly fs");
> >> sbi->sb->s_flags &= ~SB_RDONLY;
> >> --
> >> 2.18.0.rc1
> >>
> >>
> >>
> >> _______________________________________________
> >> Linux-f2fs-devel mailing list
> >> [email protected]
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >
On 2019/4/15 19:04, Ju Hyung Park wrote:
> Thanks for the explanation.
>
> And yes, this patch fixed it, the kernel log is now clean.
Thanks for the quick test. :)
Thanks,
>
> Thanks!
>
> [ 22.506553] F2FS-fs (loop0): write access unavailable, skipping
> orphan cleanup
> [ 22.506555] F2FS-fs (loop0): recover fsync data on readonly fs
> [ 22.506556] F2FS-fs (loop0): quota file may be corrupted, skip loading it
> [ 22.507015] F2FS-fs (loop0): Mounted with checkpoint version = 26e7ba3e
>
> On Mon, Apr 15, 2019 at 5:57 PM Chao Yu <[email protected]> wrote:
>>
>> On 2019/4/15 16:10, Ju Hyung Park wrote:
>>> Hi,
>>>
>>> Thanks for the fix. I'll try this sooner than later.
>>>
>>> One minor request though, can you change
>>> "JuHyung Park <[email protected]>"
>>> to
>>> "Park Ju Hyung <[email protected]>"?
>>>
>>> That's my preference and I'd like to avoid any inconsistencies.
>>
>> Sure, will update it in next version. :)
>>
>>>
>>> One additional question from reviewing the code surrounding it:
>>> does it really makes sense to cleanup orphan inodes even when the "ro"
>>> mount option is passed?
>>> It's an explicit request from the user not to write to the block device/image.
>>
>> Now, f2fs follows the rule that ext4 kept, you can check codes in
>> ext4_orphan_cleanup()
>>
>> if (bdev_read_only(sb->s_bdev)) {
>> ext4_msg(sb, KERN_ERR, "write access "
>> "unavailable, skipping orphan cleanup");
>> return;
>> }
>> ...
>> if (s_flags & SB_RDONLY) {
>> ext4_msg(sb, KERN_INFO, "orphan cleanup on readonly fs");
>> sb->s_flags &= ~SB_RDONLY;
>> }
>>
>> There are two points in above codes:
>> - if block device is readonly, filesystem should not execute any recovery flow
>> which can trigger write IO.
>> - if filesystem was mounted as readonly one, and recovery is needed, it will
>> ignore readonly flag and update data in device for journal recovery during mount.
>>
>> So IMO, readonly mountoption sematics is only try to restrict data/meta update
>> behavior that is triggered by user from mountpoint, but filesystem still can do
>> any updates on a writable device if it needs, mostly like recovery flow.
>>
>> Anyway, if you want to limit any updates on block device, making it readonly
>> will be a good choice. :)
>>
>> Thanks,
>>
>>>
>>> Thanks.
>>>
>>> On Mon, Apr 15, 2019 at 4:31 PM Chao Yu <[email protected]> wrote:
>>>>
>>>> As JuHyung Park reported in mailing list:
>>>>
>>>> https://sourceforge.net/p/linux-f2fs/mailman/message/36639787/
>>>>
>>>> generic_make_request: Trying to write to read-only block-device loop0 (partno 0)
>>>> WARNING: CPU: 0 PID: 23437 at block/blk-core.c:2174 generic_make_request_checks+0x594/0x630
>>>>
>>>> generic_make_request+0x46/0x3d0
>>>> submit_bio+0x30/0x110
>>>> __submit_merged_bio+0x68/0x390
>>>> f2fs_submit_page_write+0x1bb/0x7f0
>>>> f2fs_do_write_meta_page+0x7f/0x160
>>>> __f2fs_write_meta_page+0x70/0x140
>>>> f2fs_sync_meta_pages+0x140/0x250
>>>> f2fs_write_checkpoint+0x5c5/0x17b0
>>>> f2fs_sync_fs+0x9c/0x110
>>>> sync_filesystem+0x66/0x80
>>>> f2fs_recover_fsync_data+0x790/0xa30
>>>> f2fs_fill_super+0xe4e/0x1980
>>>> mount_bdev+0x518/0x610
>>>> mount_fs+0x34/0x13f
>>>> vfs_kern_mount.part.11+0x4f/0x120
>>>> do_mount+0x2d1/0xe40
>>>> __x64_sys_mount+0xbf/0xe0
>>>> do_syscall_64+0x4a/0xf0
>>>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>>
>>>> print_req_error: I/O error, dev loop0, sector 4096
>>>>
>>>> If block device is readonly, we should never trigger write IO from
>>>> filesystem layer, but previously, orphan recovery didn't consider
>>>> such condition, result in triggering above warning, fix it.
>>>>
>>>> Reported-by: JuHyung Park <[email protected]>
>>>> Signed-off-by: Chao Yu <[email protected]>
>>>> ---
>>>> fs/f2fs/checkpoint.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>> index a7ad1b1e5750..90e1bab86269 100644
>>>> --- a/fs/f2fs/checkpoint.c
>>>> +++ b/fs/f2fs/checkpoint.c
>>>> @@ -674,6 +674,12 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
>>>> if (!is_set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG))
>>>> return 0;
>>>>
>>>> + if (bdev_read_only(sbi->sb->s_bdev)) {
>>>> + f2fs_msg(sbi->sb, KERN_INFO, "write access "
>>>> + "unavailable, skipping orphan cleanup");
>>>> + return 0;
>>>> + }
>>>> +
>>>> if (s_flags & SB_RDONLY) {
>>>> f2fs_msg(sbi->sb, KERN_INFO, "orphan cleanup on readonly fs");
>>>> sbi->sb->s_flags &= ~SB_RDONLY;
>>>> --
>>>> 2.18.0.rc1
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-f2fs-devel mailing list
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>> .
>>>
> .
>