| Message ID | d380f7c79d21ea2f5020d07da8518973b519afb4.1738019838.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] btrfs: expose per-inode stable writes flag | expand |
On Tue, Jan 28, 2025 at 09:48:18AM +1030, Qu Wenruo wrote: > The address space flag AS_STABLE_WRITES determine if FGP_STABLE for will > wait for the folio to finish its writeback. > > For btrfs, due to the default data checksum behavior, if we modify the > folio while it's still under writeback, it will cause data checksum > mismatch. > Thus for quite some call sites we manually call folio_wait_writeback() > to prevent such problem from happening. > > Currently there is only one call site inside btrfs really utilize > FGP_STABLE, and in that case we also manually call folio_wait_writeback() > to do the wait. > > But it's better to properly expose the stable writes flag to a per-inode > basis, to allow call sites to fully benefit from FGP_STABLE flag. > E.g. for inodes with NODATASUM allowing beginning dirtying the page > without waiting for writeback. > > This involves: > > - Update the mapping's stable write flag when setting/clearing NODATASUM > inode flag using ioctl > This only works for empty files, so it should be fine. > > - Update the mapping's stable write flag when reading an inode from disk > > - Remove the explicitly folio_wait_writeback() for FGP_BEGINWRITE call > site > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Changelog: > v3: > - Rename "export" to "expose", and slightly update the commit message > Currently the only usage of AS_STABLE_WRITES is to make > FGP_STABLE/FGP_BEGINWRITE to wait for writeback. > Make this point a little more explicit. > > - Rename the helper to btrfs_update_inode_mapping_flags() > > - Fix the call site inside btrfs_fileattr_set() > We should only call the helper after btrfs_inode::flags got updated. Thanks, this looks ok. As the inode flags and mapping are not something that is otherwise logically connected I was thinking about the safest way how to prevent accidental disconnect between the two in the future. So the idea is to place the mapping flags update as the last call where it's relevant. As implemented in v3 it looks like it is. Setting or clearing the mapping flags is just a bit set, a negligible overhead, so making it unconditionl is also an option in calls that change flags. This is only up to consideration if there's a better placement than now.
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index b2fa33911c28..029fba82b81d 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -516,6 +516,14 @@ static inline void btrfs_assert_inode_locked(struct btrfs_inode *inode) lockdep_assert_held(&inode->vfs_inode.i_rwsem); } +static inline void btrfs_update_inode_mapping_flags(struct btrfs_inode *inode) +{ + if (inode->flags & BTRFS_INODE_NODATASUM) + mapping_clear_stable_writes(inode->vfs_inode.i_mapping); + else + mapping_set_stable_writes(inode->vfs_inode.i_mapping); +} + /* Array of bytes with variable length, hexadecimal format 0x1234 */ #define CSUM_FMT "0x%*phN" #define CSUM_FMT_VALUE(size, bytes) size, bytes diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index bb821fb89fc1..68b14ee1f85c 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -875,7 +875,6 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_ ret = PTR_ERR(folio); return ret; } - folio_wait_writeback(folio); /* Only support page sized folio yet. */ ASSERT(folio_order(folio) == 0); ret = set_folio_extent_mapped(folio); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 5b3fdba10245..31d9777c443a 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3948,6 +3948,7 @@ static int btrfs_read_locked_inode(struct inode *inode, struct btrfs_path *path) btrfs_inode_split_flags(btrfs_inode_flags(leaf, inode_item), &BTRFS_I(inode)->flags, &BTRFS_I(inode)->ro_flags); + btrfs_update_inode_mapping_flags(BTRFS_I(inode)); cache_index: /* @@ -6363,6 +6364,7 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans, if (btrfs_test_opt(fs_info, NODATACOW)) BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW | BTRFS_INODE_NODATASUM; + btrfs_update_inode_mapping_flags(BTRFS_I(inode)); } ret = btrfs_insert_inode_locked(inode); diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 69c0444369b7..f5f66623551c 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -393,6 +393,7 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, update_flags: binode->flags = binode_flags; + btrfs_update_inode_mapping_flags(binode); btrfs_sync_inode_flags_to_i_flags(inode); inode_inc_iversion(inode); inode_set_ctime_current(inode);
The address space flag AS_STABLE_WRITES determine if FGP_STABLE for will wait for the folio to finish its writeback. For btrfs, due to the default data checksum behavior, if we modify the folio while it's still under writeback, it will cause data checksum mismatch. Thus for quite some call sites we manually call folio_wait_writeback() to prevent such problem from happening. Currently there is only one call site inside btrfs really utilize FGP_STABLE, and in that case we also manually call folio_wait_writeback() to do the wait. But it's better to properly expose the stable writes flag to a per-inode basis, to allow call sites to fully benefit from FGP_STABLE flag. E.g. for inodes with NODATASUM allowing beginning dirtying the page without waiting for writeback. This involves: - Update the mapping's stable write flag when setting/clearing NODATASUM inode flag using ioctl This only works for empty files, so it should be fine. - Update the mapping's stable write flag when reading an inode from disk - Remove the explicitly folio_wait_writeback() for FGP_BEGINWRITE call site Signed-off-by: Qu Wenruo <wqu@suse.com> --- Changelog: v3: - Rename "export" to "expose", and slightly update the commit message Currently the only usage of AS_STABLE_WRITES is to make FGP_STABLE/FGP_BEGINWRITE to wait for writeback. Make this point a little more explicit. - Rename the helper to btrfs_update_inode_mapping_flags() - Fix the call site inside btrfs_fileattr_set() We should only call the helper after btrfs_inode::flags got updated. v2: - Remove the superblock SB_I_STABLE_WRITES flag Since we're setting the proper flag for each inode, there is no need to bother the superblock flag anymore. --- fs/btrfs/btrfs_inode.h | 8 ++++++++ fs/btrfs/file.c | 1 - fs/btrfs/inode.c | 2 ++ fs/btrfs/ioctl.c | 1 + 4 files changed, 11 insertions(+), 1 deletion(-)