| Message ID | a7dd26219b1dba932d48b373fa8189a2bb6989bc.1737092798.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: export per-inode stable writes flag | expand |
On 17.01.25 06:47, Qu Wenruo wrote: > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index f809c3200c21..bd71e57d2978 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -962,6 +962,13 @@ static int btrfs_fill_super(struct super_block *sb, > sb->s_xattr = btrfs_xattr_handlers; > sb->s_time_gran = 1; > sb->s_iflags |= SB_I_CGROUPWB; > + /* > + * By default data csum is enabled, thus a folio should not be modified > + * until writeback is finished. Or it will cause csum mismatch. > + * For NODATACSUM inodes, the stable writes feature will be disabled > + * on a per-inode basis. > + */ > + sb->s_iflags |= SB_I_STABLE_WRITES; > > err = super_setup_bdi(sb); > if (err) { Just a quick question, when we set SB_I_STABLE_WRITES per default, don't we need to clear it in case of mount -onodatasum?
在 2025/1/17 17:23, Johannes Thumshirn 写道: > On 17.01.25 06:47, Qu Wenruo wrote: >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index f809c3200c21..bd71e57d2978 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -962,6 +962,13 @@ static int btrfs_fill_super(struct super_block *sb, >> sb->s_xattr = btrfs_xattr_handlers; >> sb->s_time_gran = 1; >> sb->s_iflags |= SB_I_CGROUPWB; >> + /* >> + * By default data csum is enabled, thus a folio should not be modified >> + * until writeback is finished. Or it will cause csum mismatch. >> + * For NODATACSUM inodes, the stable writes feature will be disabled >> + * on a per-inode basis. >> + */ >> + sb->s_iflags |= SB_I_STABLE_WRITES; >> >> err = super_setup_bdi(sb); >> if (err) { > > Just a quick question, when we set SB_I_STABLE_WRITES per default, don't > we need to clear it in case of mount -onodatasum? Thanks for point this out. The truth is, we do not need to set the sb flags at all. Since btrfs_update_address_space_flags() will be called for every inode read from disk, newly created one or changed by ioctl, we have covered all cases and no need to set the super flags. I'll remove the sb one in this case. Thanks, Qu
On 17.01.25 07:59, Qu Wenruo wrote: > > > 在 2025/1/17 17:23, Johannes Thumshirn 写道: >> On 17.01.25 06:47, Qu Wenruo wrote: >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>> index f809c3200c21..bd71e57d2978 100644 >>> --- a/fs/btrfs/super.c >>> +++ b/fs/btrfs/super.c >>> @@ -962,6 +962,13 @@ static int btrfs_fill_super(struct super_block *sb, >>> sb->s_xattr = btrfs_xattr_handlers; >>> sb->s_time_gran = 1; >>> sb->s_iflags |= SB_I_CGROUPWB; >>> + /* >>> + * By default data csum is enabled, thus a folio should not be modified >>> + * until writeback is finished. Or it will cause csum mismatch. >>> + * For NODATACSUM inodes, the stable writes feature will be disabled >>> + * on a per-inode basis. >>> + */ >>> + sb->s_iflags |= SB_I_STABLE_WRITES; >>> >>> err = super_setup_bdi(sb); >>> if (err) { >> >> Just a quick question, when we set SB_I_STABLE_WRITES per default, don't >> we need to clear it in case of mount -onodatasum? > > Thanks for point this out. > > The truth is, we do not need to set the sb flags at all. > Since btrfs_update_address_space_flags() will be called for every inode > read from disk, newly created one or changed by ioctl, we have covered > all cases and no need to set the super flags. > > I'll remove the sb one in this case. Ha, indeed! Didn't think of it that way :)
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index b2fa33911c28..b090a435675a 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_address_space_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..c4769c438859 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_address_space_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_address_space_flags(BTRFS_I(inode)); } ret = btrfs_insert_inode_locked(inode); diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 69c0444369b7..c70e4d2d9b27 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -325,9 +325,11 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, * Otherwise we want the flag to reflect the real COW * status of the file and will not set it. */ - if (inode->i_size == 0) + if (inode->i_size == 0) { binode_flags |= BTRFS_INODE_NODATACOW | BTRFS_INODE_NODATASUM; + btrfs_update_address_space_flags(binode); + } } else { binode_flags |= BTRFS_INODE_NODATACOW; } @@ -336,9 +338,11 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, * Revert back under same assumptions as above */ if (S_ISREG(inode->i_mode)) { - if (inode->i_size == 0) + if (inode->i_size == 0) { binode_flags &= ~(BTRFS_INODE_NODATACOW | BTRFS_INODE_NODATASUM); + btrfs_update_address_space_flags(binode); + } } else { binode_flags &= ~BTRFS_INODE_NODATACOW; } diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index f809c3200c21..bd71e57d2978 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -962,6 +962,13 @@ static int btrfs_fill_super(struct super_block *sb, sb->s_xattr = btrfs_xattr_handlers; sb->s_time_gran = 1; sb->s_iflags |= SB_I_CGROUPWB; + /* + * By default data csum is enabled, thus a folio should not be modified + * until writeback is finished. Or it will cause csum mismatch. + * For NODATACSUM inodes, the stable writes feature will be disabled + * on a per-inode basis. + */ + sb->s_iflags |= SB_I_STABLE_WRITES; err = super_setup_bdi(sb); if (err) {
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 happneing. 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 export the stable writes flag to a per-inode basis. This involves: - Explicitly set SB_I_STABLE_WRITES for btrfs super blocks This mean all inode address spaces will have AS_STABLE_WRITES by default. - 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> --- fs/btrfs/btrfs_inode.h | 8 ++++++++ fs/btrfs/file.c | 1 - fs/btrfs/inode.c | 2 ++ fs/btrfs/ioctl.c | 8 ++++++-- fs/btrfs/super.c | 7 +++++++ 5 files changed, 23 insertions(+), 3 deletions(-)