reflink and atomic write bugs
Subsystems (4)
v8 cover reflink and atomic write bugs
[ the send command got interrupted apologies for the two dups ]
This series did a few rounds privately, but looks ready for the list now. Carlos has accepted patches 1-4, but I've kept them here so the whole series can still be tested.
Except for Christoph's patch, these were all found and fixed by an LLM.
The meat of the series is fixing reflink corner cases that incorrectly let you write into the source of a reflink. LLMs also tripped over a few ways to use atomic writes to expose stale blocks on the drive, and a few races with realtime flag and extent size hint manipulations.
v7->v8:
Christoph pointed out that only page faults can cope with the -EAGAIN from "xfs: restart faults racing with realtime flag changes". pNFS is the one other caller that can really get there, so now faults get -EAGAIN and everyone else gets -EINVAL.
Sashiko had two complaints about the same patch. The stale reservation is fixed by the two "resample" patches. Those are the rest of the 3 additional patches I promised in v7, I just managed to leave them out of the posting.
The second complaint turned out to be on disk corruption for realtime filesystems with extents bigger than one block, with nothing returning an error. That's the new "xfs: test for an extsize hint again under the ILOCK".
While checking what a DAX fault does with the -EAGAIN, we found that DAX and zoned inodes don't work together at all. With a zoned filesystem on pmem and dax=always, the first store into a mapped file oopses on 7.4-rc2, and so does ftruncate(). "xfs: don't enable DAX on zoned inodes" turns DAX off for zoned inodes.
Added Christoph's review to "xfs: test for always COW after the dio write checks".
v6->v7:
Sashiko noticed a few pre-existing corners that I had ignored, so we have 3 additional patches dealing with races between FS_IOC_FSSETXATTR and decisions about file writes.
"xfs: restart faults racing with realtime flag changes" changed enough that I dropped Christoph's review.
Chris Mason (14):
xfs: don't skip the flush before shifting extents
xfs: flush the shifted range before changing the file
xfs: don't change the block map under a dirty folio
iomap: don't punch delalloc under a dirty EOF block
fs: validate atomic writes after O_APPEND and limits
xfs: recheck direct write alignment after write checks
xfs: test for reflink after the dio write checks
xfs: test for always COW after the dio write checks
xfs: refuse fallocate racing with realtime flag changes
xfs: restart faults racing with realtime flag changes
xfs: test for an extsize hint again under the ILOCK
xfs: resample the extsize hint after taking the ILOCK
xfs: resample the COW extsize hint after cycling ILOCK
xfs: don't enable DAX on zoned inodes
Christoph Hellwig (1):
xfs: refuse writes to files racing with realtime inode flag changes
fs/ext4/file.c | 4 - fs/iomap/buffered-io.c | 34 ++++++++- fs/read_write.c | 13 ++++ fs/xfs/xfs_bmap_util.c | 19 ++++- fs/xfs/xfs_file.c | 167 ++++++++++++++++++++++++++++++++--------- fs/xfs/xfs_ioctl.c | 8 +- fs/xfs/xfs_iomap.c | 73 ++++++++++++++++-- fs/xfs/xfs_iops.c | 4 + fs/xfs/xfs_reflink.c | 16 +++- 9 files changed, 282 insertions(+), 56 deletions(-)
PATCH v8 1/15 xfs: don't skip the flush before shifting extents
On an XFS file larger than 4 EiB, collapse range and insert range can drop dirty data without writing it back, and then fail with EFSCORRUPTED.
xfs_prepare_shift() flushes the page cache from its offset to EOF, but passes i_size as the length. Above 4 EiB the end of that range can overflow and turn negative. filemap_write_and_wait_range() then writes nothing, but truncate_pagecache_range() takes the end as unsigned and drops every folio from the start of the range onward.
A delalloc extent under a dropped folio stays in the data fork, and the shift returns -EFSCORRUPTED for it.
The fix used here is to pass i_size - offset as the length.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/xfs/xfs_bmap_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_bmap_util.c +1 -1
@@ -980,7 +980,7 @@ xfs_prepare_shift(* Writeback and invalidate cache for the remainder of the file as we're * about to shift down every extent from offset to EOF. */ - error =xfs_flush_unmap_range (ip, offset,XFS_ISIZE (ip)); + error =xfs_flush_unmap_range (ip, offset,XFS_ISIZE (ip) - offset);if (error)return error;
Signature · 1 line
2.55.0
PATCH v8 2/15 xfs: flush the shifted range before changing the file
In reply to Chris MasonIf writeback of the range to be shifted fails, collapse range on XFS can fail with its hole already punched, and insert range with the file already grown.
Both calls change the file first: collapse range punches its hole, and insert range grows i_size. Only then does xfs_prepare_shift() flush the range from the offset to EOF, and that flush fails if the writeback fails.
The fix used here is to flush and invalidate that range before the file is changed.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/xfs/xfs_file.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
fs/xfs/xfs_file.c +23 -2
@@ -1201,6 +1201,22 @@ xfs_falloc_setsize( &iattr); } +/* +* Collapse and insert range change the file in several steps, and each step +* writes back and empties the part of the page cache that it needs. That can +* fail. Do it here for everything that is going to move, before the first +* step, so that a failure leaves the file as it was. xfs_prepare_shift() +* starts one allocation unit before the offset, so do the same. +*/ +static int +xfs_falloc_flush_shift_range ( +struct xfs_inode *ip, +loff_t offset) +{ + offset -=min_t (loff_t, offset,xfs_inode_alloc_unitsize (ip)); +return xfs_flush_unmap_range (ip, offset,XFS_ISIZE (ip) - offset); +} + static int xfs_falloc_collapse_range( struct file *file,
@@ -1222,6 +1238,10 @@ xfs_falloc_collapse_range(if (offset + len >=i_size_read (inode))return -EINVAL ; + error =xfs_falloc_flush_shift_range (XFS_I (inode), offset); +if (error) +return error; + error =xfs_collapse_file_space (XFS_I (inode), offset, len, ac);if (error)return error;
@@ -1263,9 +1283,10 @@ xfs_falloc_insert_range(* fork. This shifts back out beyond EOF, but creates an expectedly * written post-eof block. The insert is going to flush, unmap and * cancel prealloc across this whole range, so flush EOF now before we -* bump i_size to provide consistent behavior. +* bump i_size to provide consistent behavior. Flush and unmap the rest +* of the range as well, so that a failure to do so leaves i_size alone. */ - error =filemap_write_and_wait_range (inode->i_mapping, isize, isize); + error =xfs_falloc_flush_shift_range (XFS_I (inode), offset);if (error)return error;
Signature · 1 line
2.55.0
PATCH v8 3/15 xfs: don't change the block map under a dirty folio
In reply to Chris MasonWith CONFIG_TRANSPARENT_HUGEPAGE=y, writeback on XFS can write in place to a shared block and corrupt the other file that shares it.
xfs_map_blocks() writes in place unless the COW fork has an extent for the block. write() and write faults reserve one before they dirty a folio over a shared block, so xfs_flush_unmap_range() writes back and truncates the range before a clone changes the block map. But the holder of a GUP write pin takes no filesystem lock, and the truncate keeps a large dirty folio that straddles the range:
clone pin holder writeback
===== ========== ========= writes the range back
dirties the folio
truncate keeps the folio
maps a shared block under it
dirties it again
finds no COW extent writes in place /* shared block */
The fix used here is to invalidate the range instead of truncating it, and to fail the call with -EINVAL if a dirty folio is left, before the block map changes.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/xfs/xfs_bmap_util.c | 17 +++++++++++++++-- fs/xfs/xfs_reflink.c | 2 +- 2 files changed, 16 insertions(+), 3 deletions(-)
fs/xfs/xfs_bmap_util.c +15 -2
@@ -863,8 +863,21 @@ xfs_flush_unmap_range( error =filemap_write_and_wait_range (inode->i_mapping, start, end);if (error)return error; -truncate_pagecache_range (inode, start, end); -return 0 ; + +/* +* The callers go on to change the block map, so no folio may stay over +* the range. The holder of a GUP pin can mark a folio dirty again +* after the flush, with no lock of ours. A truncate drops such a folio +* if it lies inside the range. If it is a large folio that straddles +* the range, the truncate cannot split it, so it zeroes a part of the +* folio, keeps it, and reports nothing. The invalidation removes whole +* folios, changes no data, and fails on a dirty folio. +*/ + error =invalidate_inode_pages2_range (inode->i_mapping, + start >>PAGE_SHIFT , end >>PAGE_SHIFT ); +if (error == -EBUSY ) + error = -EINVAL ; +return error; }int
fs/xfs/xfs_reflink.c +1 -1
@@ -1663,7 +1663,7 @@ xfs_reflink_zero_posteof(/* * Prepare two files for range cloning. Upon a successful return both inodes * will have the iolock and mmaplock held, the page cache of the out file will -* be truncated, and any leases on the out file will have been broken. This +* be invalidated, and any leases on the out file will have been broken. This * function borrows heavily from xfs_file_aio_write_checks. * * The VFS allows partial EOF blocks to "match" for dedupe even though it hasn't
Signature · 1 line
2.55.0
PATCH v8 4/15 iomap: don't punch delalloc under a dirty EOF block
In reply to Chris MasonAfter a failed buffered write, writeback on XFS can write in place to the shared block that contains EOF, and corrupt the other file that shares it.
After a failed buffered write, iomap_write_delalloc_release() punches the delalloc reservation of the write, except under dirty folios, which still need it. It looks for dirty folios only up to i_size, and punches the rest unconditionally. But XFS rounds that punch down to a block boundary, so the block that contains EOF loses its COW fork reservation, and writeback goes to the shared block:
block size 4096, i_size 5000, reservation 4096 to 8192 searched for dirty folios: 4096 to 5000
punched unconditionally: 5000 to 8192
rounded to blocks by XFS: 4096 to 8192
The fix used here is to look for dirty folios up to the end of the block that contains EOF.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/iomap/buffered-io.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)
fs/iomap/buffered-io.c +30 -4
@@ -1495,9 +1495,34 @@ static void iomap_write_delalloc_scan(struct inode *inode,* mapping_seek_hole_data() will return it, and we will skip the punch because * the folio is dirty. THis is incorrect - we always need to punch out delalloc * beyond EOF in this case as writeback will never write back and covert that -* delalloc block beyond EOF. Hence we limit the cached data scan range to EOF, -* resulting in always punching out the range from the EOF to the end of the -* range the iomap spans. +* delalloc block beyond EOF. Hence we limit the cached data scan range, and +* always punch out the range from the end of the scan to the end of the range +* the iomap spans. +* +* The scan cannot simply end at EOF, because EOF is a byte offset and a +* delalloc reservation covers whole blocks. If EOF lies inside a block, it +* splits that block in two: +* +* [block start, EOF) File data. If it is dirty it will be written +* back, so the block must keep its delalloc +* backing. +* [EOF, block end) Beyond EOF. By the rule above, this would always +* be punched out. +* +* Both parts share one reservation, and it cannot be split: a punch of a part +* of a block takes away the reservation for the whole block. If the scan ended +* at EOF, we would skip the punch for the first part because it is dirty, and +* then punch the second part, which takes the reservation away from under the +* dirty data in the first part. On XFS, writeback that has lost a COW fork +* reservation like this writes in place, over a block that is shared with +* another file. +* +* Hence we treat the EOF block as a whole, and let the cached data over it +* decide: the scan range is rounded up to the end of the block that EOF lies +* in. If the data over the EOF block is dirty, the scan moves the punch start +* past the block. If it is clean, the block is punched like any other block in +* the range. Only blocks that start at or beyond EOF are punched +* unconditionally. If EOF is block aligned, the round up changes nothing. * * Intervals are of the form [start_byte, end_byte) (i.e. open ended) because it * matches the intervals returned by mapping_seek_hole_data(). i.e. SEEK_DATA
@@ -1511,7 +1536,8 @@ void iomap_write_delalloc_release(struct inode *inode, loff_t start_byte, iomap_punch_t punch) {loff_t punch_start_byte = start_byte; -loff_t scan_end_byte =min (i_size_read (inode), end_byte); +loff_t scan_end_byte =min_t (u64, end_byte, +round_up ((u64 )i_size_read (inode),i_blocksize (inode)));/* * The caller must hold invalidate_lock to avoid races with page faults
Signature · 1 line
2.55.0
PATCH v8 5/15 fs: validate atomic writes after O_APPEND and limits
In reply to Chris MasonAn RWF_ATOMIC write on XFS with reflink can expose stale disk contents if O_APPEND or RLIMIT_FSIZE leaves it sector aligned but not block aligned.
XFS and ext4 call generic_atomic_write_valid() on the range that the caller passed. But generic_write_checks() changes the range later:O_APPEND moves the position, and RLIMIT_FSIZE cuts the length. XFS can then fill only a part of a new block, and still map the whole block into the file:
block size 4096, i_size 512, O_APPEND
validated: 0 to 4095
written: 512 to 4607
new blocks: 0 to 8191
stale below EOF: 0 to 511
The fix used here is to validate an atomic write in generic_write_checks(), after its range is final, and to refuse one that was cut. XFS no longer picks the path of an atomic write by the alignment of the range that the caller passed.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/ext4/file.c | 4 ---- fs/read_write.c | 13 +++++++++++++ fs/xfs/xfs_file.c | 17 +++++++++++------ 3 files changed, 24 insertions(+), 10 deletions(-)
fs/ext4/file.c +0 -4
@@ -802,10 +802,6 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)if (len <EXT4_SB (inode->i_sb)->s_awu_min || len >EXT4_SB (inode->i_sb)->s_awu_max)return -EINVAL ; - - ret =generic_atomic_write_valid (iocb, from); -if (ret) -return ret; }if (iocb->ki_flags &IOCB_DIRECT )
fs/read_write.c +13 -0
@@ -1775,6 +1775,19 @@ ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)if (ret)return ret; +/* +* O_APPEND has moved the position and the file size limits have +* shortened the write by now, so this is the range to validate. An +* atomic write is all or nothing, so refuse it if it got shorter. +*/ +if (iocb->ki_flags &IOCB_ATOMIC ) { +if (count !=iov_iter_count (from)) +return -EINVAL ; + ret =generic_atomic_write_valid (iocb, from); +if (ret) +return ret; + } +iov_iter_truncate (from, count);return iov_iter_count (from); }
fs/xfs/xfs_file.c +11 -6
@@ -926,12 +926,21 @@ xfs_file_dio_write(if ((iocb->ki_pos | count) & target->bt_logical_sectormask)return -EINVAL ; +/* +* An atomic write is at least one block long, and generic_write_checks() +* refuses it unless its final position is a multiple of its length. So +* there is no unaligned case to handle for it. +*/ +if (iocb->ki_flags &IOCB_ATOMIC ) { +if (xfs_is_zoned_inode (ip)) +return xfs_file_dio_write_zoned (ip, iocb, from); +return xfs_file_dio_write_atomic (ip, iocb, from); + } +if ((iocb->ki_pos | count) & ip->i_mount->m_blockmask)return xfs_file_dio_write_unaligned (ip, iocb, from);if (xfs_is_zoned_inode (ip))return xfs_file_dio_write_zoned (ip, iocb, from); -if (iocb->ki_flags &IOCB_ATOMIC ) -return xfs_file_dio_write_atomic (ip, iocb, from);return xfs_file_dio_write_aligned (ip, iocb, from, &xfs_direct_write_iomap_ops, &xfs_dio_write_ops,NULL ); }
@@ -1128,10 +1137,6 @@ xfs_file_write_iter(if (ocount >xfs_get_atomic_write_max (ip))return -EINVAL ; - - ret =generic_atomic_write_valid (iocb, from); -if (ret) -return ret; }if (IS_DAX (inode))
Signature · 1 line
2.55.0
PATCH v8 6/15 xfs: recheck direct write alignment after write checks
In reply to Chris MasonA direct write to a reflinked file on XFS can expose stale disk contents if O_APPEND or RLIMIT_FSIZE leaves it sector aligned but not block aligned.
A direct write cannot fill only a part of a shared block, so XFS does such a write through the page cache. xfs_file_dio_write() decides that from the range that the caller passed. But O_APPEND and RLIMIT_FSIZE change the range later, in xfs_file_write_checks():
block size 4096, i_size 4608, O_APPEND, block 1 is shared
checked: 0 to 4095, block aligned
written: 4608 to 8703
new COW block: 4096 to 8191
never written: 4096 to 4607The fix used here is to check the alignment again once the range is final, and to start over on the unaligned path if it no longer holds. A zoned file now falls back to buffered I/O ahead of that path.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/xfs/xfs_file.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-)
fs/xfs/xfs_file.c +38 -6
@@ -710,6 +710,17 @@ xfs_file_dio_write_aligned(if (ret) goto out_unlock; +/* +* O_APPEND moves the position and the file size limits shorten the +* write, so only now is the range final. If it is not block aligned, +* this is the wrong path for it. Return -EAGAIN to +* xfs_file_dio_write(), which picks the path. +*/ +if ((iocb->ki_pos |iov_iter_count (from)) & ip->i_mount->m_blockmask) { + ret = -EAGAIN ; + goto out_unlock; + } +/* * We don't need to hold the IOLOCK exclusively across the IO, so demote * the iolock back to shared if we had to take the exclusive lock in
@@ -921,6 +932,7 @@ xfs_file_dio_write(struct xfs_inode *ip =XFS_I (file_inode (iocb->ki_filp));struct xfs_buftarg *target =xfs_inode_buftarg (ip);size_t count =iov_iter_count (from); +ssize_t ret;/* direct I/O must be aligned to device logical sector size */ if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
@@ -937,12 +949,32 @@ xfs_file_dio_write(return xfs_file_dio_write_atomic (ip, iocb, from); } -if ((iocb->ki_pos | count) & ip->i_mount->m_blockmask) -return xfs_file_dio_write_unaligned (ip, iocb, from); -if (xfs_is_zoned_inode (ip)) -return xfs_file_dio_write_zoned (ip, iocb, from); -return xfs_file_dio_write_aligned (ip, iocb, from, - &xfs_direct_write_iomap_ops, &xfs_dio_write_ops,NULL ); +/* +* O_APPEND and the file size limits can still change the range, in +* xfs_file_write_checks(). So a range that is block aligned here may +* not stay so, and the write then comes back with -EAGAIN. If we are +* doing nonblocking user I/O, propagate the error. +*/ +if (!((iocb->ki_pos | count) & ip->i_mount->m_blockmask)) { +if (xfs_is_zoned_inode (ip)) + ret =xfs_file_dio_write_zoned (ip, iocb, from); +else + ret =xfs_file_dio_write_aligned (ip, iocb, from, + &xfs_direct_write_iomap_ops, + &xfs_dio_write_ops,NULL ); +if (ret != -EAGAIN || (iocb->ki_flags &IOCB_NOWAIT )) +return ret; + } + +/* +* Zoned inodes always write out of place, which does not work for a +* part of a block. Fall back to buffered I/O. +*/ +if (xfs_is_zoned_inode (ip)) { +trace_xfs_reflink_bounce_dio_write (iocb, from); +return -ENOTBLK ; + } +return xfs_file_dio_write_unaligned (ip, iocb, from); }static noinline ssize_t
Signature · 1 line
2.55.0
PATCH v8 7/15 xfs: test for reflink after the dio write checks
In reply to Chris MasonA direct write that is not block aligned can expose stale disk contents on XFS if it races with the first clone of the file.
xfs_file_dio_write_unaligned() falls back to buffered I/O if the file is reflinked, and tests for that right after it takes the iolock. But xfs_file_write_checks() runs after the test and can drop the iolock, for example if S_NOSEC is not set. A clone can run in that gap:
direct write clone
============ =====
takes the iolock shared
sees that the file is not reflinked
drops the iolock
shares the blocks
takes the iolock exclusive
writes a part of a new COW block
maps the whole block into the file
/* the rest was never written */
The fix used here is to move the test after xfs_file_write_checks().
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/xfs/xfs_file.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
fs/xfs/xfs_file.c +7 -5
@@ -877,9 +877,15 @@ xfs_file_dio_write_unaligned(if (ret)return ret; + ret =xfs_file_write_checks (iocb, from, &iolock,NULL ); +if (ret) + goto out_unlock; +/* * We can't properly handle unaligned direct I/O to reflink files yet, -* as we can't unshare a partial block. +* as we can't unshare a partial block. xfs_file_write_checks() can +* drop the iolock and take it again, and the file can be reflinked in +* between, so test for that only now. */ if (xfs_is_cow_inode (ip)) {trace_xfs_reflink_bounce_dio_write (iocb, from);
@@ -887,10 +893,6 @@ xfs_file_dio_write_unaligned( goto out_unlock; } - ret =xfs_file_write_checks (iocb, from, &iolock,NULL ); -if (ret) - goto out_unlock; -/* * If we are doing exclusive unaligned I/O, this must be the only I/O * in-flight. Otherwise we risk data corruption due to unwritten extent
Signature · 1 line
2.55.0
PATCH v8 8/15 xfs: refuse writes to files racing with realtime inode flag changes
In reply to Chris MasonFrom: Christoph Hellwig <[email protected]>
Zoned writes need to reserve space before taking the iolock to avoid deadlocks with garbage collection. But this means that the realtime inode flag which indicates if the file will be placed on the zoned realtime device or the main data device can be flipped after taking the reservation and before taking the write, or after deciding to go down the regular write path, both of which can't work.
Re-sample the realtime flag just before the actual write, and refuse the write if the flag changed. Do not retry the write using the now correct path, as switching the flag under a write is not a normal use case, and trying to flip between the zoned and regular path is an inherent life lock as it could be switched around again the next time this check is performed.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
fs/xfs/xfs_file.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
fs/xfs/xfs_file.c +13 -0
@@ -441,6 +441,19 @@ xfs_file_write_checks( goto restart; } +/* +* Now that we are in the same IOLOCK critical section as the actual +* write, check that the inode still has the realtime flag in the state +* that the caller expects. If not, some deliberately cleared it while +* I/O is ongoing, so bail out. +*/ +if (xfs_is_zoned_inode (XFS_I (inode)) != !!ac) { +xfs_warn_ratelimited (XFS_I (inode)->i_mount, +"RT flag modifying during write for inode 0x%llx." , + inode->i_ino); +return -EINVAL ; + } +/* * If the offset is beyond the size of the file, we need to zero all * blocks that fall between the existing EOF and the start of this
Signature · 1 line
2.55.0
PATCH v8 9/15 xfs: test for always COW after the dio write checks
In reply to Chris MasonA direct write to a zoned file from vectors that are not block aligned can silently corrupt the file's data, if the realtime flag is cleared and set again while the write starts.
Every bio of an always COW write has to cover whole blocks, so xfs_file_dio_write_aligned() passes IOMAP_DIO_FSBLOCK_ALIGNED for such an inode. But it tests for always COW before it takes the IOLOCK, and the realtime flag of a file with no blocks can change until then:
xfs_file_dio_write_zoned() FS_IOC_FSSETXATTR
========================== =================
xfs_zoned_write_space_reserve()
clears the realtime flag
xfs_file_dio_write_aligned()
xfs_is_always_cow_inode() is false
sets the realtime flag
xfs_ilock_iocb_for_write() xfs_file_write_checks()
iomap_dio_rw()
splits a bio inside a block
The fix used here is to test for always COW once the IOLOCK is held.
Before commit 8caec6c9fef7 ("xfs: support sub-block aligned vectors in always COW mode"), xfs_file_dio_write() made the same test with no lock held, to send such vectors down the unaligned path. So v6.15 to v6.18 have the same race, and need a different version of this fix.
---
Notes:
v7->v8:
- Added the Reviewed-by of Christoph Hellwig.
v6->v7:
- New patch.
fs/xfs/xfs_file.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
fs/xfs/xfs_file.c +9 -8
@@ -708,14 +708,6 @@ xfs_file_dio_write_aligned(unsigned int dio_flags =0 ;ssize_t ret; -/* -* For always COW inodes, each bio must be aligned to the file system -* block size and not just the device sector size because we need to -* allocate a block-aligned amount of space for each write. -*/ -if (xfs_is_always_cow_inode (ip)) - dio_flags |=IOMAP_DIO_FSBLOCK_ALIGNED ; - ret =xfs_ilock_iocb_for_write (iocb, &iolock);if (ret)return ret;
@@ -743,6 +735,15 @@ xfs_file_dio_write_aligned(xfs_ilock_demote (ip,XFS_IOLOCK_EXCL ); iolock =XFS_IOLOCK_SHARED ; } + +/* +* For always COW inodes, each bio must be aligned to the file system +* block size and not just the device sector size because we need to +* allocate a block-aligned amount of space for each write. The +* realtime flag can change until we hold the IOLOCK, so test only now. +*/ +if (xfs_is_always_cow_inode (ip)) + dio_flags |=IOMAP_DIO_FSBLOCK_ALIGNED ;if (mapping_stable_writes (iocb->ki_filp->f_mapping)) dio_flags |=IOMAP_DIO_BOUNCE ;trace_xfs_file_direct_write (iocb, from);
Signature · 1 line
2.55.0
PATCH v8 10/15 xfs: refuse fallocate racing with realtime flag changes
In reply to Chris MasonOn a zoned XFS, fallocate() can dereference a NULL pointer if the realtime flag of the file is set during the call.
xfs_file_fallocate() reserves zoned space only for a zoned inode, and tests for that before it takes the iolock. FS_IOC_FSSETXATTR can change the realtime flag of a file with no blocks in that gap. xfs_falloc_force_zero() then sees a zoned inode and reads the reservation, which is NULL:
fallocate(FALLOC_FL_ZERO_RANGE) FS_IOC_FSSETXATTR
=============================== =================
takes the iolock
sees that the inode is not zoned
waits for the iolock
sets the realtime flag drops the iolock
xfs_falloc_force_zero()
sees that the inode is zoned
/* ac is NULL */
The fix used here is to test again under the iolock, and to fail the call with -EINVAL if the answer has changed, as the write path does.
---
Notes:
v7->v8:
- No change.
v6->v7:
- No change.
- Was 9/10.
fs/xfs/xfs_file.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-)
fs/xfs/xfs_file.c +32 -9
@@ -1573,6 +1573,21 @@ xfs_falloc_allocate_range( FALLOC_FL_ZERO_RANGE | FALLOC_FL_INSERT_RANGE | \ FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_WRITE_ZEROES) +/* +* For zoned file systems, zeroing the first and last block of a hole punch +* requires allocating a new block to rewrite the remaining data and new zeroes +* out of place, which needs a space reservation. +*/ +static inline bool +xfs_falloc_needs_zoned_resv ( +struct xfs_inode *ip, +int mode) +{ +return xfs_is_zoned_inode (ip) && + (mode & (FALLOC_FL_PUNCH_HOLE |FALLOC_FL_ZERO_RANGE | +FALLOC_FL_COLLAPSE_RANGE )); +} + STATIC long __xfs_file_fallocate( struct file *file,
@@ -1591,6 +1606,19 @@ __xfs_file_fallocate(if (error) goto out_unlock; +/* +* The caller looked at the realtime flag before we took the IOLOCK, to +* decide if it has to reserve space. Bail out if the flag has changed +* since. +*/ +if (xfs_falloc_needs_zoned_resv (ip, mode) != !!ac) { +xfs_warn_ratelimited (ip->i_mount, +"RT flag modifying during fallocate for inode 0x%llx." , + inode->i_ino); + error = -EINVAL ; + goto out_unlock; + } +/* * Must wait for all AIO to complete before we continue as AIO can * change the file size on completion without holding any locks we
@@ -1686,16 +1714,11 @@ xfs_file_fallocate(return -EOPNOTSUPP ;/* -* For zoned file systems, zeroing the first and last block of a hole -* punch requires allocating a new block to rewrite the remaining data -* and new zeroes out of place. Get a reservations for those before -* taking the iolock. Dip into the reserved pool because we are -* expected to be able to punch a hole even on a completely full -* file system. +* Get the reservation before taking the iolock. Dip into the reserved +* pool because we are expected to be able to punch a hole even on a +* completely full file system. */ -if (xfs_is_zoned_inode (XFS_I (inode)) && - (mode & (FALLOC_FL_PUNCH_HOLE |FALLOC_FL_ZERO_RANGE | -FALLOC_FL_COLLAPSE_RANGE ))) +if (xfs_falloc_needs_zoned_resv (XFS_I (inode), mode))return xfs_file_zoned_fallocate (file, mode, offset, len);return __xfs_file_fallocate (file, mode, offset, len,NULL ); }
Signature · 1 line
2.55.0
PATCH v8 11/15 xfs: restart faults racing with realtime flag changes
In reply to Chris MasonOn a zoned XFS, a store through a shared mapping can dereference a NULL pointer, or corrupt the free block counter, if the realtime flag of the file changes during the write fault.
xfs_write_fault() reserves zoned space only for a zoned inode, and tests for that with no lock held. xfs_buffered_write_iomap_begin() tests again before it takes the ILOCK. FS_IOC_FSSETXATTR can change the flag of a file with no blocks between the two tests, since a fault never takes the IOLOCK:
write fault FS_IOC_FSSETXATTR
=========== =================
xfs_write_fault()
sees that the inode is not zoned
sets the realtime flag
xfs_buffered_write_iomap_begin()
sees that the inode is zoned
/* ac is NULL */
If the flag is cleared instead, xfs_zoned_buffered_write_iomap_begin() can be waiting for the ILOCK at that time. It then reserves blocks the zoned way on a regular inode, and the free block counter ends up too high.
The fix used here is to test again under the ILOCK, before blocks are reserved or allocated, and to make the fault start over if the flag has changed. Any other caller that gets there fails, as a write does.
---
Notes:
v7->v8:
- Only a page fault gets -EAGAIN. Any other caller that gets to one of the tests fails with -EINVAL, as a write and an fallocate do. pNFS can get there: xfs_fs_map_blocks() tests the flag before it takes its locks (Christoph Hellwig).
- The warning now says "during block mapping".
- The helper reads the inode number with I_INO(), and its comment no longer says that the flag is stable once the inode has blocks.
- Was 11/11.
v6->v7:
- xfs_direct_write_iomap_begin() tests the flag too, as soon as it holds the ILOCK. It could allocate blocks for a COW fork extent before it got to the test in xfs_iomap_write_direct().
- The assert on the extent size hint in xfs_zoned_buffered_write_iomap_begin() moved below the test of the flag.
- Dropped the Reviewed-by of Christoph Hellwig, since the patch changed.
- Was 10/10.
fs/xfs/xfs_iomap.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
fs/xfs/xfs_iomap.c +40 -2
@@ -264,6 +264,25 @@ xfs_iomap_eof_align_last_fsb(return end_fsb; } +/* +* A write fault picks the zoned or the regular path before it takes a lock, and +* it never takes the IOLOCK. The realtime flag cannot change under the ILOCK, +* so look at it again there, before reserving or allocating blocks. Only a +* fault can start over, so refuse any other caller, as the write path does. +*/ +static int +xfs_iomap_rtflag_changed ( +struct xfs_inode *ip, +unsigned int flags) +{ +xfs_warn_ratelimited (ip->i_mount, +"RT flag modifying during block mapping for inode 0x%llx." , +I_INO (ip)); +if (flags &IOMAP_FAULT ) +return -EAGAIN ; +return -EINVAL ; +} + int xfs_iomap_write_direct( struct xfs_inode *ip,
@@ -334,6 +353,11 @@ xfs_iomap_write_direct(if (error)return error; +if (xfs_is_zoned_inode (ip)) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_trans_cancel; + } + error =xfs_iext_count_extend (tp, ip,XFS_DATA_FORK , nr_exts);if (error) goto out_trans_cancel;
@@ -919,6 +943,11 @@ xfs_direct_write_iomap_begin(if (error)return error; +if (xfs_is_zoned_inode (ip)) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_unlock; + } +/* * The reflink iflag could have changed since the earlier unlocked * check, check if it again and relock if needed.
@@ -1666,9 +1695,7 @@ xfs_zoned_buffered_write_iomap_begin(struct xfs_iext_cursor icur;int error =0 ; -ASSERT (!xfs_get_extsz_hint (ip));ASSERT (!(flags &IOMAP_UNSHARE )); -ASSERT (ac);if (xfs_is_shutdown (mp))return -EIO ;
@@ -1682,6 +1709,12 @@ xfs_zoned_buffered_write_iomap_begin(if (error)return error; +if (!xfs_is_zoned_inode (ip) || !ac) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_unlock; + } +ASSERT (!xfs_get_extsz_hint (ip)); +if (XFS_IS_CORRUPT (mp, !xfs_ifork_has_extents (&ip->i_df)) ||XFS_TEST_ERROR (mp,XFS_ERRTAG_BMAPIFORMAT )) {xfs_bmap_mark_sick (ip,XFS_DATA_FORK );
@@ -1865,6 +1898,11 @@ xfs_buffered_write_iomap_begin(if (error)return error; +if (xfs_is_zoned_inode (ip)) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_unlock; + } +if (XFS_IS_CORRUPT (mp, !xfs_ifork_has_extents (&ip->i_df)) ||XFS_TEST_ERROR (mp,XFS_ERRTAG_BMAPIFORMAT )) {xfs_bmap_mark_sick (ip,XFS_DATA_FORK );
Signature · 1 line
2.55.0
Re: [PATCH v8 11/15] xfs: restart faults racing with realtime flag changes
Quoted 121 lines · Chris Mason
On a zoned XFS, a store through a shared mapping can dereference a NULL pointer, or corrupt the free block counter, if the realtime flag of the file changes during the write fault.
xfs_write_fault()reserves zoned space only for a zoned inode, and tests for that with no lock held.xfs_buffered_write_iomap_begin()tests again before it takes the ILOCK.FS_IOC_FSSETXATTRcan change the flag of a file with no blocks between the two tests, since a fault never takes the IOLOCK:write fault
FS_IOC_FSSETXATTR
=========== =================
xfs_write_fault()
sees that the inode is not zoned
sets the realtime flag
xfs_buffered_write_iomap_begin()
sees that the inode is zoned
/* ac is NULL */If the flag is cleared instead,
xfs_zoned_buffered_write_iomap_begin()can be waiting for the ILOCK at that time. It then reserves blocks the zoned way on a regular inode, and the free block counter ends up too high.The fix used here is to test again under the ILOCK, before blocks are reserved or allocated, and to make the fault start over if the flag has changed. Any other caller that gets there fails, as a write does.
Fixes 058dd70c65ab ("xfs: implement buffered writes to zoned RT devices")Signed-off-by Chris Mason <[email protected]>---
Notes:
v7->v8:
- Only a page fault gets-EAGAIN. Any other caller that gets to one of the tests fails with-EINVAL, as a write and an fallocate do. pNFS can get there:xfs_fs_map_blocks()tests the flag before it takes its locks (Christoph Hellwig).
- The warning now says "during block mapping".
- The helper reads the inode number withI_INO(), and its comment no longer says that the flag is stable once the inode has blocks.
- Was 11/11.
v6->v7:
-xfs_direct_write_iomap_begin()tests the flag too, as soon as it holds the ILOCK. It could allocate blocks for a COW fork extent before it got to the test inxfs_iomap_write_direct().
- The assert on the extent size hint inxfs_zoned_buffered_write_iomap_begin()moved below the test of the flag.
- Dropped the Reviewed-by of Christoph Hellwig, since the patch changed.
- Was 10/10.fs/xfs/xfs_iomap.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)fs/xfs/xfs_iomap.c +29
-2@@ -264,6 +264,25 @@ xfs_iomap_eof_align_last_fsb(return end_fsb; } +/* +* A write fault picks the zoned or the regular path before it takes a lock, and +* it never takes the IOLOCK. The realtime flag cannot change under the ILOCK, +* so look at it again there, before reserving or allocating blocks. Only a +* fault can start over, so refuse any other caller, as the write path does. +*/ +static int +xfs_iomap_rtflag_changed ( +struct xfs_inode *ip, +unsigned int flags) +{ +xfs_warn_ratelimited (ip->i_mount, +"RT flag modifying during block mapping for inode 0x%llx." , +I_INO (ip)); +if (flags &IOMAP_FAULT ) +return -EAGAIN ; +return -EINVAL ; +} + int xfs_iomap_write_direct( struct xfs_inode *ip,@@ -334,6 +353,11 @@ xfs_iomap_write_direct(if (error)return error; +if (xfs_is_zoned_inode (ip)) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_trans_cancel; + } + error =xfs_iext_count_extend (tp, ip,XFS_DATA_FORK , nr_exts);if (error) goto out_trans_cancel;@@ -919,6 +943,11 @@ xfs_direct_write_iomap_begin(if (error)return error; +if (xfs_is_zoned_inode (ip)) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_unlock; + } +/* * The reflink iflag could have changed since the earlier unlocked * check, check if it again and relock if needed. @@ -1666,9 +1695,7 @@ xfs_zoned_buffered_write_iomap_begin(struct xfs_iext_cursor icur;int error =0 ; -ASSERT (!xfs_get_extsz_hint (ip));ASSERT (!(flags &IOMAP_UNSHARE )); -ASSERT (ac);if (xfs_is_shutdown (mp))return -EIO ;
fs/xfs/xfs_iomap.c xfs_zoned_buffered_write_iomap_begin( · line 1715
Quoted 7 lines
fs/xfs/xfs_iomap.c +4
-0@@ -1682,6 +1709,12 @@ xfs_zoned_buffered_write_iomap_begin(if (error)return error; +if (!xfs_is_zoned_inode (ip) || !ac) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_unlock; + }
For actual writes this can't happens as we're under the IOLOCK (i_rwsem) taken in xfs_file_buffered_write here, and the only way to change the REALTIME flag is through vfs_fileattr_set which also takes i_rwsem in exclusive mode. So this should probably assert that IOMAP_FAULT is set.
Given that the MMAPLOCK (invalidate_lock) nests inside the IOLOCK, it might makes sense to also take the MMAPLOCK exclusive in xfs_fileattr_set and just check for the rtflag check early in __xfs_write_fault instead of leaking this into iomap for buffered I/O. This would also remove the need for the extra ac check.
fs/xfs/xfs_iomap.c xfs_buffered_write_iomap_begin( · line 1904
Quoted 8 lines
fs/xfs/xfs_iomap.c +4
-0@@ -1865,6 +1898,11 @@ xfs_buffered_write_iomap_begin(if (error)return error; +if (xfs_is_zoned_inode (ip)) { + error =xfs_iomap_rtflag_changed (ip, flags); + goto out_unlock; + }
Same here.
Quoted 5 lines
fs/xfs/xfs_iomap.c +0
-0if (XFS_IS_CORRUPT (mp, !xfs_ifork_has_extents (&ip->i_df)) ||XFS_TEST_ERROR (mp,XFS_ERRTAG_BMAPIFORMAT )) {xfs_bmap_mark_sick (ip,XFS_DATA_FORK );Signature · 1 line
2.55.0
---end quoted text---
PATCH v8 12/15 xfs: test for an extsize hint again under the ILOCK
In reply to Chris MasonA store through a shared mapping can corrupt the data fork and the free block counter on disk, if the realtime flag of the file is set during the write fault and realtime extents are larger than one block.
Such a realtime inode has an extent size hint, and cannot use delayed allocation. xfs_buffered_write_iomap_begin() tests for a hint before it takes the ILOCK. FS_IOC_FSSETXATTR can change the flag of a file with no blocks until then, since a fault never takes the IOLOCK:
write fault FS_IOC_FSSETXATTR
=========== =================
xfs_buffered_write_iomap_begin()
xfs_get_extsz_hint() is 0
sets the realtime flag
xfs_ilock_for_iomap() xfs_bmapi_reserve_delalloc()
/* 1 block, 0 realtime extents */
The fix used here is to test for a hint again once the ILOCK is held, and to allocate the blocks right away if the inode has got one.
---
Notes:
v7->v8:
- New patch.
fs/xfs/xfs_iomap.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/xfs/xfs_iomap.c +10 -0
@@ -1903,6 +1903,16 @@ xfs_buffered_write_iomap_begin( goto out_unlock; } +/* +* A write fault doesn't hold the IOLOCK, so the inode may have got an +* extent size hint since the test above. +*/ +if (xfs_get_extsz_hint (ip)) { +xfs_iunlock (ip, lockmode); +return xfs_direct_write_iomap_begin (inode, offset, count, + flags, iomap, srcmap); + } +if (XFS_IS_CORRUPT (mp, !xfs_ifork_has_extents (&ip->i_df)) ||XFS_TEST_ERROR (mp,XFS_ERRTAG_BMAPIFORMAT )) {xfs_bmap_mark_sick (ip,XFS_DATA_FORK );
Signature · 1 line
2.55.0
Re: [PATCH v8 12/15] xfs: test for an extsize hint again under the ILOCK
Looks good:
PATCH v8 13/15 xfs: resample the extsize hint after taking the ILOCK
In reply to Chris MasonA write fault on a file with an extent size hint can overrun its block reservation and shut down the filesystem, if it races with a change of the hint or of the realtime flag.
xfs_iomap_write_direct() sizes the reservation from the hint and the flag before xfs_trans_alloc_inode() takes the ILOCK, and the allocator reads both again under it. But a write fault only holds the MMAPLOCK, and FS_IOC_FSSETXATTR changes both on a file with no blocks without it:
write fault FS_IOC_FSSETXATTR
=========== =================
xfs_iomap_write_direct()
hint is 1 block
dblocks = 1 + bmbt blocks
xfs_ilock(ILOCK_EXCL) hint = 4096 blocks xfs_iunlock(ILOCK_EXCL)
xfs_trans_alloc_inode()
xfs_bmapi_write()
allocates 4096 blocks
xfs_trans_mod_sb()
t_blk_res_used > t_blk_res
The fix used here is to compare both with the values that sized the reservation once the ILOCK is held, and to start over if either has changed.
---
Notes:
v7->v8:
- New patch.
fs/xfs/xfs_iomap.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
fs/xfs/xfs_iomap.c +18 -3
@@ -295,9 +295,11 @@ xfs_iomap_write_direct(struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp;xfs_filblks_t resaligned; +xfs_extlen_t extsz;int nimaps;unsigned int dblocks, rblocks;bool force = false; +bool rt;int error;int bmapi_flags =XFS_BMAPI_PREALLOC ;int nr_exts =XFS_IEXT_ADD_NOSPLIT_CNT ;
@@ -312,9 +314,11 @@ xfs_iomap_write_direct(*/ count_fsb =min_t (xfs_filblks_t, count_fsb,XFS_MAX_BMBT_EXTLEN ); - resaligned =xfs_aligned_fsb_count (offset_fsb, count_fsb, -xfs_get_extsz_hint (ip)); -if (unlikely (XFS_IS_REALTIME_INODE (ip))) { +retry: + extsz =xfs_get_extsz_hint (ip); + rt =XFS_IS_REALTIME_INODE (ip); + resaligned =xfs_aligned_fsb_count (offset_fsb, count_fsb, extsz); +if (unlikely (rt)) { dblocks =XFS_DIOSTRAT_SPACE_RES (mp,0 ); rblocks = resaligned; }else {
@@ -358,6 +362,17 @@ xfs_iomap_write_direct( goto out_trans_cancel; } +/* +* A write fault doesn't hold the IOLOCK, so what sized the reservation +* can change until we hold the ILOCK. +*/ +if (extsz !=xfs_get_extsz_hint (ip) || + rt !=XFS_IS_REALTIME_INODE (ip)) { +xfs_trans_cancel (tp); +xfs_iunlock (ip,XFS_ILOCK_EXCL ); + goto retry; + } + error =xfs_iext_count_extend (tp, ip,XFS_DATA_FORK , nr_exts);if (error) goto out_trans_cancel;
Signature · 1 line
2.55.0
Re: [PATCH v8 13/15] xfs: resample the extsize hint after taking the ILOCK
Quoted 9 lines · Chris Mason
A write fault on a file with an extent size hint can overrun its block reservation and shut down the filesystem, if it races with a change of the hint or of the realtime flag.
xfs_iomap_write_direct()sizes the reservation from the hint and the flag beforexfs_trans_alloc_inode()takes the ILOCK, and the allocator reads both again under it. But a write fault only holds the MMAPLOCK, andFS_IOC_FSSETXATTRchanges both on a file with no blocks without it:
... and my previous suggestion of taking the mmlock in fileattr_set would take care of this as well.
Re: [PATCH v8 13/15] xfs: resample the extsize hint after taking the ILOCK
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Chris Mason
A write fault on a file with an extent size hint can overrun its block reservation and shut down the filesystem, if it races with a change of the hint or of the realtime flag.
Quoted 8 lines
Quoted 5 lines
xfs_iomap_write_direct()sizes the reservation from the hint and the flag beforexfs_trans_alloc_inode()takes the ILOCK, and the allocator reads both again under it. But a write fault only holds the MMAPLOCK, andFS_IOC_FSSETXATTRchanges both on a file with no blocks without it:... and my previous suggestion of taking the mmlock in
fileattr_setwould take care of this as well.
Yes, it would simplify a lot of this. I went this route because I thought it was nicer to have just the ILOCK control these things, but it would be easy to respin with the mmlock.
-chris
PATCH v8 14/15 xfs: resample the COW extsize hint after cycling ILOCK
In reply to Chris MasonA write fault on a shared extent of a file with an extent size hint, or of a DAX file, can overrun its block reservation and shut down the filesystem, if it races with a change of the COW extent size hint.
xfs_reflink_fill_cow_hole() sizes the reservation from the hint and drops the ILOCK for xfs_trans_alloc_inode(), and the allocator reads the hint again once the lock is back. But a write fault only holds the MMAPLOCK, and FS_IOC_FSSETXATTR changes the hint without it:
write fault FS_IOC_FSSETXATTR
=========== =================
xfs_reflink_fill_cow_hole()
hint is 1 block
dblocks = 1 + bmbt blocks
xfs_iunlock()
xfs_ilock(ILOCK_EXCL) hint = 4096 blocks xfs_iunlock(ILOCK_EXCL)
xfs_trans_alloc_inode()
xfs_bmapi_write()
allocates 4096 blocks
xfs_trans_mod_sb()
t_blk_res_used > t_blk_res
The fix used here is to compare the hint with the one that sized the reservation once the ILOCK is held again, and to start over if it has changed.
---
Notes:
v7->v8:
- New patch.
fs/xfs/xfs_reflink.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
fs/xfs/xfs_reflink.c +13 -1
@@ -445,14 +445,17 @@ xfs_reflink_fill_cow_hole(struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp;xfs_filblks_t resaligned; +xfs_extlen_t cowextsz;unsigned int seq_before =READ_ONCE (ip->i_df.if_seq);unsigned int dblocks =0 , rblocks =0 ;int nimaps;int error;bool found; +retry: + cowextsz =xfs_get_cowextsz_hint (ip); resaligned =xfs_aligned_fsb_count (imap->br_startoff, - imap->br_blockcount,xfs_get_cowextsz_hint (ip)); + imap->br_blockcount, cowextsz);if (XFS_IS_REALTIME_INODE (ip)) { dblocks =XFS_DIOSTRAT_SPACE_RES (mp,0 ); rblocks = resaligned;
@@ -471,6 +474,15 @@ xfs_reflink_fill_cow_hole( *lockmode =XFS_ILOCK_EXCL ; +/* +* A write fault doesn't hold the IOLOCK, so the hint that sized the +* reservation may have changed while we dropped the ILOCK. +*/ +if (cowextsz !=xfs_get_cowextsz_hint (ip)) { +xfs_trans_cancel (tp); + goto retry; + } +/* * The data fork mapping may have changed while we dropped the ILOCK * (a racing O_DIRECT writer under IOLOCK_SHARED can complete a full
Signature · 1 line
2.55.0
Re: [PATCH v8 14/15] xfs: resample the COW extsize hint after cycling ILOCK
another argument for taking the mmaplock in fileattr_get...
PATCH v8 15/15 xfs: don't enable DAX on zoned inodes
In reply to Chris MasonA store through a shared mapping can dereference a NULL pointer if the file is zoned and has S_DAX.
DAX writes in place, and zoned inodes are only written out of place, so the two can't work together. XFS doesn't check for this: with dax=always or FS_XFLAG_DAX, an inode gets S_DAX if its device does DAX, and an internal zoned section on pmem does. The write fault then takes the zoned path:
xfs_filemap_fault()
xfs_write_fault()
xfs_write_fault_zoned()
page_folio(vmf->page) /* NULL in a DAX fault */ftruncate() crashes in a similar way, in xfs_reflink_end_cow(), on kernels without "xfs: restart faults racing with realtime flag changes".
The fix used here is to never set S_DAX on a zoned inode, and to refuse to change the realtime flag of an S_DAX inode on a zoned file system.
Zoned files now use the page cache with dax=always or FS_XFLAG_DAX. A file whose realtime flag is cleared stays without DAX until its inode is evicted.
---
Notes:
v7->v8:
- New patch.
fs/xfs/xfs_ioctl.c | 8 +++++--- fs/xfs/xfs_iops.c | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)
fs/xfs/xfs_ioctl.c +5 -3
@@ -564,11 +564,13 @@ xfs_ioctl_setattr_xflags(/* * If S_DAX is enabled on this file, we can only switch the -* device if both support fsdax. We can't update S_DAX because -* there might be other threads walking down the access paths. +* device if both support fsdax, and zoned inodes never do. We +* can't update S_DAX because there might be other threads +* walking down the access paths. */ if (IS_DAX (VFS_I (ip)) && - (mp->m_ddev_targp->bt_daxdev ==NULL || + (xfs_has_zoned (mp) || + mp->m_ddev_targp->bt_daxdev ==NULL || (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_daxdev ==NULL )))return -EINVAL ;
fs/xfs/xfs_iops.c +4 -0
@@ -1373,6 +1373,10 @@ xfs_inode_supports_dax(if (mp->m_sb.sb_blocksize !=PAGE_SIZE )return false; +/* Zoned inodes are always written out of place, DAX writes in place. */ +if (xfs_is_zoned_inode (ip)) +return false; +/* Device has to support DAX too. */ return xfs_inode_buftarg (ip)->bt_daxdev !=NULL ; }
Signature · 1 line
2.55.0
Re: [PATCH v8 15/15] xfs: don't enable DAX on zoned inodes
Quoted 4 lines · Chris Mason
A store through a shared mapping can dereference a NULL pointer if the file is zoned and has
S_DAX.DAX writes in place,
DAX does not actually require in-place writes, we had patches to support DAX with reflink for example.
Quoted 10 lines
and zoned inodes are only written out of place, so
the two can't work together. XFS doesn't check for this: with dax=always orFS_XFLAG_DAX, an inode getsS_DAXif its device does DAX, and an internal zoned section on pmem does. The write fault then takes the zoned path:xfs_filemap_fault() xfs_write_fault() xfs_write_fault_zoned() page_folio(vmf->page) /* NULL in a DAX fault */
but this is real issue, and xfs_write_fault_zoned would need to find a way to deal with pfn faults.
I think thi could be supported, but there's no real reason to bother as there is no good use yet (at least right now). So I think your change is fine, but the commit log and comments are not. Just say that the zone code does not support DAX, and maybe mention this callchain.