| Message ID | 20240618142112.1315279-1-yi.zhang@huaweicloud.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iomap/xfs: fix stale data exposure when truncating realtime inodes | expand |
On Tue, Jun 18, 2024 at 10:21:10PM +0800, Zhang Yi wrote: > Changes since v5: > - Drop all the code about zeroing out the whole allocation unitsize > on truncate down in xfs_setattr_size() as Christoph suggested, let's > just fix this issue for RT file by converting tail blocks to > unwritten now, and we could think about forced aligned extent and > atomic write later until it needs, so only pick patch 6 and 8 in > previous version, do some minor git log changes. This mostly makes sense, let's see how it fares with overnight fstests. For now, this is a provisional Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Changes since v4: > - Drop the first patch in v4 "iomap: zeroing needs to be pagecache > aware" since this series is not strongly depends on it, that patch > still needs furtuer analyse and also should add to handle the case of > a pending COW extent that extends over a data fork hole. This is a > big job, so let's fix the exposure stale data issue and brings back > the changes in iomap_write_end() first, don't block the ext4 buffered > iomap conversion. > - In patch 1, drop the 'ifndef rem_u64'. > - In patch 4, factor out a helper xfs_setattr_truncate_data() to handle > the zero out, update i_size, write back and drop pagecache on > truncate. > - In patch 5, switch to use xfs_inode_alloc_unitsize() in > xfs_itruncate_extents_flags(). > - In patch 6, changes to reserve blocks for rtextsize > 1 realtime > inodes on truncate down. > - In patch 7, drop the unwritten convert threshold, always convert tail > blocks to unwritten on truncate down realtime inodes. > - Add patch 8 to bring back 'commit 943bc0882ceb ("iomap: don't > increase i_size if it's not a write operation")'. > > Changes since v3: > - Factor out a new helper to get the remainder in math64.h as Darrick > suggested. > - Adjust the truncating order to prevent too much redundant blocking > writes as Dave suggested. > - Improve to convert the tail extent to unwritten when truncating down > an inode with large rtextsize as Darrick and Dave suggested. > > Since 'commit 943bc0882ceb ("iomap: don't increase i_size if it's not a > write operation")' merged, Chandan reported a stale data exposure issue > when running fstests generic/561 on xfs with realtime device [1]. This > issue has been fix in 6.10 by revert this commit through commit > '0841ea4a3b41 ("iomap: keep on increasing i_size in iomap_write_end()")', > but the real problem is xfs_setattr_size() doesn't zero out enough range > when truncate down a realtime inode. So this series fix this problem by > just converting the tail blocks to unwritten when truncate down realtime > inodes, then we could bring commit 943bc0882ceb back. > > I've tested this series on fstests (1) with reflink=0, (2) with > reflink=1, (3) with 28K RT device, no new failures detected, and it > passed generic/561 on RT device over 300+ rounds, please let me know if > we need any other test. > > [1] https://lore.kernel.org/linux-xfs/87ttj8ircu.fsf@debian-BULLSEYE-live-builder-AMD64/ > > Thanks, > Yi. > > Zhang Yi (2): > xfs: reserve blocks for truncating large realtime inode > iomap: don't increase i_size in iomap_write_end() > > fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++------------------- > fs/xfs/xfs_iops.c | 15 +++++++++++- > 2 files changed, 43 insertions(+), 25 deletions(-) > > -- > 2.39.2 > >
On 2024/6/19 8:24, Darrick J. Wong wrote: > On Tue, Jun 18, 2024 at 10:21:10PM +0800, Zhang Yi wrote: >> Changes since v5: >> - Drop all the code about zeroing out the whole allocation unitsize >> on truncate down in xfs_setattr_size() as Christoph suggested, let's >> just fix this issue for RT file by converting tail blocks to >> unwritten now, and we could think about forced aligned extent and >> atomic write later until it needs, so only pick patch 6 and 8 in >> previous version, do some minor git log changes. > > This mostly makes sense, let's see how it fares with overnight fstests. > For now, this is a provisional > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > Thanks for the review. BTW, what kind of fstests configs do you usually run? I always run kvm-xfstests -g auto[1] before I submitting ext4 patches, I'd also like to run full tests and hope to find regressions before submitting xfs patches. [1] https://github.com/tytso/xfstests-bld Thanks, Yi. > > >> Changes since v4: >> - Drop the first patch in v4 "iomap: zeroing needs to be pagecache >> aware" since this series is not strongly depends on it, that patch >> still needs furtuer analyse and also should add to handle the case of >> a pending COW extent that extends over a data fork hole. This is a >> big job, so let's fix the exposure stale data issue and brings back >> the changes in iomap_write_end() first, don't block the ext4 buffered >> iomap conversion. >> - In patch 1, drop the 'ifndef rem_u64'. >> - In patch 4, factor out a helper xfs_setattr_truncate_data() to handle >> the zero out, update i_size, write back and drop pagecache on >> truncate. >> - In patch 5, switch to use xfs_inode_alloc_unitsize() in >> xfs_itruncate_extents_flags(). >> - In patch 6, changes to reserve blocks for rtextsize > 1 realtime >> inodes on truncate down. >> - In patch 7, drop the unwritten convert threshold, always convert tail >> blocks to unwritten on truncate down realtime inodes. >> - Add patch 8 to bring back 'commit 943bc0882ceb ("iomap: don't >> increase i_size if it's not a write operation")'. >> >> Changes since v3: >> - Factor out a new helper to get the remainder in math64.h as Darrick >> suggested. >> - Adjust the truncating order to prevent too much redundant blocking >> writes as Dave suggested. >> - Improve to convert the tail extent to unwritten when truncating down >> an inode with large rtextsize as Darrick and Dave suggested. >> >> Since 'commit 943bc0882ceb ("iomap: don't increase i_size if it's not a >> write operation")' merged, Chandan reported a stale data exposure issue >> when running fstests generic/561 on xfs with realtime device [1]. This >> issue has been fix in 6.10 by revert this commit through commit >> '0841ea4a3b41 ("iomap: keep on increasing i_size in iomap_write_end()")', >> but the real problem is xfs_setattr_size() doesn't zero out enough range >> when truncate down a realtime inode. So this series fix this problem by >> just converting the tail blocks to unwritten when truncate down realtime >> inodes, then we could bring commit 943bc0882ceb back. >> >> I've tested this series on fstests (1) with reflink=0, (2) with >> reflink=1, (3) with 28K RT device, no new failures detected, and it >> passed generic/561 on RT device over 300+ rounds, please let me know if >> we need any other test. >> >> [1] https://lore.kernel.org/linux-xfs/87ttj8ircu.fsf@debian-BULLSEYE-live-builder-AMD64/ >> >> Thanks, >> Yi. >> >> Zhang Yi (2): >> xfs: reserve blocks for truncating large realtime inode >> iomap: don't increase i_size in iomap_write_end() >> >> fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++------------------- >> fs/xfs/xfs_iops.c | 15 +++++++++++- >> 2 files changed, 43 insertions(+), 25 deletions(-) >> >> -- >> 2.39.2 >> >>
On Tue, 18 Jun 2024 22:21:10 +0800, Zhang Yi wrote: > Changes since v5: > - Drop all the code about zeroing out the whole allocation unitsize > on truncate down in xfs_setattr_size() as Christoph suggested, let's > just fix this issue for RT file by converting tail blocks to > unwritten now, and we could think about forced aligned extent and > atomic write later until it needs, so only pick patch 6 and 8 in > previous version, do some minor git log changes. > > [...] I've put this into vfs.iomap for testing which should end up in fs-next asap. --- Applied to the vfs.iomap branch of the vfs/vfs.git tree. Patches in the vfs.iomap branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.iomap [1/2] xfs: reserve blocks for truncating large realtime inode https://git.kernel.org/vfs/vfs/c/d048945150b7 [2/2] iomap: don't increase i_size in iomap_write_end() https://git.kernel.org/vfs/vfs/c/602f09f4029c
Changes since v5: - Drop all the code about zeroing out the whole allocation unitsize on truncate down in xfs_setattr_size() as Christoph suggested, let's just fix this issue for RT file by converting tail blocks to unwritten now, and we could think about forced aligned extent and atomic write later until it needs, so only pick patch 6 and 8 in previous version, do some minor git log changes. Changes since v4: - Drop the first patch in v4 "iomap: zeroing needs to be pagecache aware" since this series is not strongly depends on it, that patch still needs furtuer analyse and also should add to handle the case of a pending COW extent that extends over a data fork hole. This is a big job, so let's fix the exposure stale data issue and brings back the changes in iomap_write_end() first, don't block the ext4 buffered iomap conversion. - In patch 1, drop the 'ifndef rem_u64'. - In patch 4, factor out a helper xfs_setattr_truncate_data() to handle the zero out, update i_size, write back and drop pagecache on truncate. - In patch 5, switch to use xfs_inode_alloc_unitsize() in xfs_itruncate_extents_flags(). - In patch 6, changes to reserve blocks for rtextsize > 1 realtime inodes on truncate down. - In patch 7, drop the unwritten convert threshold, always convert tail blocks to unwritten on truncate down realtime inodes. - Add patch 8 to bring back 'commit 943bc0882ceb ("iomap: don't increase i_size if it's not a write operation")'. Changes since v3: - Factor out a new helper to get the remainder in math64.h as Darrick suggested. - Adjust the truncating order to prevent too much redundant blocking writes as Dave suggested. - Improve to convert the tail extent to unwritten when truncating down an inode with large rtextsize as Darrick and Dave suggested. Since 'commit 943bc0882ceb ("iomap: don't increase i_size if it's not a write operation")' merged, Chandan reported a stale data exposure issue when running fstests generic/561 on xfs with realtime device [1]. This issue has been fix in 6.10 by revert this commit through commit '0841ea4a3b41 ("iomap: keep on increasing i_size in iomap_write_end()")', but the real problem is xfs_setattr_size() doesn't zero out enough range when truncate down a realtime inode. So this series fix this problem by just converting the tail blocks to unwritten when truncate down realtime inodes, then we could bring commit 943bc0882ceb back. I've tested this series on fstests (1) with reflink=0, (2) with reflink=1, (3) with 28K RT device, no new failures detected, and it passed generic/561 on RT device over 300+ rounds, please let me know if we need any other test. [1] https://lore.kernel.org/linux-xfs/87ttj8ircu.fsf@debian-BULLSEYE-live-builder-AMD64/ Thanks, Yi. Zhang Yi (2): xfs: reserve blocks for truncating large realtime inode iomap: don't increase i_size in iomap_write_end() fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++------------------- fs/xfs/xfs_iops.c | 15 +++++++++++- 2 files changed, 43 insertions(+), 25 deletions(-)