[f2fs-dev] [PATCH v2 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[f2fs-dev] [PATCH v2 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

[1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html

As [1] reported, if lower device doesn't support write barrier, in below
case:

- write page #0; persist
- overwrite page #0
- fsync
 - write data page #0 OPU into device's cache
 - write inode page into device's cache
 - issue flush

If SPO is triggered during flush command, inode page can be persisted
before data page #0, so that after recovery, inode page can be recovered
with new physical block address of data page #0, however there may
contains dummy data in new physical block address.

Then what user will see is: after overwrite & fsync + SPO, old data in
file was corrupted, if any user do care about such case, we can suggest
user to use STRICT fsync mode, in this mode, we will force to trigger
preflush command to persist data in device cache in prior to node
writeback, it avoids potential data corruption during fsync().

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2:
- fix this by adding additional preflush command rather than using
atomic write flow.
 fs/f2fs/file.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7d5311d54f63..238ca2a733ac 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
 			goto flush_out;
 		goto out;
+	} else {
+		/*
+		 * for OPU case, during fsync(), node can be persisted before
+		 * data when lower device doesn't support write barrier, result
+		 * in data corruption after SPO.
+		 * So for strict fsync mode, force to trigger preflush to keep
+		 * data/node write order to avoid potential data corruption.
+		 */
+		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
+								!atomic) {
+			ret = f2fs_issue_flush(sbi, inode->i_ino);
+			if (ret)
+				goto out;
+		}
 	}
 go_write:
 	/*
-- 
2.29.2



_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

Jaegeuk,

Any comments on this patch?

On 2021/6/1 18:10, Chao Yu wrote:
> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> 
> As [1] reported, if lower device doesn't support write barrier, in below
> case:
> 
> - write page #0; persist
> - overwrite page #0
> - fsync
>   - write data page #0 OPU into device's cache
>   - write inode page into device's cache
>   - issue flush
> 
> If SPO is triggered during flush command, inode page can be persisted
> before data page #0, so that after recovery, inode page can be recovered
> with new physical block address of data page #0, however there may
> contains dummy data in new physical block address.
> 
> Then what user will see is: after overwrite & fsync + SPO, old data in
> file was corrupted, if any user do care about such case, we can suggest
> user to use STRICT fsync mode, in this mode, we will force to trigger
> preflush command to persist data in device cache in prior to node
> writeback, it avoids potential data corruption during fsync().
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v2:
> - fix this by adding additional preflush command rather than using
> atomic write flow.
>   fs/f2fs/file.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 7d5311d54f63..238ca2a733ac 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>   				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>   			goto flush_out;
>   		goto out;
> +	} else {
> +		/*
> +		 * for OPU case, during fsync(), node can be persisted before
> +		 * data when lower device doesn't support write barrier, result
> +		 * in data corruption after SPO.
> +		 * So for strict fsync mode, force to trigger preflush to keep
> +		 * data/node write order to avoid potential data corruption.
> +		 */
> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> +								!atomic) {
> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> +			if (ret)
> +				goto out;
> +		}
>   	}
>   go_write:
>   	/*
> 


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

Still no time to check this?

Thanks,

On 2021/6/4 0:00, Chao Yu wrote:
> Jaegeuk,
> 
> Any comments on this patch?
> 
> On 2021/6/1 18:10, Chao Yu wrote:
>> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
>>
>> As [1] reported, if lower device doesn't support write barrier, in below
>> case:
>>
>> - write page #0; persist
>> - overwrite page #0
>> - fsync
>>   - write data page #0 OPU into device's cache
>>   - write inode page into device's cache
>>   - issue flush
>>
>> If SPO is triggered during flush command, inode page can be persisted
>> before data page #0, so that after recovery, inode page can be recovered
>> with new physical block address of data page #0, however there may
>> contains dummy data in new physical block address.
>>
>> Then what user will see is: after overwrite & fsync + SPO, old data in
>> file was corrupted, if any user do care about such case, we can suggest
>> user to use STRICT fsync mode, in this mode, we will force to trigger
>> preflush command to persist data in device cache in prior to node
>> writeback, it avoids potential data corruption during fsync().
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> v2:
>> - fix this by adding additional preflush command rather than using
>> atomic write flow.
>>   fs/f2fs/file.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 7d5311d54f63..238ca2a733ac 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>                   f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>>               goto flush_out;
>>           goto out;
>> +    } else {
>> +        /*
>> +         * for OPU case, during fsync(), node can be persisted before
>> +         * data when lower device doesn't support write barrier, result
>> +         * in data corruption after SPO.
>> +         * So for strict fsync mode, force to trigger preflush to keep
>> +         * data/node write order to avoid potential data corruption.
>> +         */
>> +        if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
>> +                                !atomic) {
>> +            ret = f2fs_issue_flush(sbi, inode->i_ino);
>> +            if (ret)
>> +                goto out;
>> +        }
>>       }
>>   go_write:
>>       /*
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Jaegeuk Kim]

On 06/01, Chao Yu wrote:
> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> 
> As [1] reported, if lower device doesn't support write barrier, in below
> case:
> 
> - write page #0; persist
> - overwrite page #0
> - fsync
>  - write data page #0 OPU into device's cache
>  - write inode page into device's cache
>  - issue flush

Well, we have preflush for node writes, so I don't think this is the case.

 fio.op_flags |= REQ_PREFLUSH | REQ_FUA;

> 
> If SPO is triggered during flush command, inode page can be persisted
> before data page #0, so that after recovery, inode page can be recovered
> with new physical block address of data page #0, however there may
> contains dummy data in new physical block address.
> 
> Then what user will see is: after overwrite & fsync + SPO, old data in
> file was corrupted, if any user do care about such case, we can suggest
> user to use STRICT fsync mode, in this mode, we will force to trigger
> preflush command to persist data in device cache in prior to node
> writeback, it avoids potential data corruption during fsync().
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v2:
> - fix this by adding additional preflush command rather than using
> atomic write flow.
>  fs/f2fs/file.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 7d5311d54f63..238ca2a733ac 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>  			goto flush_out;
>  		goto out;
> +	} else {
> +		/*
> +		 * for OPU case, during fsync(), node can be persisted before
> +		 * data when lower device doesn't support write barrier, result
> +		 * in data corruption after SPO.
> +		 * So for strict fsync mode, force to trigger preflush to keep
> +		 * data/node write order to avoid potential data corruption.
> +		 */
> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> +								!atomic) {
> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> +			if (ret)
> +				goto out;
> +		}
>  	}
>  go_write:
>  	/*
> -- 
> 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

On 2021/7/2 1:10, Jaegeuk Kim wrote:
> On 06/01, Chao Yu wrote:
>> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
>>
>> As [1] reported, if lower device doesn't support write barrier, in below
>> case:
>>
>> - write page #0; persist
>> - overwrite page #0
>> - fsync
>>   - write data page #0 OPU into device's cache
>>   - write inode page into device's cache
>>   - issue flush
> 
> Well, we have preflush for node writes, so I don't think this is the case.
> 
>   fio.op_flags |= REQ_PREFLUSH | REQ_FUA;

This is only used for atomic write case, right?

I mean the common case which is called from f2fs_issue_flush() in
f2fs_do_sync_file().

And please see do_checkpoint(), we call f2fs_flush_device_cache() and
commit_checkpoint() separately to keep persistence order of CP datas.

See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
for details.

Thanks,

> 
>>
>> If SPO is triggered during flush command, inode page can be persisted
>> before data page #0, so that after recovery, inode page can be recovered
>> with new physical block address of data page #0, however there may
>> contains dummy data in new physical block address.
>>
>> Then what user will see is: after overwrite & fsync + SPO, old data in
>> file was corrupted, if any user do care about such case, we can suggest
>> user to use STRICT fsync mode, in this mode, we will force to trigger
>> preflush command to persist data in device cache in prior to node
>> writeback, it avoids potential data corruption during fsync().
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> v2:
>> - fix this by adding additional preflush command rather than using
>> atomic write flow.
>>   fs/f2fs/file.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 7d5311d54f63..238ca2a733ac 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>   				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>>   			goto flush_out;
>>   		goto out;
>> +	} else {
>> +		/*
>> +		 * for OPU case, during fsync(), node can be persisted before
>> +		 * data when lower device doesn't support write barrier, result
>> +		 * in data corruption after SPO.
>> +		 * So for strict fsync mode, force to trigger preflush to keep
>> +		 * data/node write order to avoid potential data corruption.
>> +		 */
>> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
>> +								!atomic) {
>> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
>> +			if (ret)
>> +				goto out;
>> +		}
>>   	}
>>   go_write:
>>   	/*
>> -- 
>> 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Jaegeuk Kim]

On 07/02, Chao Yu wrote:
> On 2021/7/2 1:10, Jaegeuk Kim wrote:
> > On 06/01, Chao Yu wrote:
> > > [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> > > 
> > > As [1] reported, if lower device doesn't support write barrier, in below
> > > case:
> > > 
> > > - write page #0; persist
> > > - overwrite page #0
> > > - fsync
> > >   - write data page #0 OPU into device's cache
> > >   - write inode page into device's cache
> > >   - issue flush
> > 
> > Well, we have preflush for node writes, so I don't think this is the case.
> > 
> >   fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> 
> This is only used for atomic write case, right?
> 
> I mean the common case which is called from f2fs_issue_flush() in
> f2fs_do_sync_file().

How about adding PREFLUSH when writing node blocks aligned to the above set?

> 
> And please see do_checkpoint(), we call f2fs_flush_device_cache() and
> commit_checkpoint() separately to keep persistence order of CP datas.
> 
> See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
> for details.
> 
> Thanks,
> 
> > 
> > > 
> > > If SPO is triggered during flush command, inode page can be persisted
> > > before data page #0, so that after recovery, inode page can be recovered
> > > with new physical block address of data page #0, however there may
> > > contains dummy data in new physical block address.
> > > 
> > > Then what user will see is: after overwrite & fsync + SPO, old data in
> > > file was corrupted, if any user do care about such case, we can suggest
> > > user to use STRICT fsync mode, in this mode, we will force to trigger
> > > preflush command to persist data in device cache in prior to node
> > > writeback, it avoids potential data corruption during fsync().
> > > 
> > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > ---
> > > v2:
> > > - fix this by adding additional preflush command rather than using
> > > atomic write flow.
> > >   fs/f2fs/file.c | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > index 7d5311d54f63..238ca2a733ac 100644
> > > --- a/fs/f2fs/file.c
> > > +++ b/fs/f2fs/file.c
> > > @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
> > >   				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
> > >   			goto flush_out;
> > >   		goto out;
> > > +	} else {
> > > +		/*
> > > +		 * for OPU case, during fsync(), node can be persisted before
> > > +		 * data when lower device doesn't support write barrier, result
> > > +		 * in data corruption after SPO.
> > > +		 * So for strict fsync mode, force to trigger preflush to keep
> > > +		 * data/node write order to avoid potential data corruption.
> > > +		 */
> > > +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> > > +								!atomic) {
> > > +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> > > +			if (ret)
> > > +				goto out;
> > > +		}
> > >   	}
> > >   go_write:
> > >   	/*
> > > -- 
> > > 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

On 2021/7/2 9:32, Jaegeuk Kim wrote:
> On 07/02, Chao Yu wrote:
>> On 2021/7/2 1:10, Jaegeuk Kim wrote:
>>> On 06/01, Chao Yu wrote:
>>>> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
>>>>
>>>> As [1] reported, if lower device doesn't support write barrier, in below
>>>> case:
>>>>
>>>> - write page #0; persist
>>>> - overwrite page #0
>>>> - fsync
>>>>    - write data page #0 OPU into device's cache
>>>>    - write inode page into device's cache
>>>>    - issue flush
>>>
>>> Well, we have preflush for node writes, so I don't think this is the case.
>>>
>>>    fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
>>
>> This is only used for atomic write case, right?
>>
>> I mean the common case which is called from f2fs_issue_flush() in
>> f2fs_do_sync_file().
> 
> How about adding PREFLUSH when writing node blocks aligned to the above set?

You mean implementation like v1 as below?

https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/

Thanks,

> 
>>
>> And please see do_checkpoint(), we call f2fs_flush_device_cache() and
>> commit_checkpoint() separately to keep persistence order of CP datas.
>>
>> See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
>> for details.
>>
>> Thanks,
>>
>>>
>>>>
>>>> If SPO is triggered during flush command, inode page can be persisted
>>>> before data page #0, so that after recovery, inode page can be recovered
>>>> with new physical block address of data page #0, however there may
>>>> contains dummy data in new physical block address.
>>>>
>>>> Then what user will see is: after overwrite & fsync + SPO, old data in
>>>> file was corrupted, if any user do care about such case, we can suggest
>>>> user to use STRICT fsync mode, in this mode, we will force to trigger
>>>> preflush command to persist data in device cache in prior to node
>>>> writeback, it avoids potential data corruption during fsync().
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>> v2:
>>>> - fix this by adding additional preflush command rather than using
>>>> atomic write flow.
>>>>    fs/f2fs/file.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index 7d5311d54f63..238ca2a733ac 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>    				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>>>>    			goto flush_out;
>>>>    		goto out;
>>>> +	} else {
>>>> +		/*
>>>> +		 * for OPU case, during fsync(), node can be persisted before
>>>> +		 * data when lower device doesn't support write barrier, result
>>>> +		 * in data corruption after SPO.
>>>> +		 * So for strict fsync mode, force to trigger preflush to keep
>>>> +		 * data/node write order to avoid potential data corruption.
>>>> +		 */
>>>> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
>>>> +								!atomic) {
>>>> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>> +			if (ret)
>>>> +				goto out;
>>>> +		}
>>>>    	}
>>>>    go_write:
>>>>    	/*
>>>> -- 
>>>> 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Jaegeuk Kim]

On 07/02, Chao Yu wrote:
> On 2021/7/2 9:32, Jaegeuk Kim wrote:
> > On 07/02, Chao Yu wrote:
> > > On 2021/7/2 1:10, Jaegeuk Kim wrote:
> > > > On 06/01, Chao Yu wrote:
> > > > > [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> > > > > 
> > > > > As [1] reported, if lower device doesn't support write barrier, in below
> > > > > case:
> > > > > 
> > > > > - write page #0; persist
> > > > > - overwrite page #0
> > > > > - fsync
> > > > >    - write data page #0 OPU into device's cache
> > > > >    - write inode page into device's cache
> > > > >    - issue flush
> > > > 
> > > > Well, we have preflush for node writes, so I don't think this is the case.
> > > > 
> > > >    fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> > > 
> > > This is only used for atomic write case, right?
> > > 
> > > I mean the common case which is called from f2fs_issue_flush() in
> > > f2fs_do_sync_file().
> > 
> > How about adding PREFLUSH when writing node blocks aligned to the above set?
> 
> You mean implementation like v1 as below?
> 
> https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/

Yea, I think so. :P

> 
> Thanks,
> 
> > 
> > > 
> > > And please see do_checkpoint(), we call f2fs_flush_device_cache() and
> > > commit_checkpoint() separately to keep persistence order of CP datas.
> > > 
> > > See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
> > > for details.
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > If SPO is triggered during flush command, inode page can be persisted
> > > > > before data page #0, so that after recovery, inode page can be recovered
> > > > > with new physical block address of data page #0, however there may
> > > > > contains dummy data in new physical block address.
> > > > > 
> > > > > Then what user will see is: after overwrite & fsync + SPO, old data in
> > > > > file was corrupted, if any user do care about such case, we can suggest
> > > > > user to use STRICT fsync mode, in this mode, we will force to trigger
> > > > > preflush command to persist data in device cache in prior to node
> > > > > writeback, it avoids potential data corruption during fsync().
> > > > > 
> > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > ---
> > > > > v2:
> > > > > - fix this by adding additional preflush command rather than using
> > > > > atomic write flow.
> > > > >    fs/f2fs/file.c | 14 ++++++++++++++
> > > > >    1 file changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > > index 7d5311d54f63..238ca2a733ac 100644
> > > > > --- a/fs/f2fs/file.c
> > > > > +++ b/fs/f2fs/file.c
> > > > > @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
> > > > >    				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
> > > > >    			goto flush_out;
> > > > >    		goto out;
> > > > > +	} else {
> > > > > +		/*
> > > > > +		 * for OPU case, during fsync(), node can be persisted before
> > > > > +		 * data when lower device doesn't support write barrier, result
> > > > > +		 * in data corruption after SPO.
> > > > > +		 * So for strict fsync mode, force to trigger preflush to keep
> > > > > +		 * data/node write order to avoid potential data corruption.
> > > > > +		 */
> > > > > +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> > > > > +								!atomic) {
> > > > > +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> > > > > +			if (ret)
> > > > > +				goto out;
> > > > > +		}
> > > > >    	}
> > > > >    go_write:
> > > > >    	/*
> > > > > -- 
> > > > > 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

On 2021/7/8 1:48, Jaegeuk Kim wrote:
> On 07/02, Chao Yu wrote:
>> On 2021/7/2 9:32, Jaegeuk Kim wrote:
>>> On 07/02, Chao Yu wrote:
>>>> On 2021/7/2 1:10, Jaegeuk Kim wrote:
>>>>> On 06/01, Chao Yu wrote:
>>>>>> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
>>>>>>
>>>>>> As [1] reported, if lower device doesn't support write barrier, in below
>>>>>> case:
>>>>>>
>>>>>> - write page #0; persist
>>>>>> - overwrite page #0
>>>>>> - fsync
>>>>>>     - write data page #0 OPU into device's cache
>>>>>>     - write inode page into device's cache
>>>>>>     - issue flush
>>>>>
>>>>> Well, we have preflush for node writes, so I don't think this is the case.
>>>>>
>>>>>     fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
>>>>
>>>> This is only used for atomic write case, right?
>>>>
>>>> I mean the common case which is called from f2fs_issue_flush() in
>>>> f2fs_do_sync_file().
>>>
>>> How about adding PREFLUSH when writing node blocks aligned to the above set?
>>
>> You mean implementation like v1 as below?
>>
>> https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/
> 
> Yea, I think so. :P

I prefer v2, we may have several schemes to improve performance with v2, e.g.
- use inplace IO to avoid newly added preflush
- use flush_merge option to avoid redundant preflush
- if lower device supports barrier IO, we can avoid newly added preflush

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> And please see do_checkpoint(), we call f2fs_flush_device_cache() and
>>>> commit_checkpoint() separately to keep persistence order of CP datas.
>>>>
>>>> See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
>>>> for details.
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> If SPO is triggered during flush command, inode page can be persisted
>>>>>> before data page #0, so that after recovery, inode page can be recovered
>>>>>> with new physical block address of data page #0, however there may
>>>>>> contains dummy data in new physical block address.
>>>>>>
>>>>>> Then what user will see is: after overwrite & fsync + SPO, old data in
>>>>>> file was corrupted, if any user do care about such case, we can suggest
>>>>>> user to use STRICT fsync mode, in this mode, we will force to trigger
>>>>>> preflush command to persist data in device cache in prior to node
>>>>>> writeback, it avoids potential data corruption during fsync().
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - fix this by adding additional preflush command rather than using
>>>>>> atomic write flow.
>>>>>>     fs/f2fs/file.c | 14 ++++++++++++++
>>>>>>     1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 7d5311d54f63..238ca2a733ac 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>>>     				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>>>>>>     			goto flush_out;
>>>>>>     		goto out;
>>>>>> +	} else {
>>>>>> +		/*
>>>>>> +		 * for OPU case, during fsync(), node can be persisted before
>>>>>> +		 * data when lower device doesn't support write barrier, result
>>>>>> +		 * in data corruption after SPO.
>>>>>> +		 * So for strict fsync mode, force to trigger preflush to keep
>>>>>> +		 * data/node write order to avoid potential data corruption.
>>>>>> +		 */
>>>>>> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
>>>>>> +								!atomic) {
>>>>>> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>>> +			if (ret)
>>>>>> +				goto out;
>>>>>> +		}
>>>>>>     	}
>>>>>>     go_write:
>>>>>>     	/*
>>>>>> -- 
>>>>>> 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Jaegeuk Kim]

On 07/13, Chao Yu wrote:
> On 2021/7/8 1:48, Jaegeuk Kim wrote:
> > On 07/02, Chao Yu wrote:
> > > On 2021/7/2 9:32, Jaegeuk Kim wrote:
> > > > On 07/02, Chao Yu wrote:
> > > > > On 2021/7/2 1:10, Jaegeuk Kim wrote:
> > > > > > On 06/01, Chao Yu wrote:
> > > > > > > [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> > > > > > > 
> > > > > > > As [1] reported, if lower device doesn't support write barrier, in below
> > > > > > > case:
> > > > > > > 
> > > > > > > - write page #0; persist
> > > > > > > - overwrite page #0
> > > > > > > - fsync
> > > > > > >     - write data page #0 OPU into device's cache
> > > > > > >     - write inode page into device's cache
> > > > > > >     - issue flush
> > > > > > 
> > > > > > Well, we have preflush for node writes, so I don't think this is the case.
> > > > > > 
> > > > > >     fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> > > > > 
> > > > > This is only used for atomic write case, right?
> > > > > 
> > > > > I mean the common case which is called from f2fs_issue_flush() in
> > > > > f2fs_do_sync_file().
> > > > 
> > > > How about adding PREFLUSH when writing node blocks aligned to the above set?
> > > 
> > > You mean implementation like v1 as below?
> > > 
> > > https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/
> > 
> > Yea, I think so. :P
> 
> I prefer v2, we may have several schemes to improve performance with v2, e.g.
> - use inplace IO to avoid newly added preflush
> - use flush_merge option to avoid redundant preflush
> - if lower device supports barrier IO, we can avoid newly added preflush

Doesn't v2 give one more flush than v1? Why do you want to take worse one and
try to improve back? Not clear the benefit on v2.

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > And please see do_checkpoint(), we call f2fs_flush_device_cache() and
> > > > > commit_checkpoint() separately to keep persistence order of CP datas.
> > > > > 
> > > > > See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
> > > > > for details.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > If SPO is triggered during flush command, inode page can be persisted
> > > > > > > before data page #0, so that after recovery, inode page can be recovered
> > > > > > > with new physical block address of data page #0, however there may
> > > > > > > contains dummy data in new physical block address.
> > > > > > > 
> > > > > > > Then what user will see is: after overwrite & fsync + SPO, old data in
> > > > > > > file was corrupted, if any user do care about such case, we can suggest
> > > > > > > user to use STRICT fsync mode, in this mode, we will force to trigger
> > > > > > > preflush command to persist data in device cache in prior to node
> > > > > > > writeback, it avoids potential data corruption during fsync().
> > > > > > > 
> > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > > ---
> > > > > > > v2:
> > > > > > > - fix this by adding additional preflush command rather than using
> > > > > > > atomic write flow.
> > > > > > >     fs/f2fs/file.c | 14 ++++++++++++++
> > > > > > >     1 file changed, 14 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > > > > index 7d5311d54f63..238ca2a733ac 100644
> > > > > > > --- a/fs/f2fs/file.c
> > > > > > > +++ b/fs/f2fs/file.c
> > > > > > > @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
> > > > > > >     				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
> > > > > > >     			goto flush_out;
> > > > > > >     		goto out;
> > > > > > > +	} else {
> > > > > > > +		/*
> > > > > > > +		 * for OPU case, during fsync(), node can be persisted before
> > > > > > > +		 * data when lower device doesn't support write barrier, result
> > > > > > > +		 * in data corruption after SPO.
> > > > > > > +		 * So for strict fsync mode, force to trigger preflush to keep
> > > > > > > +		 * data/node write order to avoid potential data corruption.
> > > > > > > +		 */
> > > > > > > +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> > > > > > > +								!atomic) {
> > > > > > > +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> > > > > > > +			if (ret)
> > > > > > > +				goto out;
> > > > > > > +		}
> > > > > > >     	}
> > > > > > >     go_write:
> > > > > > >     	/*
> > > > > > > -- 
> > > > > > > 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

On 2021/7/14 7:34, Jaegeuk Kim wrote:
> On 07/13, Chao Yu wrote:
>> On 2021/7/8 1:48, Jaegeuk Kim wrote:
>>> On 07/02, Chao Yu wrote:
>>>> On 2021/7/2 9:32, Jaegeuk Kim wrote:
>>>>> On 07/02, Chao Yu wrote:
>>>>>> On 2021/7/2 1:10, Jaegeuk Kim wrote:
>>>>>>> On 06/01, Chao Yu wrote:
>>>>>>>> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
>>>>>>>>
>>>>>>>> As [1] reported, if lower device doesn't support write barrier, in below
>>>>>>>> case:
>>>>>>>>
>>>>>>>> - write page #0; persist
>>>>>>>> - overwrite page #0
>>>>>>>> - fsync
>>>>>>>>      - write data page #0 OPU into device's cache
>>>>>>>>      - write inode page into device's cache
>>>>>>>>      - issue flush
>>>>>>>
>>>>>>> Well, we have preflush for node writes, so I don't think this is the case.
>>>>>>>
>>>>>>>      fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
>>>>>>
>>>>>> This is only used for atomic write case, right?
>>>>>>
>>>>>> I mean the common case which is called from f2fs_issue_flush() in
>>>>>> f2fs_do_sync_file().
>>>>>
>>>>> How about adding PREFLUSH when writing node blocks aligned to the above set?
>>>>
>>>> You mean implementation like v1 as below?
>>>>
>>>> https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/
>>>
>>> Yea, I think so. :P
>>
>> I prefer v2, we may have several schemes to improve performance with v2, e.g.
>> - use inplace IO to avoid newly added preflush
>> - use flush_merge option to avoid redundant preflush
>> - if lower device supports barrier IO, we can avoid newly added preflush
> 
> Doesn't v2 give one more flush than v1? Why do you want to take worse one and

FUA implies an extra preflush command or similar mechanism in lower device to keep data
in bio being persistent before this command's completion.

Also if lower device doesn't support FUA natively, block layer turns it into an empty
PREFLUSH command.

So, it's hard to say which one will win the benchmark game, maybe we need some
performance data before making the choice, but you know, it depends on device's
character.

> try to improve back? Not clear the benefit on v2.

Well, if user suffer and complain performance regression with v1, any plan to improve it?

I just thought about plan B/C/D for no matter v1 or v2.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> And please see do_checkpoint(), we call f2fs_flush_device_cache() and
>>>>>> commit_checkpoint() separately to keep persistence order of CP datas.
>>>>>>
>>>>>> See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
>>>>>> for details.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> If SPO is triggered during flush command, inode page can be persisted
>>>>>>>> before data page #0, so that after recovery, inode page can be recovered
>>>>>>>> with new physical block address of data page #0, however there may
>>>>>>>> contains dummy data in new physical block address.
>>>>>>>>
>>>>>>>> Then what user will see is: after overwrite & fsync + SPO, old data in
>>>>>>>> file was corrupted, if any user do care about such case, we can suggest
>>>>>>>> user to use STRICT fsync mode, in this mode, we will force to trigger
>>>>>>>> preflush command to persist data in device cache in prior to node
>>>>>>>> writeback, it avoids potential data corruption during fsync().
>>>>>>>>
>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>> - fix this by adding additional preflush command rather than using
>>>>>>>> atomic write flow.
>>>>>>>>      fs/f2fs/file.c | 14 ++++++++++++++
>>>>>>>>      1 file changed, 14 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>>> index 7d5311d54f63..238ca2a733ac 100644
>>>>>>>> --- a/fs/f2fs/file.c
>>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>>> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>>>>>      				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>>>>>>>>      			goto flush_out;
>>>>>>>>      		goto out;
>>>>>>>> +	} else {
>>>>>>>> +		/*
>>>>>>>> +		 * for OPU case, during fsync(), node can be persisted before
>>>>>>>> +		 * data when lower device doesn't support write barrier, result
>>>>>>>> +		 * in data corruption after SPO.
>>>>>>>> +		 * So for strict fsync mode, force to trigger preflush to keep
>>>>>>>> +		 * data/node write order to avoid potential data corruption.
>>>>>>>> +		 */
>>>>>>>> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
>>>>>>>> +								!atomic) {
>>>>>>>> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>>>>> +			if (ret)
>>>>>>>> +				goto out;
>>>>>>>> +		}
>>>>>>>>      	}
>>>>>>>>      go_write:
>>>>>>>>      	/*
>>>>>>>> -- 
>>>>>>>> 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Jaegeuk Kim]

On 07/14, Chao Yu wrote:
> On 2021/7/14 7:34, Jaegeuk Kim wrote:
> > On 07/13, Chao Yu wrote:
> > > On 2021/7/8 1:48, Jaegeuk Kim wrote:
> > > > On 07/02, Chao Yu wrote:
> > > > > On 2021/7/2 9:32, Jaegeuk Kim wrote:
> > > > > > On 07/02, Chao Yu wrote:
> > > > > > > On 2021/7/2 1:10, Jaegeuk Kim wrote:
> > > > > > > > On 06/01, Chao Yu wrote:
> > > > > > > > > [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> > > > > > > > > 
> > > > > > > > > As [1] reported, if lower device doesn't support write barrier, in below
> > > > > > > > > case:
> > > > > > > > > 
> > > > > > > > > - write page #0; persist
> > > > > > > > > - overwrite page #0
> > > > > > > > > - fsync
> > > > > > > > >      - write data page #0 OPU into device's cache
> > > > > > > > >      - write inode page into device's cache
> > > > > > > > >      - issue flush
> > > > > > > > 
> > > > > > > > Well, we have preflush for node writes, so I don't think this is the case.
> > > > > > > > 
> > > > > > > >      fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> > > > > > > 
> > > > > > > This is only used for atomic write case, right?
> > > > > > > 
> > > > > > > I mean the common case which is called from f2fs_issue_flush() in
> > > > > > > f2fs_do_sync_file().
> > > > > > 
> > > > > > How about adding PREFLUSH when writing node blocks aligned to the above set?
> > > > > 
> > > > > You mean implementation like v1 as below?
> > > > > 
> > > > > https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/
> > > > 
> > > > Yea, I think so. :P
> > > 
> > > I prefer v2, we may have several schemes to improve performance with v2, e.g.
> > > - use inplace IO to avoid newly added preflush
> > > - use flush_merge option to avoid redundant preflush
> > > - if lower device supports barrier IO, we can avoid newly added preflush
> > 
> > Doesn't v2 give one more flush than v1? Why do you want to take worse one and
> 
> FUA implies an extra preflush command or similar mechanism in lower device to keep data
> in bio being persistent before this command's completion.
> 
> Also if lower device doesn't support FUA natively, block layer turns it into an empty
> PREFLUSH command.
> 
> So, it's hard to say which one will win the benchmark game, maybe we need some
> performance data before making the choice, but you know, it depends on device's
> character.

I was looking at # of bios.

> 
> > try to improve back? Not clear the benefit on v2.
> 
> Well, if user suffer and complain performance regression with v1, any plan to improve it?
> 
> I just thought about plan B/C/D for no matter v1 or v2.

I assumed you wanted v2 since it might be used for B/C/D improvements. But, it
seems it wasn't. My point is to save one bio, but piggyback the flag to the
device driver.

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > And please see do_checkpoint(), we call f2fs_flush_device_cache() and
> > > > > > > commit_checkpoint() separately to keep persistence order of CP datas.
> > > > > > > 
> > > > > > > See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
> > > > > > > for details.
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > If SPO is triggered during flush command, inode page can be persisted
> > > > > > > > > before data page #0, so that after recovery, inode page can be recovered
> > > > > > > > > with new physical block address of data page #0, however there may
> > > > > > > > > contains dummy data in new physical block address.
> > > > > > > > > 
> > > > > > > > > Then what user will see is: after overwrite & fsync + SPO, old data in
> > > > > > > > > file was corrupted, if any user do care about such case, we can suggest
> > > > > > > > > user to use STRICT fsync mode, in this mode, we will force to trigger
> > > > > > > > > preflush command to persist data in device cache in prior to node
> > > > > > > > > writeback, it avoids potential data corruption during fsync().
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > > > > ---
> > > > > > > > > v2:
> > > > > > > > > - fix this by adding additional preflush command rather than using
> > > > > > > > > atomic write flow.
> > > > > > > > >      fs/f2fs/file.c | 14 ++++++++++++++
> > > > > > > > >      1 file changed, 14 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > > > > > > index 7d5311d54f63..238ca2a733ac 100644
> > > > > > > > > --- a/fs/f2fs/file.c
> > > > > > > > > +++ b/fs/f2fs/file.c
> > > > > > > > > @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
> > > > > > > > >      				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
> > > > > > > > >      			goto flush_out;
> > > > > > > > >      		goto out;
> > > > > > > > > +	} else {
> > > > > > > > > +		/*
> > > > > > > > > +		 * for OPU case, during fsync(), node can be persisted before
> > > > > > > > > +		 * data when lower device doesn't support write barrier, result
> > > > > > > > > +		 * in data corruption after SPO.
> > > > > > > > > +		 * So for strict fsync mode, force to trigger preflush to keep
> > > > > > > > > +		 * data/node write order to avoid potential data corruption.
> > > > > > > > > +		 */
> > > > > > > > > +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> > > > > > > > > +								!atomic) {
> > > > > > > > > +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> > > > > > > > > +			if (ret)
> > > > > > > > > +				goto out;
> > > > > > > > > +		}
> > > > > > > > >      	}
> > > > > > > > >      go_write:
> > > > > > > > >      	/*
> > > > > > > > > -- 
> > > > > > > > > 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Chao Yu]

On 2021/7/14 10:19, Jaegeuk Kim wrote:
> On 07/14, Chao Yu wrote:
>> On 2021/7/14 7:34, Jaegeuk Kim wrote:
>>> On 07/13, Chao Yu wrote:
>>>> On 2021/7/8 1:48, Jaegeuk Kim wrote:
>>>>> On 07/02, Chao Yu wrote:
>>>>>> On 2021/7/2 9:32, Jaegeuk Kim wrote:
>>>>>>> On 07/02, Chao Yu wrote:
>>>>>>>> On 2021/7/2 1:10, Jaegeuk Kim wrote:
>>>>>>>>> On 06/01, Chao Yu wrote:
>>>>>>>>>> [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
>>>>>>>>>>
>>>>>>>>>> As [1] reported, if lower device doesn't support write barrier, in below
>>>>>>>>>> case:
>>>>>>>>>>
>>>>>>>>>> - write page #0; persist
>>>>>>>>>> - overwrite page #0
>>>>>>>>>> - fsync
>>>>>>>>>>       - write data page #0 OPU into device's cache
>>>>>>>>>>       - write inode page into device's cache
>>>>>>>>>>       - issue flush
>>>>>>>>>
>>>>>>>>> Well, we have preflush for node writes, so I don't think this is the case.
>>>>>>>>>
>>>>>>>>>       fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
>>>>>>>>
>>>>>>>> This is only used for atomic write case, right?
>>>>>>>>
>>>>>>>> I mean the common case which is called from f2fs_issue_flush() in
>>>>>>>> f2fs_do_sync_file().
>>>>>>>
>>>>>>> How about adding PREFLUSH when writing node blocks aligned to the above set?
>>>>>>
>>>>>> You mean implementation like v1 as below?
>>>>>>
>>>>>> https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/
>>>>>
>>>>> Yea, I think so. :P
>>>>
>>>> I prefer v2, we may have several schemes to improve performance with v2, e.g.
>>>> - use inplace IO to avoid newly added preflush
>>>> - use flush_merge option to avoid redundant preflush
>>>> - if lower device supports barrier IO, we can avoid newly added preflush
>>>
>>> Doesn't v2 give one more flush than v1? Why do you want to take worse one and
>>
>> FUA implies an extra preflush command or similar mechanism in lower device to keep data
>> in bio being persistent before this command's completion.
>>
>> Also if lower device doesn't support FUA natively, block layer turns it into an empty
>> PREFLUSH command.
>>
>> So, it's hard to say which one will win the benchmark game, maybe we need some
>> performance data before making the choice, but you know, it depends on device's
>> character.
> 
> I was looking at # of bios.
> 
>>
>>> try to improve back? Not clear the benefit on v2.
>>
>> Well, if user suffer and complain performance regression with v1, any plan to improve it?
>>
>> I just thought about plan B/C/D for no matter v1 or v2.
> 
> I assumed you wanted v2 since it might be used for B/C/D improvements. But, it
> seems it wasn't. My point is to save one bio, but piggyback the flag to the
> device driver.

I doubt the conclusion...but it needs to get some data to prove it.

I think the right way is merging v1 now to fix the bug firstly, and let me do
the comparison on them a little bit later to see whether we need another
implementation... thoughts?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> And please see do_checkpoint(), we call f2fs_flush_device_cache() and
>>>>>>>> commit_checkpoint() separately to keep persistence order of CP datas.
>>>>>>>>
>>>>>>>> See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
>>>>>>>> for details.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If SPO is triggered during flush command, inode page can be persisted
>>>>>>>>>> before data page #0, so that after recovery, inode page can be recovered
>>>>>>>>>> with new physical block address of data page #0, however there may
>>>>>>>>>> contains dummy data in new physical block address.
>>>>>>>>>>
>>>>>>>>>> Then what user will see is: after overwrite & fsync + SPO, old data in
>>>>>>>>>> file was corrupted, if any user do care about such case, we can suggest
>>>>>>>>>> user to use STRICT fsync mode, in this mode, we will force to trigger
>>>>>>>>>> preflush command to persist data in device cache in prior to node
>>>>>>>>>> writeback, it avoids potential data corruption during fsync().
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>> v2:
>>>>>>>>>> - fix this by adding additional preflush command rather than using
>>>>>>>>>> atomic write flow.
>>>>>>>>>>       fs/f2fs/file.c | 14 ++++++++++++++
>>>>>>>>>>       1 file changed, 14 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>>>>> index 7d5311d54f63..238ca2a733ac 100644
>>>>>>>>>> --- a/fs/f2fs/file.c
>>>>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>>>>> @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>>>>>>>       				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
>>>>>>>>>>       			goto flush_out;
>>>>>>>>>>       		goto out;
>>>>>>>>>> +	} else {
>>>>>>>>>> +		/*
>>>>>>>>>> +		 * for OPU case, during fsync(), node can be persisted before
>>>>>>>>>> +		 * data when lower device doesn't support write barrier, result
>>>>>>>>>> +		 * in data corruption after SPO.
>>>>>>>>>> +		 * So for strict fsync mode, force to trigger preflush to keep
>>>>>>>>>> +		 * data/node write order to avoid potential data corruption.
>>>>>>>>>> +		 */
>>>>>>>>>> +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
>>>>>>>>>> +								!atomic) {
>>>>>>>>>> +			ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>>>>>>> +			if (ret)
>>>>>>>>>> +				goto out;
>>>>>>>>>> +		}
>>>>>>>>>>       	}
>>>>>>>>>>       go_write:
>>>>>>>>>>       	/*
>>>>>>>>>> -- 
>>>>>>>>>> 2.29.2


_______________________________________________
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 RFC] f2fs: fix to force keeping write barrier for strict fsync mode

[Jaegeuk Kim]

On 07/14, Chao Yu wrote:
> On 2021/7/14 10:19, Jaegeuk Kim wrote:
> > On 07/14, Chao Yu wrote:
> > > On 2021/7/14 7:34, Jaegeuk Kim wrote:
> > > > On 07/13, Chao Yu wrote:
> > > > > On 2021/7/8 1:48, Jaegeuk Kim wrote:
> > > > > > On 07/02, Chao Yu wrote:
> > > > > > > On 2021/7/2 9:32, Jaegeuk Kim wrote:
> > > > > > > > On 07/02, Chao Yu wrote:
> > > > > > > > > On 2021/7/2 1:10, Jaegeuk Kim wrote:
> > > > > > > > > > On 06/01, Chao Yu wrote:
> > > > > > > > > > > [1] https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg15126.html
> > > > > > > > > > > 
> > > > > > > > > > > As [1] reported, if lower device doesn't support write barrier, in below
> > > > > > > > > > > case:
> > > > > > > > > > > 
> > > > > > > > > > > - write page #0; persist
> > > > > > > > > > > - overwrite page #0
> > > > > > > > > > > - fsync
> > > > > > > > > > >       - write data page #0 OPU into device's cache
> > > > > > > > > > >       - write inode page into device's cache
> > > > > > > > > > >       - issue flush
> > > > > > > > > > 
> > > > > > > > > > Well, we have preflush for node writes, so I don't think this is the case.
> > > > > > > > > > 
> > > > > > > > > >       fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
> > > > > > > > > 
> > > > > > > > > This is only used for atomic write case, right?
> > > > > > > > > 
> > > > > > > > > I mean the common case which is called from f2fs_issue_flush() in
> > > > > > > > > f2fs_do_sync_file().
> > > > > > > > 
> > > > > > > > How about adding PREFLUSH when writing node blocks aligned to the above set?
> > > > > > > 
> > > > > > > You mean implementation like v1 as below?
> > > > > > > 
> > > > > > > https://lore.kernel.org/linux-f2fs-devel/20200120100045.70210-1-yuchao0@huawei.com/
> > > > > > 
> > > > > > Yea, I think so. :P
> > > > > 
> > > > > I prefer v2, we may have several schemes to improve performance with v2, e.g.
> > > > > - use inplace IO to avoid newly added preflush
> > > > > - use flush_merge option to avoid redundant preflush
> > > > > - if lower device supports barrier IO, we can avoid newly added preflush
> > > > 
> > > > Doesn't v2 give one more flush than v1? Why do you want to take worse one and
> > > 
> > > FUA implies an extra preflush command or similar mechanism in lower device to keep data
> > > in bio being persistent before this command's completion.
> > > 
> > > Also if lower device doesn't support FUA natively, block layer turns it into an empty
> > > PREFLUSH command.
> > > 
> > > So, it's hard to say which one will win the benchmark game, maybe we need some
> > > performance data before making the choice, but you know, it depends on device's
> > > character.
> > 
> > I was looking at # of bios.
> > 
> > > 
> > > > try to improve back? Not clear the benefit on v2.
> > > 
> > > Well, if user suffer and complain performance regression with v1, any plan to improve it?
> > > 
> > > I just thought about plan B/C/D for no matter v1 or v2.
> > 
> > I assumed you wanted v2 since it might be used for B/C/D improvements. But, it
> > seems it wasn't. My point is to save one bio, but piggyback the flag to the
> > device driver.
> 
> I doubt the conclusion...but it needs to get some data to prove it.
> 
> I think the right way is merging v1 now to fix the bug firstly, and let me do
> the comparison on them a little bit later to see whether we need another
> implementation... thoughts?

Chao, could you please post v1 with an updated description?

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > And please see do_checkpoint(), we call f2fs_flush_device_cache() and
> > > > > > > > > commit_checkpoint() separately to keep persistence order of CP datas.
> > > > > > > > > 
> > > > > > > > > See commit 46706d5917f4 ("f2fs: flush cp pack except cp pack 2 page at first")
> > > > > > > > > for details.
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > If SPO is triggered during flush command, inode page can be persisted
> > > > > > > > > > > before data page #0, so that after recovery, inode page can be recovered
> > > > > > > > > > > with new physical block address of data page #0, however there may
> > > > > > > > > > > contains dummy data in new physical block address.
> > > > > > > > > > > 
> > > > > > > > > > > Then what user will see is: after overwrite & fsync + SPO, old data in
> > > > > > > > > > > file was corrupted, if any user do care about such case, we can suggest
> > > > > > > > > > > user to use STRICT fsync mode, in this mode, we will force to trigger
> > > > > > > > > > > preflush command to persist data in device cache in prior to node
> > > > > > > > > > > writeback, it avoids potential data corruption during fsync().
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > > > > > > > ---
> > > > > > > > > > > v2:
> > > > > > > > > > > - fix this by adding additional preflush command rather than using
> > > > > > > > > > > atomic write flow.
> > > > > > > > > > >       fs/f2fs/file.c | 14 ++++++++++++++
> > > > > > > > > > >       1 file changed, 14 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > > > > > > > > index 7d5311d54f63..238ca2a733ac 100644
> > > > > > > > > > > --- a/fs/f2fs/file.c
> > > > > > > > > > > +++ b/fs/f2fs/file.c
> > > > > > > > > > > @@ -301,6 +301,20 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
> > > > > > > > > > >       				f2fs_exist_written_data(sbi, ino, UPDATE_INO))
> > > > > > > > > > >       			goto flush_out;
> > > > > > > > > > >       		goto out;
> > > > > > > > > > > +	} else {
> > > > > > > > > > > +		/*
> > > > > > > > > > > +		 * for OPU case, during fsync(), node can be persisted before
> > > > > > > > > > > +		 * data when lower device doesn't support write barrier, result
> > > > > > > > > > > +		 * in data corruption after SPO.
> > > > > > > > > > > +		 * So for strict fsync mode, force to trigger preflush to keep
> > > > > > > > > > > +		 * data/node write order to avoid potential data corruption.
> > > > > > > > > > > +		 */
> > > > > > > > > > > +		if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT &&
> > > > > > > > > > > +								!atomic) {
> > > > > > > > > > > +			ret = f2fs_issue_flush(sbi, inode->i_ino);
> > > > > > > > > > > +			if (ret)
> > > > > > > > > > > +				goto out;
> > > > > > > > > > > +		}
> > > > > > > > > > >       	}
> > > > > > > > > > >       go_write:
> > > > > > > > > > >       	/*
> > > > > > > > > > > -- 
> > > > > > > > > > > 2.29.2


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel