[PATCH 09/13] f2fs: fix to do sanity check on valid node/block count

[PATCH 09/13] f2fs: fix to do sanity check on valid node/block count

[Chao Yu]

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 <yuchao0@huawei.com>
---
 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

[PATCH 10/13] f2fs: fix to do sanity check on valid block count of segment

[Chao Yu]

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 <yuchao0@huawei.com>
---
 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

[PATCH 11/13] f2fs: fix to avoid panic in f2fs_inplace_write_data()

[Chao Yu]

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 <yuchao0@huawei.com>
---
 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

[PATCH 12/13] f2fs: fix to set FI_UPDATE_WRITE correctly

[Chao Yu]

This patch fixes to set FI_UPDATE_WRITE only if in-place IO was issued.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 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

[PATCH 13/13] f2fs: don't recovery orphan inode on readonly device

[Chao Yu]

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 <qkrwngud825@gmail.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 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

Re: [f2fs-dev] [PATCH 13/13] f2fs: don't recovery orphan inode on readonly device

[Ju Hyung Park]

Hi,

Thanks for the fix. I'll try this sooner than later.

One minor request though, can you change
"JuHyung Park <qkrwngud825@gmail.com>"
to
"Park Ju Hyung <qkrwngud825@gmail.com>"?

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 <yuchao0@huawei.com> 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 <qkrwngud825@gmail.com>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 13/13] f2fs: don't recovery orphan inode on readonly device

[Chao Yu]

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 <qkrwngud825@gmail.com>"
> to
> "Park Ju Hyung <qkrwngud825@gmail.com>"?
> 
> 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 <yuchao0@huawei.com> 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 <qkrwngud825@gmail.com>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  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
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 

Re: [f2fs-dev] [PATCH 13/13] f2fs: don't recovery orphan inode on readonly device

[Ju Hyung Park]

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 <yuchao0@huawei.com> 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 <qkrwngud825@gmail.com>"
> > to
> > "Park Ju Hyung <qkrwngud825@gmail.com>"?
> >
> > 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 <yuchao0@huawei.com> 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 <qkrwngud825@gmail.com>
> >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >>  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
> >> Linux-f2fs-devel@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >

Re: [f2fs-dev] [PATCH 13/13] f2fs: don't recovery orphan inode on readonly device

[Chao Yu]

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 <yuchao0@huawei.com> 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 <qkrwngud825@gmail.com>"
>>> to
>>> "Park Ju Hyung <qkrwngud825@gmail.com>"?
>>>
>>> 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 <yuchao0@huawei.com> 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 <qkrwngud825@gmail.com>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>  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
>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>> .
>>>
> .
>