From: Shida Zhang <zhangshida@kylinos.cn>
Using __block_write_begin() make it inconvenient to journal the
user data dirty process. We can't tell the block layer maintainer,
‘Hey, we want to trace the dirty user data in ext4, can we add some
special code for ext4 in __block_write_begin?’:P
So use ext4_block_write_begin() instead.
The two functions are basically doing the same thing except for the
fscrypt related code. Narrow the scope of CONFIG_FS_ENCRYPTION so as
to allow ext4_block_write_begin() to function like __block_write_begin
when the config is disabled.
And hoist the ext4_block_write_begin so that it can be used in other
files.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
---
fs/ext4/ext4.h | 2 ++
fs/ext4/inline.c | 10 +++++-----
fs/ext4/inode.c | 23 ++++++-----------------
3 files changed, 13 insertions(+), 22 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 08acd152261e..5f8257b68190 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3851,6 +3851,8 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
return buffer_uptodate(bh);
}
+extern int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
+ get_block_t *get_block);
#endif /* __KERNEL__ */
#define EFSBADCRC EBADMSG /* Bad CRC detected */
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index e7a09a99837b..0a1a8431e281 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -601,10 +601,10 @@ static int ext4_convert_inline_data_to_extent(struct address_space *mapping,
goto out;
if (ext4_should_dioread_nolock(inode)) {
- ret = __block_write_begin(&folio->page, from, to,
- ext4_get_block_unwritten);
+ ret = ext4_block_write_begin(folio, from, to,
+ ext4_get_block_unwritten);
} else
- ret = __block_write_begin(&folio->page, from, to, ext4_get_block);
+ ret = ext4_block_write_begin(folio, from, to, ext4_get_block);
if (!ret && ext4_should_journal_data(inode)) {
ret = ext4_walk_page_buffers(handle, inode,
@@ -856,8 +856,8 @@ static int ext4_da_convert_inline_data_to_extent(struct address_space *mapping,
goto out;
}
- ret = __block_write_begin(&folio->page, 0, inline_size,
- ext4_da_get_block_prep);
+ ret = ext4_block_write_begin(folio, 0, inline_size,
+ ext4_da_get_block_prep);
if (ret) {
up_read(&EXT4_I(inode)->xattr_sem);
folio_unlock(folio);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 941c1c0d5c6e..6b15805ca88b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1041,9 +1041,8 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
return ret;
}
-#ifdef CONFIG_FS_ENCRYPTION
-static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
- get_block_t *get_block)
+int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
+ get_block_t *get_block)
{
unsigned from = pos & (PAGE_SIZE - 1);
unsigned to = from + len;
@@ -1119,7 +1118,9 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
}
if (unlikely(err)) {
folio_zero_new_buffers(folio, from, to);
- } else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
+ }
+#ifdef CONFIG_FS_ENCRYPTION
+ else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
for (i = 0; i < nr_wait; i++) {
int err2;
@@ -1131,10 +1132,10 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
}
}
}
+#endif
return err;
}
-#endif
/*
* To preserve ordering, it is essential that the hole instantiation and
@@ -1216,19 +1217,11 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
/* In case writeback began while the folio was unlocked */
folio_wait_stable(folio);
-#ifdef CONFIG_FS_ENCRYPTION
if (ext4_should_dioread_nolock(inode))
ret = ext4_block_write_begin(folio, pos, len,
ext4_get_block_unwritten);
else
ret = ext4_block_write_begin(folio, pos, len, ext4_get_block);
-#else
- if (ext4_should_dioread_nolock(inode))
- ret = __block_write_begin(&folio->page, pos, len,
- ext4_get_block_unwritten);
- else
- ret = __block_write_begin(&folio->page, pos, len, ext4_get_block);
-#endif
if (!ret && ext4_should_journal_data(inode)) {
ret = ext4_walk_page_buffers(handle, inode,
folio_buffers(folio), from, to,
@@ -2961,11 +2954,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
if (IS_ERR(folio))
return PTR_ERR(folio);
-#ifdef CONFIG_FS_ENCRYPTION
ret = ext4_block_write_begin(folio, pos, len, ext4_da_get_block_prep);
-#else
- ret = __block_write_begin(&folio->page, pos, len, ext4_da_get_block_prep);
-#endif
if (ret < 0) {
folio_unlock(folio);
folio_put(folio);
--
2.33.0
On Fri, Aug 23, 2024 at 09:33:28AM +0800, zhangshida wrote:
> From: Shida Zhang <zhangshida@kylinos.cn>
>
> Using __block_write_begin() make it inconvenient to journal the
> user data dirty process. We can't tell the block layer maintainer,
> ‘Hey, we want to trace the dirty user data in ext4, can we add some
> special code for ext4 in __block_write_begin?’:P
>
> So use ext4_block_write_begin() instead.
>
> The two functions are basically doing the same thing except for the
> fscrypt related code. Narrow the scope of CONFIG_FS_ENCRYPTION so as
> to allow ext4_block_write_begin() to function like __block_write_begin
> when the config is disabled.
> And hoist the ext4_block_write_begin so that it can be used in other
> files.
>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
> fs/ext4/ext4.h | 2 ++
> fs/ext4/inline.c | 10 +++++-----
> fs/ext4/inode.c | 23 ++++++-----------------
> 3 files changed, 13 insertions(+), 22 deletions(-)
Thanks for cleaning this up.
There are still some comments in fs/ext4/inode.c that reference
__block_write_begin. Can you update them too?
One more thing below.
> @@ -1119,7 +1118,9 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> }
> if (unlikely(err)) {
> folio_zero_new_buffers(folio, from, to);
> - } else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> + }
> +#ifdef CONFIG_FS_ENCRYPTION
> + else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> for (i = 0; i < nr_wait; i++) {
> int err2;
>
> @@ -1131,10 +1132,10 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> }
> }
> }
> +#endif
This #ifdef isn't necessary since fscrypt_inode_uses_fs_layer_crypto() returns
false (and it's known at compile time) when !CONFIG_FS_ENCRYPTION.
- Eric
Eric Biggers <ebiggers@kernel.org> 于2024年8月29日周四 08:03写道：
>
> On Fri, Aug 23, 2024 at 09:33:28AM +0800, zhangshida wrote:
> > From: Shida Zhang <zhangshida@kylinos.cn>
> >
> > Using __block_write_begin() make it inconvenient to journal the
> > user data dirty process. We can't tell the block layer maintainer,
> > ‘Hey, we want to trace the dirty user data in ext4, can we add some
> > special code for ext4 in __block_write_begin?’:P
> >
> > So use ext4_block_write_begin() instead.
> >
> > The two functions are basically doing the same thing except for the
> > fscrypt related code. Narrow the scope of CONFIG_FS_ENCRYPTION so as
> > to allow ext4_block_write_begin() to function like __block_write_begin
> > when the config is disabled.
> > And hoist the ext4_block_write_begin so that it can be used in other
> > files.
> >
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> > ---
> > fs/ext4/ext4.h | 2 ++
> > fs/ext4/inline.c | 10 +++++-----
> > fs/ext4/inode.c | 23 ++++++-----------------
> > 3 files changed, 13 insertions(+), 22 deletions(-)
>
> Thanks for cleaning this up.
>
> There are still some comments in fs/ext4/inode.c that reference
> __block_write_begin. Can you update them too?
>
> One more thing below.
>
> > @@ -1119,7 +1118,9 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> > }
> > if (unlikely(err)) {
> > folio_zero_new_buffers(folio, from, to);
> > - } else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> > + }
> > +#ifdef CONFIG_FS_ENCRYPTION
> > + else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> > for (i = 0; i < nr_wait; i++) {
> > int err2;
> >
> > @@ -1131,10 +1132,10 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> > }
> > }
> > }
> > +#endif
>
> This #ifdef isn't necessary since fscrypt_inode_uses_fs_layer_crypto() returns
> false (and it's known at compile time) when !CONFIG_FS_ENCRYPTION.
>
Okay. Will make another version.
Thanks,
Stephen
> - Eric
On Fri 23-08-24 09:33:28, zhangshida wrote:
> From: Shida Zhang <zhangshida@kylinos.cn>
>
> Using __block_write_begin() make it inconvenient to journal the
> user data dirty process. We can't tell the block layer maintainer,
> ‘Hey, we want to trace the dirty user data in ext4, can we add some
> special code for ext4 in __block_write_begin?’:P
>
> So use ext4_block_write_begin() instead.
>
> The two functions are basically doing the same thing except for the
> fscrypt related code. Narrow the scope of CONFIG_FS_ENCRYPTION so as
> to allow ext4_block_write_begin() to function like __block_write_begin
> when the config is disabled.
> And hoist the ext4_block_write_begin so that it can be used in other
> files.
>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
Looks good to me! Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/ext4.h | 2 ++
> fs/ext4/inline.c | 10 +++++-----
> fs/ext4/inode.c | 23 ++++++-----------------
> 3 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 08acd152261e..5f8257b68190 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3851,6 +3851,8 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
> return buffer_uptodate(bh);
> }
>
> +extern int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> + get_block_t *get_block);
> #endif /* __KERNEL__ */
>
> #define EFSBADCRC EBADMSG /* Bad CRC detected */
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index e7a09a99837b..0a1a8431e281 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -601,10 +601,10 @@ static int ext4_convert_inline_data_to_extent(struct address_space *mapping,
> goto out;
>
> if (ext4_should_dioread_nolock(inode)) {
> - ret = __block_write_begin(&folio->page, from, to,
> - ext4_get_block_unwritten);
> + ret = ext4_block_write_begin(folio, from, to,
> + ext4_get_block_unwritten);
> } else
> - ret = __block_write_begin(&folio->page, from, to, ext4_get_block);
> + ret = ext4_block_write_begin(folio, from, to, ext4_get_block);
>
> if (!ret && ext4_should_journal_data(inode)) {
> ret = ext4_walk_page_buffers(handle, inode,
> @@ -856,8 +856,8 @@ static int ext4_da_convert_inline_data_to_extent(struct address_space *mapping,
> goto out;
> }
>
> - ret = __block_write_begin(&folio->page, 0, inline_size,
> - ext4_da_get_block_prep);
> + ret = ext4_block_write_begin(folio, 0, inline_size,
> + ext4_da_get_block_prep);
> if (ret) {
> up_read(&EXT4_I(inode)->xattr_sem);
> folio_unlock(folio);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 941c1c0d5c6e..6b15805ca88b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1041,9 +1041,8 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
> return ret;
> }
>
> -#ifdef CONFIG_FS_ENCRYPTION
> -static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> - get_block_t *get_block)
> +int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> + get_block_t *get_block)
> {
> unsigned from = pos & (PAGE_SIZE - 1);
> unsigned to = from + len;
> @@ -1119,7 +1118,9 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> }
> if (unlikely(err)) {
> folio_zero_new_buffers(folio, from, to);
> - } else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> + }
> +#ifdef CONFIG_FS_ENCRYPTION
> + else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> for (i = 0; i < nr_wait; i++) {
> int err2;
>
> @@ -1131,10 +1132,10 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> }
> }
> }
> +#endif
>
> return err;
> }
> -#endif
>
> /*
> * To preserve ordering, it is essential that the hole instantiation and
> @@ -1216,19 +1217,11 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
> /* In case writeback began while the folio was unlocked */
> folio_wait_stable(folio);
>
> -#ifdef CONFIG_FS_ENCRYPTION
> if (ext4_should_dioread_nolock(inode))
> ret = ext4_block_write_begin(folio, pos, len,
> ext4_get_block_unwritten);
> else
> ret = ext4_block_write_begin(folio, pos, len, ext4_get_block);
> -#else
> - if (ext4_should_dioread_nolock(inode))
> - ret = __block_write_begin(&folio->page, pos, len,
> - ext4_get_block_unwritten);
> - else
> - ret = __block_write_begin(&folio->page, pos, len, ext4_get_block);
> -#endif
> if (!ret && ext4_should_journal_data(inode)) {
> ret = ext4_walk_page_buffers(handle, inode,
> folio_buffers(folio), from, to,
> @@ -2961,11 +2954,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
> if (IS_ERR(folio))
> return PTR_ERR(folio);
>
> -#ifdef CONFIG_FS_ENCRYPTION
> ret = ext4_block_write_begin(folio, pos, len, ext4_da_get_block_prep);
> -#else
> - ret = __block_write_begin(&folio->page, pos, len, ext4_da_get_block_prep);
> -#endif
> if (ret < 0) {
> folio_unlock(folio);
> folio_put(folio);
> --
> 2.33.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
© 2016 - 2026 Red Hat, Inc.