Revert "f2fs: use flush command instead of FUA for zoned device"

[PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Wenjie Cheng 1 year, 8 months ago

This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.

Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
command instead of FUA for zoned device") used additional flush
command to keep write order.

Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
Introduce zone write plugging") has enabled the block layer to
handle this order issue, there is no need to use flush command.

Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
---
 fs/f2fs/file.c | 3 +--
 fs/f2fs/node.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index eae2e7908072..f08e6208e183 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
 	clear_inode_flag(inode, FI_APPEND_WRITE);
 flush_out:
-	if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER) ||
-	    (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
+	if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
 		ret = f2fs_issue_flush(sbi, inode->i_ino);
 	if (!ret) {
 		f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 144f9f966690..c45d341dcf6e 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
 		goto redirty_out;
 	}
 
-	if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
+	if (atomic && !test_opt(sbi, NOBARRIER))
 		fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
 
 	/* should add to global list before clearing PAGECACHE status */
-- 
2.34.1

Re: [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Chao Yu 1 year, 6 months ago

On 2024/6/14 8:48, Wenjie Cheng wrote:
> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
> 
> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
> command instead of FUA for zoned device") used additional flush
> command to keep write order.
> 
> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
> Introduce zone write plugging") has enabled the block layer to
> handle this order issue, there is no need to use flush command.
> 
> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Chao Yu 1 year, 7 months ago

Jaegeuk,

Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
instead of FUA for zoned device")
"
The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
command to keep the write order.
"

It seems mq-deadline use fifo queue and make queue depth of zone device
as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
layer?

Thanks,

On 2024/6/14 8:48, Wenjie Cheng wrote:
> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
> 
> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
> command instead of FUA for zoned device") used additional flush
> command to keep write order.
> 
> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
> Introduce zone write plugging") has enabled the block layer to
> handle this order issue, there is no need to use flush command.
> 
> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
> ---
>   fs/f2fs/file.c | 3 +--
>   fs/f2fs/node.c | 2 +-
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index eae2e7908072..f08e6208e183 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>   	f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
>   	clear_inode_flag(inode, FI_APPEND_WRITE);
>   flush_out:
> -	if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER) ||
> -	    (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
> +	if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>   		ret = f2fs_issue_flush(sbi, inode->i_ino);
>   	if (!ret) {
>   		f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 144f9f966690..c45d341dcf6e 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>   		goto redirty_out;
>   	}
>   
> -	if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
> +	if (atomic && !test_opt(sbi, NOBARRIER))
>   		fio.op_flags |= REQ_PREFLUSH | REQ_FUA;
>   
>   	/* should add to global list before clearing PAGECACHE status */

RE:(2) [f2fs-dev] [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Daejun Park 1 year, 7 months ago

Hi Chao,

> Jaegeuk,
> 
> Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
> instead of FUA for zoned device")
> "
> The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
> command to keep the write order.
> "
> 
> It seems mq-deadline use fifo queue and make queue depth of zone device
> as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
> layer?

While other writes are aligned by the mq-deadline, write with FUA is not passed
to the scheduler but handled at the block layer.

Thanks,
Daejun

> 
> Thanks,
> 
> On 2024/6/14 8:48, Wenjie Cheng wrote:
> > This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
> >
> > Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
> > command instead of FUA for zoned device") used additional flush
> > command to keep write order.
> >
> > Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
> > Introduce zone write plugging") has enabled the block layer to
> > handle this order issue, there is no need to use flush command.
> >
> > Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
> > ---
> >  fs/f2fs/file.c 3 +--
> >  fs/f2fs/node.c 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index eae2e7908072..f08e6208e183 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
> >           f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
> >           clear_inode_flag(inode, FI_APPEND_WRITE);
> >  flush_out:
> > -        if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
> > -            (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
> > +        if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
> >                   ret = f2fs_issue_flush(sbi, inode->i_ino);
> >           if (!ret) {
> >                   f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 144f9f966690..c45d341dcf6e 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> >                   goto redirty_out;
> >           }
> > 
> > -        if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
> > +        if (atomic && !test_opt(sbi, NOBARRIER))
> >                   fio.op_flags = REQ_PREFLUSH REQ_FUA;
> > 
> >           /* should add to global list before clearing PAGECACHE status */

Re: (2) [f2fs-dev] [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Chao Yu 1 year, 7 months ago

On 2024/6/20 13:56, Daejun Park wrote:
> Hi Chao,
> 
>> Jaegeuk,
>>
>> Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
>> instead of FUA for zoned device")
>> "
>> The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
>> command to keep the write order.
>> "
>>
>> It seems mq-deadline use fifo queue and make queue depth of zone device
>> as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
>> layer?
> 
> While other writes are aligned by the mq-deadline, write with FUA is not passed
> to the scheduler but handled at the block layer.

Hi Daejun,

IIUC, do you mean write w/ FUA may be handled directly in below path?

- blk_mq_submit_bio
  - op_is_flush && blk_insert_flush

Thanks,

> 
> Thanks,
> Daejun
> 
>>
>> Thanks,
>>
>> On 2024/6/14 8:48, Wenjie Cheng wrote:
>>> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
>>>
>>> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
>>> command instead of FUA for zoned device") used additional flush
>>> command to keep write order.
>>>
>>> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
>>> Introduce zone write plugging") has enabled the block layer to
>>> handle this order issue, there is no need to use flush command.
>>>
>>> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
>>> ---
>>>    fs/f2fs/file.c 3 +--
>>>    fs/f2fs/node.c 2 +-
>>>    2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index eae2e7908072..f08e6208e183 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>             f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
>>>             clear_inode_flag(inode, FI_APPEND_WRITE);
>>>    flush_out:
>>> -        if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>> -            (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
>>> +        if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>                     ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>             if (!ret) {
>>>                     f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index 144f9f966690..c45d341dcf6e 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>>                     goto redirty_out;
>>>             }
>>>   
>>> -        if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
>>> +        if (atomic && !test_opt(sbi, NOBARRIER))
>>>                     fio.op_flags = REQ_PREFLUSH REQ_FUA;
>>>   
>>>             /* should add to global list before clearing PAGECACHE status */

RE:(2) (2) [f2fs-dev] [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Daejun Park 1 year, 7 months ago

>On 2024/6/20 13:56, Daejun Park wrote:
>> Hi Chao,
>>
>>> Jaegeuk,
>>>
>>> Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
>>> instead of FUA for zoned device")
>>> "
>>> The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
>>> command to keep the write order.
>>> "
>>>
>>> It seems mq-deadline use fifo queue and make queue depth of zone device
>>> as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
>>> layer?
>>
>> While other writes are aligned by the mq-deadline, write with FUA is not passed
>> to the scheduler but handled at the block layer.
>
>Hi Daejun,
>
>IIUC, do you mean write w/ FUA may be handled directly in below path?
>
>- blk_mq_submit_bio
>  - op_is_flush && blk_insert_flush

Hi Chao,

Yes, I think the path caused an unaligned write when the zone lock was
being applied by mq-deadline.

>
>Thanks,
>
>>
>> Thanks,
>> Daejun
>>
>>>
>>> Thanks,
>>>
>>> On 2024/6/14 8:48, Wenjie Cheng wrote:
>>>> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
>>>>
>>>> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
>>>> command instead of FUA for zoned device") used additional flush
>>>> command to keep write order.
>>>>
>>>> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
>>>> Introduce zone write plugging") has enabled the block layer to
>>>> handle this order issue, there is no need to use flush command.
>>>>
>>>> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
>>>> ---
>>>>    fs/f2fs/file.c 3 +--
>>>>    fs/f2fs/node.c 2 +-
>>>>    2 files changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index eae2e7908072..f08e6208e183 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>             f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
>>>>             clear_inode_flag(inode, FI_APPEND_WRITE);
>>>>    flush_out:
>>>> -        if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>> -            (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
>>>> +        if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>                     ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>             if (!ret) {
>>>>                     f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>> index 144f9f966690..c45d341dcf6e 100644
>>>> --- a/fs/f2fs/node.c
>>>> +++ b/fs/f2fs/node.c
>>>> @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>>>                     goto redirty_out;
>>>>             }
>>>> 
>>>> -        if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
>>>> +        if (atomic && !test_opt(sbi, NOBARRIER))
>>>>                     fio.op_flags = REQ_PREFLUSH REQ_FUA;
>>>> 
>>>>             /* should add to global list before clearing PAGECACHE status */

Re: (2) (2) [f2fs-dev] [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Chao Yu 1 year, 7 months ago

On 2024/6/20 15:22, Daejun Park wrote:
>> On 2024/6/20 13:56, Daejun Park wrote:
>>> Hi Chao,
>>>
>>>> Jaegeuk,
>>>>
>>>> Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
>>>> instead of FUA for zoned device")
>>>> "
>>>> The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
>>>> command to keep the write order.
>>>> "
>>>>
>>>> It seems mq-deadline use fifo queue and make queue depth of zone device
>>>> as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
>>>> layer?
>>>
>>> While other writes are aligned by the mq-deadline, write with FUA is not passed
>>> to the scheduler but handled at the block layer.
>>
>> Hi Daejun,
>>
>> IIUC, do you mean write w/ FUA may be handled directly in below path?
>>
>> - blk_mq_submit_bio
>>    - op_is_flush && blk_insert_flush
> 
> Hi Chao,
> 
> Yes, I think the path caused an unaligned write when the zone lock was
> being applied by mq-deadline.

But, blk_insert_flush() may return false due to policy should be
REQ_FSEQ_DATA or REQ_FSEQ_DATA | REQ_FSEQ_POSTFLUSH, then
blk_mq_insert_request() after blk_insert_flush() will be called?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>> Daejun
>>>
>>>>
>>>> Thanks,
>>>>
>>>> On 2024/6/14 8:48, Wenjie Cheng wrote:
>>>>> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
>>>>>
>>>>> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
>>>>> command instead of FUA for zoned device") used additional flush
>>>>> command to keep write order.
>>>>>
>>>>> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
>>>>> Introduce zone write plugging") has enabled the block layer to
>>>>> handle this order issue, there is no need to use flush command.
>>>>>
>>>>> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
>>>>> ---
>>>>>      fs/f2fs/file.c 3 +--
>>>>>      fs/f2fs/node.c 2 +-
>>>>>      2 files changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>> index eae2e7908072..f08e6208e183 100644
>>>>> --- a/fs/f2fs/file.c
>>>>> +++ b/fs/f2fs/file.c
>>>>> @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>>               f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
>>>>>               clear_inode_flag(inode, FI_APPEND_WRITE);
>>>>>      flush_out:
>>>>> -        if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>> -            (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
>>>>> +        if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>>                       ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>>               if (!ret) {
>>>>>                       f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>> index 144f9f966690..c45d341dcf6e 100644
>>>>> --- a/fs/f2fs/node.c
>>>>> +++ b/fs/f2fs/node.c
>>>>> @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>>>>                       goto redirty_out;
>>>>>               }
>>>>>   
>>>>> -        if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
>>>>> +        if (atomic && !test_opt(sbi, NOBARRIER))
>>>>>                       fio.op_flags = REQ_PREFLUSH REQ_FUA;
>>>>>   
>>>>>               /* should add to global list before clearing PAGECACHE status */

RE:(2) (2) (2) [f2fs-dev] [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Daejun Park 1 year, 7 months ago

>On 2024/6/20 15:22, Daejun Park wrote:
>>> On 2024/6/20 13:56, Daejun Park wrote:
>>>> Hi Chao,
>>>>
>>>>> Jaegeuk,
>>>>>
>>>>> Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
>>>>> instead of FUA for zoned device")
>>>>> "
>>>>> The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
>>>>> command to keep the write order.
>>>>> "
>>>>>
>>>>> It seems mq-deadline use fifo queue and make queue depth of zone device
>>>>> as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
>>>>> layer?
>>>>
>>>> While other writes are aligned by the mq-deadline, write with FUA is not passed
>>>> to the scheduler but handled at the block layer.
>>>
>>> Hi Daejun,
>>>
>>> IIUC, do you mean write w/ FUA may be handled directly in below path?
>>>
>>> - blk_mq_submit_bio
>>>    - op_is_flush && blk_insert_flush
>>
>> Hi Chao,
>>
>> Yes, I think the path caused an unaligned write when the zone lock was
>> being applied by mq-deadline.
>
>But, blk_insert_flush() may return false due to policy should be
>REQ_FSEQ_DATA or REQ_FSEQ_DATA REQ_FSEQ_POSTFLUSH, then
>blk_mq_insert_request() after blk_insert_flush() will be called?
>

I was just discussing the handling of FUAs in commit c550e25bca66,
which is not an issue in the current code as FUAs are handled correctly.

Thanks,


>Thanks,
>
>>
>>>
>>> Thanks,
>>>
>>>>
>>>> Thanks,
>>>> Daejun
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> On 2024/6/14 8:48, Wenjie Cheng wrote:
>>>>>> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
>>>>>>
>>>>>> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
>>>>>> command instead of FUA for zoned device") used additional flush
>>>>>> command to keep write order.
>>>>>>
>>>>>> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
>>>>>> Introduce zone write plugging") has enabled the block layer to
>>>>>> handle this order issue, there is no need to use flush command.
>>>>>>
>>>>>> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
>>>>>> ---
>>>>>>      fs/f2fs/file.c 3 +--
>>>>>>      fs/f2fs/node.c 2 +-
>>>>>>      2 files changed, 2 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index eae2e7908072..f08e6208e183 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>>>               f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
>>>>>>               clear_inode_flag(inode, FI_APPEND_WRITE);
>>>>>>      flush_out:
>>>>>> -        if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>>> -            (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
>>>>>> +        if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>>>                       ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>>>               if (!ret) {
>>>>>>                       f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>>> index 144f9f966690..c45d341dcf6e 100644
>>>>>> --- a/fs/f2fs/node.c
>>>>>> +++ b/fs/f2fs/node.c
>>>>>> @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>>>>>                       goto redirty_out;
>>>>>>               }
>>>>>> 
>>>>>> -        if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
>>>>>> +        if (atomic && !test_opt(sbi, NOBARRIER))
>>>>>>                       fio.op_flags = REQ_PREFLUSH REQ_FUA;
>>>>>> 
>>>>>>               /* should add to global list before clearing PAGECACHE status */

Re: (2) (2) (2) [f2fs-dev] [PATCH] Revert "f2fs: use flush command instead of FUA for zoned device"

Posted by Chao Yu 1 year, 7 months ago

On 2024/6/20 15:56, Daejun Park wrote:
>> On 2024/6/20 15:22, Daejun Park wrote:
>>>> On 2024/6/20 13:56, Daejun Park wrote:
>>>>> Hi Chao,
>>>>>
>>>>>> Jaegeuk,
>>>>>>
>>>>>> Quoted commit message from commit c550e25bca66 ("f2fs: use flush command
>>>>>> instead of FUA for zoned device")
>>>>>> "
>>>>>> The block layer for zoned disk can reorder the FUA'ed IOs. Let's use flush
>>>>>> command to keep the write order.
>>>>>> "
>>>>>>
>>>>>> It seems mq-deadline use fifo queue and make queue depth of zone device
>>>>>> as 1 to IO order, so why FUA'ed write node IOs can be reordered by block
>>>>>> layer?
>>>>>
>>>>> While other writes are aligned by the mq-deadline, write with FUA is not passed
>>>>> to the scheduler but handled at the block layer.
>>>>
>>>> Hi Daejun,
>>>>
>>>> IIUC, do you mean write w/ FUA may be handled directly in below path?
>>>>
>>>> - blk_mq_submit_bio
>>>>      - op_is_flush && blk_insert_flush
>>>
>>> Hi Chao,
>>>
>>> Yes, I think the path caused an unaligned write when the zone lock was
>>> being applied by mq-deadline.
>>
>> But, blk_insert_flush() may return false due to policy should be
>> REQ_FSEQ_DATA or REQ_FSEQ_DATA REQ_FSEQ_POSTFLUSH, then
>> blk_mq_insert_request() after blk_insert_flush() will be called?
>>
> 
> I was just discussing the handling of FUAs in commit c550e25bca66,
> which is not an issue in the current code as FUAs are handled correctly.

Yup, I think it needs to be reverted. :)

Thanks,

> 
> Thanks,
> 
> 
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>> Daejun
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> On 2024/6/14 8:48, Wenjie Cheng wrote:
>>>>>>> This reverts commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1.
>>>>>>>
>>>>>>> Commit c550e25bca660ed2554cbb48d32b82d0bb98e4b1 ("f2fs: use flush
>>>>>>> command instead of FUA for zoned device") used additional flush
>>>>>>> command to keep write order.
>>>>>>>
>>>>>>> Since Commit dd291d77cc90eb6a86e9860ba8e6e38eebd57d12 ("block:
>>>>>>> Introduce zone write plugging") has enabled the block layer to
>>>>>>> handle this order issue, there is no need to use flush command.
>>>>>>>
>>>>>>> Signed-off-by: Wenjie Cheng <cwjhust@gmail.com>
>>>>>>> ---
>>>>>>>        fs/f2fs/file.c 3 +--
>>>>>>>        fs/f2fs/node.c 2 +-
>>>>>>>        2 files changed, 2 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>> index eae2e7908072..f08e6208e183 100644
>>>>>>> --- a/fs/f2fs/file.c
>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>> @@ -372,8 +372,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>>>>>>                 f2fs_remove_ino_entry(sbi, ino, APPEND_INO);
>>>>>>>                 clear_inode_flag(inode, FI_APPEND_WRITE);
>>>>>>>        flush_out:
>>>>>>> -        if ((!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>>>> -            (atomic && !test_opt(sbi, NOBARRIER) && f2fs_sb_has_blkzoned(sbi)))
>>>>>>> +        if (!atomic && F2FS_OPTION(sbi).fsync_mode != FSYNC_MODE_NOBARRIER)
>>>>>>>                         ret = f2fs_issue_flush(sbi, inode->i_ino);
>>>>>>>                 if (!ret) {
>>>>>>>                         f2fs_remove_ino_entry(sbi, ino, UPDATE_INO);
>>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>>>>> index 144f9f966690..c45d341dcf6e 100644
>>>>>>> --- a/fs/f2fs/node.c
>>>>>>> +++ b/fs/f2fs/node.c
>>>>>>> @@ -1631,7 +1631,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>>>>>>                         goto redirty_out;
>>>>>>>                 }
>>>>>>>   
>>>>>>> -        if (atomic && !test_opt(sbi, NOBARRIER) && !f2fs_sb_has_blkzoned(sbi))
>>>>>>> +        if (atomic && !test_opt(sbi, NOBARRIER))
>>>>>>>                         fio.op_flags = REQ_PREFLUSH REQ_FUA;
>>>>>>>   
>>>>>>>                 /* should add to global list before clearing PAGECACHE status */