| Message ID | 20240829085407.3331490-3-zhangshida@kylinos.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix an error caused by improperly dirtied buffer | expand |
On Thu 29-08-24 16:54:06, 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. Remove the unnecessary #ifdef since > fscrypt_inode_uses_fs_layer_crypto() returns false (and it's known at > compile time) when !CONFIG_FS_ENCRYPTION. > > And hoist the ext4_block_write_begin so that it can be used in other > files. > > Suggested-by: Jan Kara <jack@suse.cz> > Suggested-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Shida Zhang <zhangshida@kylinos.cn> I think I've given my Reviewed-by on this already in previous version - you can keep those tags unless the patch significantly changes. Anyway: 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 | 24 +++++------------------- > 3 files changed, 12 insertions(+), 24 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 a0a55cb8db53..4964c67e029e 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -1024,10 +1024,10 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode, > if (!buffer_mapped(bh) || buffer_freed(bh)) > return 0; > /* > - * __block_write_begin() could have dirtied some buffers. Clean > + * ext4_block_write_begin() could have dirtied some buffers. Clean > * the dirty bit as jbd2_journal_get_write_access() could complain > * otherwise about fs integrity issues. Setting of the dirty bit > - * by __block_write_begin() isn't a real problem here as we clear > + * by ext4_block_write_begin() isn't a real problem here as we clear > * the bit before releasing a page lock and thus writeback cannot > * ever write the buffer. > */ > @@ -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; > @@ -1134,7 +1133,6 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len, > > return err; > } > -#endif > > /* > * To preserve ordering, it is essential that the hole instantiation and > @@ -1216,19 +1214,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, > @@ -1241,7 +1231,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping, > > folio_unlock(folio); > /* > - * __block_write_begin may have instantiated a few blocks > + * ext4_block_write_begin may have instantiated a few blocks > * outside i_size. Trim these off again. Don't need > * i_size_read because we hold i_rwsem. > * > @@ -2961,11 +2951,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 Thu 29-08-24 11:12:50, Jan Kara wrote: > On Thu 29-08-24 16:54:06, 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. Remove the unnecessary #ifdef since > > fscrypt_inode_uses_fs_layer_crypto() returns false (and it's known at > > compile time) when !CONFIG_FS_ENCRYPTION. > > > > And hoist the ext4_block_write_begin so that it can be used in other > > files. > > > > Suggested-by: Jan Kara <jack@suse.cz> > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > Signed-off-by: Shida Zhang <zhangshida@kylinos.cn> > > I think I've given my Reviewed-by on this already in previous version - you > can keep those tags unless the patch significantly changes. Anyway: feel > free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> I've realized the patch slightly changed so that's likely why you've dropped the Reviewed-by so I retract my comment :) I've also realized one thing: > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -1024,10 +1024,10 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode, > > if (!buffer_mapped(bh) || buffer_freed(bh)) > > return 0; > > /* > > - * __block_write_begin() could have dirtied some buffers. Clean > > + * ext4_block_write_begin() could have dirtied some buffers. Clean > > * the dirty bit as jbd2_journal_get_write_access() could complain > > * otherwise about fs integrity issues. Setting of the dirty bit > > - * by __block_write_begin() isn't a real problem here as we clear > > + * by ext4_block_write_begin() isn't a real problem here as we clear > > * the bit before releasing a page lock and thus writeback cannot > > * ever write the buffer. > > */ This comment and the special buffer_dirty() handling in this function can be removed after patch 3 of this series. Nice additional cleanup :). Honza
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 a0a55cb8db53..4964c67e029e 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1024,10 +1024,10 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode, if (!buffer_mapped(bh) || buffer_freed(bh)) return 0; /* - * __block_write_begin() could have dirtied some buffers. Clean + * ext4_block_write_begin() could have dirtied some buffers. Clean * the dirty bit as jbd2_journal_get_write_access() could complain * otherwise about fs integrity issues. Setting of the dirty bit - * by __block_write_begin() isn't a real problem here as we clear + * by ext4_block_write_begin() isn't a real problem here as we clear * the bit before releasing a page lock and thus writeback cannot * ever write the buffer. */ @@ -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; @@ -1134,7 +1133,6 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len, return err; } -#endif /* * To preserve ordering, it is essential that the hole instantiation and @@ -1216,19 +1214,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, @@ -1241,7 +1231,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping, folio_unlock(folio); /* - * __block_write_begin may have instantiated a few blocks + * ext4_block_write_begin may have instantiated a few blocks * outside i_size. Trim these off again. Don't need * i_size_read because we hold i_rwsem. * @@ -2961,11 +2951,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);