All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ext4: Remove expensive flush on fast commit
       [not found] <CGME20210106013242epcms2p5b6b4ed8ca86f29456fdf56aa580e74b4@epcms2p5>
@ 2021-01-06  1:32 ` Daejun Park
  2021-01-06 21:58   ` harshad shirwadkar
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Daejun Park @ 2021-01-06  1:32 UTC (permalink / raw)
  To: tytso, adilger.kernel, harshadshirwadkar; +Cc: linux-ext4, linux-kernel

In the fast commit, it adds REQ_FUA and REQ_PREFLUSH on each fast commit
block when barrier is enabled. However, in recovery phase, ext4 compares
CRC value in the tail. So it is sufficient adds REQ_FUA and REQ_PREFLUSH
on the block that has tail.

Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 fs/ext4/fast_commit.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 4fcc21c25e79..e66507be334c 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -604,13 +604,13 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star
 	trace_ext4_fc_track_range(inode, start, end, ret);
 }
 
-static void ext4_fc_submit_bh(struct super_block *sb)
+static void ext4_fc_submit_bh(struct super_block *sb, bool is_tail)
 {
 	int write_flags = REQ_SYNC;
 	struct buffer_head *bh = EXT4_SB(sb)->s_fc_bh;
 
-	/* TODO: REQ_FUA | REQ_PREFLUSH is unnecessarily expensive. */
-	if (test_opt(sb, BARRIER))
+	/* Add REQ_FUA | REQ_PREFLUSH only its tail */
+	if (test_opt(sb, BARRIER) && is_tail)
 		write_flags |= REQ_FUA | REQ_PREFLUSH;
 	lock_buffer(bh);
 	set_buffer_dirty(bh);
@@ -684,7 +684,7 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)
 		*crc = ext4_chksum(sbi, *crc, tl, sizeof(*tl));
 	if (pad_len > 0)
 		ext4_fc_memzero(sb, tl + 1, pad_len, crc);
-	ext4_fc_submit_bh(sb);
+	ext4_fc_submit_bh(sb, false);
 
 	ret = jbd2_fc_get_buf(EXT4_SB(sb)->s_journal, &bh);
 	if (ret)
@@ -741,7 +741,7 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc)
 	tail.fc_crc = cpu_to_le32(crc);
 	ext4_fc_memcpy(sb, dst, &tail.fc_crc, sizeof(tail.fc_crc), NULL);
 
-	ext4_fc_submit_bh(sb);
+	ext4_fc_submit_bh(sb, true);
 
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove expensive flush on fast commit
  2021-01-06  1:32 ` [PATCH] ext4: Remove expensive flush on fast commit Daejun Park
@ 2021-01-06 21:58   ` harshad shirwadkar
  2021-01-08  2:26     ` Andreas Dilger
       [not found]     ` <CGME20210106013242epcms2p5b6b4ed8ca86f29456fdf56aa580e74b4@epcms2p3>
  2021-01-06 22:05   ` Darrick J. Wong
  2021-01-14  3:44   ` Theodore Ts'o
  2 siblings, 2 replies; 6+ messages in thread
From: harshad shirwadkar @ 2021-01-06 21:58 UTC (permalink / raw)
  To: daejun7.park; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel

Thanks Daejun! This looks good.

Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

On Tue, Jan 5, 2021 at 5:32 PM Daejun Park <daejun7.park@samsung.com> wrote:
>
> In the fast commit, it adds REQ_FUA and REQ_PREFLUSH on each fast commit
> block when barrier is enabled. However, in recovery phase, ext4 compares
> CRC value in the tail. So it is sufficient adds REQ_FUA and REQ_PREFLUSH
> on the block that has tail.
>
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
>  fs/ext4/fast_commit.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 4fcc21c25e79..e66507be334c 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -604,13 +604,13 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star
>         trace_ext4_fc_track_range(inode, start, end, ret);
>  }
>
> -static void ext4_fc_submit_bh(struct super_block *sb)
> +static void ext4_fc_submit_bh(struct super_block *sb, bool is_tail)
>  {
>         int write_flags = REQ_SYNC;
>         struct buffer_head *bh = EXT4_SB(sb)->s_fc_bh;
>
> -       /* TODO: REQ_FUA | REQ_PREFLUSH is unnecessarily expensive. */
> -       if (test_opt(sb, BARRIER))
> +       /* Add REQ_FUA | REQ_PREFLUSH only its tail */
> +       if (test_opt(sb, BARRIER) && is_tail)
>                 write_flags |= REQ_FUA | REQ_PREFLUSH;
>         lock_buffer(bh);
>         set_buffer_dirty(bh);
> @@ -684,7 +684,7 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)
>                 *crc = ext4_chksum(sbi, *crc, tl, sizeof(*tl));
>         if (pad_len > 0)
>                 ext4_fc_memzero(sb, tl + 1, pad_len, crc);
> -       ext4_fc_submit_bh(sb);
> +       ext4_fc_submit_bh(sb, false);
>
>         ret = jbd2_fc_get_buf(EXT4_SB(sb)->s_journal, &bh);
>         if (ret)
> @@ -741,7 +741,7 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc)
>         tail.fc_crc = cpu_to_le32(crc);
>         ext4_fc_memcpy(sb, dst, &tail.fc_crc, sizeof(tail.fc_crc), NULL);
>
> -       ext4_fc_submit_bh(sb);
> +       ext4_fc_submit_bh(sb, true);
>
>         return 0;
>  }
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove expensive flush on fast commit
  2021-01-06  1:32 ` [PATCH] ext4: Remove expensive flush on fast commit Daejun Park
  2021-01-06 21:58   ` harshad shirwadkar
@ 2021-01-06 22:05   ` Darrick J. Wong
  2021-01-14  3:44   ` Theodore Ts'o
  2 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2021-01-06 22:05 UTC (permalink / raw)
  To: Daejun Park
  Cc: tytso, adilger.kernel, harshadshirwadkar, linux-ext4, linux-kernel

On Wed, Jan 06, 2021 at 10:32:42AM +0900, Daejun Park wrote:
> In the fast commit, it adds REQ_FUA and REQ_PREFLUSH on each fast commit
> block when barrier is enabled. However, in recovery phase, ext4 compares
> CRC value in the tail. So it is sufficient adds REQ_FUA and REQ_PREFLUSH
> on the block that has tail.

Just out of curiosity, how does the new fastcommit code handle tests
like generic/475 that simulate sudden disk failure?

--D

> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
>  fs/ext4/fast_commit.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 4fcc21c25e79..e66507be334c 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -604,13 +604,13 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star
>  	trace_ext4_fc_track_range(inode, start, end, ret);
>  }
>  
> -static void ext4_fc_submit_bh(struct super_block *sb)
> +static void ext4_fc_submit_bh(struct super_block *sb, bool is_tail)
>  {
>  	int write_flags = REQ_SYNC;
>  	struct buffer_head *bh = EXT4_SB(sb)->s_fc_bh;
>  
> -	/* TODO: REQ_FUA | REQ_PREFLUSH is unnecessarily expensive. */
> -	if (test_opt(sb, BARRIER))
> +	/* Add REQ_FUA | REQ_PREFLUSH only its tail */
> +	if (test_opt(sb, BARRIER) && is_tail)
>  		write_flags |= REQ_FUA | REQ_PREFLUSH;
>  	lock_buffer(bh);
>  	set_buffer_dirty(bh);
> @@ -684,7 +684,7 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)
>  		*crc = ext4_chksum(sbi, *crc, tl, sizeof(*tl));
>  	if (pad_len > 0)
>  		ext4_fc_memzero(sb, tl + 1, pad_len, crc);
> -	ext4_fc_submit_bh(sb);
> +	ext4_fc_submit_bh(sb, false);
>  
>  	ret = jbd2_fc_get_buf(EXT4_SB(sb)->s_journal, &bh);
>  	if (ret)
> @@ -741,7 +741,7 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc)
>  	tail.fc_crc = cpu_to_le32(crc);
>  	ext4_fc_memcpy(sb, dst, &tail.fc_crc, sizeof(tail.fc_crc), NULL);
>  
> -	ext4_fc_submit_bh(sb);
> +	ext4_fc_submit_bh(sb, true);
>  
>  	return 0;
>  }
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove expensive flush on fast commit
  2021-01-06 21:58   ` harshad shirwadkar
@ 2021-01-08  2:26     ` Andreas Dilger
       [not found]     ` <CGME20210106013242epcms2p5b6b4ed8ca86f29456fdf56aa580e74b4@epcms2p3>
  1 sibling, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2021-01-08  2:26 UTC (permalink / raw)
  To: daejun7.park, harshad shirwadkar; +Cc: tytso, linux-ext4, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

On Tue, Jan 5, 2021 at 5:32 PM Daejun Park <daejun7.park@samsung.com> wrote:
> 
> In the fast commit, it adds REQ_FUA and REQ_PREFLUSH on each fast commit
> block when barrier is enabled. However, in recovery phase, ext4 compares
> CRC value in the tail. So it is sufficient adds REQ_FUA and REQ_PREFLUSH
> on the block that has tail.

Does the tail block *always* contain a CRC, or is that dependent on
EXT4_FEATURE_RO_COMPAT_METADATA_CSUM, JBD2_FEATURE_INCOMPAT_CSUM_V2,
or JBD2_FEATURE_INCOMPAT_CSUM_V3 being enabled?

If one of those features is *required* before the FAST_COMMIT feature
can be used, then this patch looks OK.  Otherwise, the CSUM feature
should be checked before the FUA is skipped for non-tail blocks.

Cheers, Andreas

> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
> fs/ext4/fast_commit.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 4fcc21c25e79..e66507be334c 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -604,13 +604,13 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star
>        trace_ext4_fc_track_range(inode, start, end, ret);
> }
> 
> -static void ext4_fc_submit_bh(struct super_block *sb)
> +static void ext4_fc_submit_bh(struct super_block *sb, bool is_tail)
> {
>        int write_flags = REQ_SYNC;
>        struct buffer_head *bh = EXT4_SB(sb)->s_fc_bh;
> 
> -       /* TODO: REQ_FUA | REQ_PREFLUSH is unnecessarily expensive. */
> -       if (test_opt(sb, BARRIER))
> +       /* Add REQ_FUA | REQ_PREFLUSH only its tail */
> +       if (test_opt(sb, BARRIER) && is_tail)
>                write_flags |= REQ_FUA | REQ_PREFLUSH;
>        lock_buffer(bh);
>        set_buffer_dirty(bh);
> @@ -684,7 +684,7 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)
>                *crc = ext4_chksum(sbi, *crc, tl, sizeof(*tl));
>        if (pad_len > 0)
>                ext4_fc_memzero(sb, tl + 1, pad_len, crc);
> -       ext4_fc_submit_bh(sb);
> +       ext4_fc_submit_bh(sb, false);
> 
>        ret = jbd2_fc_get_buf(EXT4_SB(sb)->s_journal, &bh);
>        if (ret)
> @@ -741,7 +741,7 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc)
>        tail.fc_crc = cpu_to_le32(crc);
>        ext4_fc_memcpy(sb, dst, &tail.fc_crc, sizeof(tail.fc_crc), NULL);
> 
> -       ext4_fc_submit_bh(sb);
> +       ext4_fc_submit_bh(sb, true);
> 
>        return 0;
> }
> --
> 2.25.1


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: Re: [PATCH] ext4: Remove expensive flush on fast commit
       [not found]     ` <CGME20210106013242epcms2p5b6b4ed8ca86f29456fdf56aa580e74b4@epcms2p3>
@ 2021-01-08  2:37       ` Daejun Park
  0 siblings, 0 replies; 6+ messages in thread
From: Daejun Park @ 2021-01-08  2:37 UTC (permalink / raw)
  To: Andreas Dilger, Daejun Park, harshad shirwadkar
  Cc: tytso, linux-ext4, linux-kernel

> > In the fast commit, it adds REQ_FUA and REQ_PREFLUSH on each fast commit
> > block when barrier is enabled. However, in recovery phase, ext4 compares
> > CRC value in the tail. So it is sufficient adds REQ_FUA and REQ_PREFLUSH
> > on the block that has tail.
> 
> Does the tail block *always* contain a CRC, or is that dependent on
> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM, JBD2_FEATURE_INCOMPAT_CSUM_V2,
> or JBD2_FEATURE_INCOMPAT_CSUM_V3 being enabled?

In the fast commit, the tail block always contain a CRC.
 
> If one of those features is *required* before the FAST_COMMIT feature
> can be used, then this patch looks OK.  Otherwise, the CSUM feature
> should be checked before the FUA is skipped for non-tail blocks.

So, I think it is OK without checking other CSUM feature.

Thanks,
Daejun

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove expensive flush on fast commit
  2021-01-06  1:32 ` [PATCH] ext4: Remove expensive flush on fast commit Daejun Park
  2021-01-06 21:58   ` harshad shirwadkar
  2021-01-06 22:05   ` Darrick J. Wong
@ 2021-01-14  3:44   ` Theodore Ts'o
  2 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2021-01-14  3:44 UTC (permalink / raw)
  To: Daejun Park; +Cc: adilger.kernel, harshadshirwadkar, linux-ext4, linux-kernel

On Wed, Jan 06, 2021 at 10:32:42AM +0900, Daejun Park wrote:
> In the fast commit, it adds REQ_FUA and REQ_PREFLUSH on each fast commit
> block when barrier is enabled. However, in recovery phase, ext4 compares
> CRC value in the tail. So it is sufficient adds REQ_FUA and REQ_PREFLUSH
> on the block that has tail.
> 
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-14  3:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210106013242epcms2p5b6b4ed8ca86f29456fdf56aa580e74b4@epcms2p5>
2021-01-06  1:32 ` [PATCH] ext4: Remove expensive flush on fast commit Daejun Park
2021-01-06 21:58   ` harshad shirwadkar
2021-01-08  2:26     ` Andreas Dilger
     [not found]     ` <CGME20210106013242epcms2p5b6b4ed8ca86f29456fdf56aa580e74b4@epcms2p3>
2021-01-08  2:37       ` Daejun Park
2021-01-06 22:05   ` Darrick J. Wong
2021-01-14  3:44   ` Theodore Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.