Re: [PATCH v4] ntfs: trim the sizes after a short write
From: liubaolin
Date: Fri Nov 13 2026 - 08:44:39 EST
在 2026/11/13 13:01, Zhan Xusheng 写道:
From: Zhan Xusheng <zhanxusheng@xxxxxxxxxx>
ntfs_file_write_iter() moves the data size and, for a non-resident
uncompressed file, the initialized size to pos + count before the copy.
__ntfs_extend_initialized_size() only zeroes [old initialized size, pos),
so the range [pos, pos + count) is initialized only by the copy itself.
When iomap_file_buffered_write() copies less than count, it returns a
positive short count and the error rollback at out: does not run. The
file keeps the size pos + count, and [pos + ret, pos + count) reads back
whatever the freed clusters held before, also after a remount. A local
user can read the old contents of freed clusters this way.
Run the existing rollback for a short write as well, and roll the data
size back to the end of what was written. For a buffered write, keep
the initialized size and zero what the copy did not reach, up to the
end of the block holding pos + count, because a page fault through a
shared mapping may already rely on that initialized size. A direct
write rolls the initialized size back as before.
Fixes: b041ca562526 ("ntfs: update iomap and address space operations")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Zhan Xusheng <zhanxusheng@xxxxxxxxxx>
---
v4: for a buffered write, zero the unwritten range instead of
truncating the page cache and taking the initialized size back, which
dropped data stored through a shared mapping during the copy (Baolin).
v3: clear the cache up to EOF, not up to the extended initialized
size, which left the rest of the block holding pos + count (Baolin).
v2: also clear the cached tail of the last block, which v1 left in
place when the write stayed inside the old data size (Baolin).
v3: https://lore.kernel.org/all/20261112093233.1118469-1-zhanxusheng1024@xxxxxxxxx/
v2: https://lore.kernel.org/all/20261112030815.982026-1-zhanxusheng1024@xxxxxxxxx/
v1: https://lore.kernel.org/all/20261111093459.660484-1-zhanxusheng1024@xxxxxxxxx/
Tested in a VM on ntfs-next b025f526c5d0, with all free space filled
with a known pattern first. Short buffered writes of 4M, old data
readable after drop_caches and after remount, 3 runs each:
unfixed v3 v4
fallocated, ends block-aligned 3137536 0 0
fallocated, ends inside a block 3141332 0 0
fallocated, unaligned start 3137536 0 0
same, pos + count in that page - 0 0
append 3137536 0 0
With the sequence from the v3 review (marker stored through a shared
mapping at 64K while pwritev() at 0 is stalled, short count 8192), the
marker is kept on unfixed and v4 and lost on v3, 3 runs each. A store
into [pos + ret, pos + count) itself during the copy is zeroed with v4,
as that range belongs to the write. fsx (buffered, no-mmap, O_DIRECT
and mixed), compressed files, fallocate, insert/collapse range and
punch hole give the same results with v4 as with v3.
---
fs/ntfs/file.c | 57 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 50 insertions(+), 7 deletions(-)
diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index 3213a0c9f96b..5ac11d60be37 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -928,10 +928,21 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
ret = iomap_file_buffered_write(iocb, from, &ntfs_write_iomap_ops,
&ntfs_iomap_folio_ops, NULL);
out:
- if (ret < 0 && ret != -EIOCBQUEUED) {
+ /*
+ * Both sizes were moved to end before the copy, so a short write,
+ * like a failed one, must take back what it did not write.
+ */
+ if (ret < count && ret != -EIOCBQUEUED) {
+ loff_t new_init_size = old_init_size;
+ loff_t new_data_size = old_data_size;
int init_err = 0;
int data_err = 0;
+ if (ret > 0) {
+ new_init_size = max(old_init_size, pos + ret);
+ new_data_size = max(old_data_size, pos + ret);
+ }
+
/*
* Like ntfs_setattr_size(): __ntfs_attr_truncate_vfs()
* expects the invalidate lock held across the size
@@ -939,14 +950,46 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
*/
filemap_invalidate_lock(vi->i_mapping);
+ /*
+ * A page fault through a shared mapping may have used the
+ * extended initialized size while a buffered write was copying,
+ * so taking it back could drop that data. Zero what the write
+ * did not reach instead, including the rest of the block at end
+ * that the copy may have read from disk, unless such a fault
+ * already moved the initialized size past end and zeroed it.
+ */
+ if (!(iocb->ki_flags & IOCB_DIRECT) && NInoNonResident(ni) &&
+ !NInoCompressed(ni) && ni->initialized_size > new_init_size) {
+ loff_t zend = ni->initialized_size > end ? end :
+ round_up(end, i_blocksize(vi));
+
+ zend = min(zend, new_data_size);
+ if (zend > new_init_size)
+ init_err = iomap_zero_range(vi, new_init_size,
+ zend - new_init_size, NULL,
+ &ntfs_seek_iomap_ops,
+ &ntfs_iomap_folio_ops, NULL);
Hi Xusheng,
Thanks for v4. I reran the buffered mmap test from my v3 review and confirmed that the marker is now preserved.
However, there is still a problem with the zeroing range here.
If the write fails without copying any bytes, new_init_size remains old_init_size, which can be below pos. A concurrent mmap write may have already stored valid data in that gap.
For example, start with a 1 MiB file with initialized_size == 0. Pause a 4 KiB buffered write at 128 KiB on its first source page, then store a marker through a shared mapping at 64 KiB. If source access subsequently fails with EFAULT, this code starts zeroing at offset zero and overwrites the marker, although it is outside the requested write range.
I reproduced this both with and without fallocate(). The marker is also lost after remount.
Could we use a separate zeroing start, bounded by max(pos, new_init_size), while keeping new_init_size for the metadata update?
Thanks,
Baolin.
+ new_init_size = min(ni->initialized_size, new_data_size);
+ }
+
/* ntfs_attr_set_initialized_size() rejects a resident one. */
- if (ni->initialized_size != old_init_size &&
+ if (!init_err && ni->initialized_size != new_init_size &&
NInoNonResident(ni)) {
struct ntfs_inode *mrec_ni = ntfs_base_inode(ni);
+ /*
+ * A direct write did not use the page cache; drop what
+ * a concurrent reader may have cached past the new
+ * initialized size, as __ntfs_extend_initialized_size()
+ * does.
+ */
+ if (iocb->ki_flags & IOCB_DIRECT)
+ truncate_pagecache_range(vi, new_init_size,
+ max_t(loff_t, i_size_read(vi),
+ ni->initialized_size) - 1);
mutex_lock(&mrec_ni->mrec_lock);
init_err = ntfs_attr_set_initialized_size(ni,
- old_init_size);
+ new_init_size);
Hi XuSheng,
The mmap race can also happen on this DIO path. A direct write can block while faulting in its source pages, and another thread can complete a shared mmap write during that time. The inode lock held by the write does not serialize that mmap activity.
I tested a 12 KiB O_DIRECT pwritev() at offset zero, paused on a source-page fault, with a concurrent mmap store at 64 KiB. When pwritev() subsequently returned EFAULT, the marker was lost,including after remount.
The initialized-size rollback already had this race before the patch. This cache truncation also discards the concurrent writer's data, so we cannot assume that the pages being dropped only contain data cached by a reader.
We need to preserve the initialization established by mmap on this path too. Removing the cache truncation alone would not be enough,since rolling initialized_size back would still hide that data.
Thanks,
Baolin.
mutex_unlock(&mrec_ni->mrec_lock);
}
/*
@@ -955,13 +998,13 @@ ssize_t ntfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
* the uninitialized tail, so skip it unless the rollback
* above restored that size.
*/
- if (ni->data_size != old_data_size && !init_err) {
+ if (ni->data_size != new_data_size && !init_err) {
Hi XuSheng,
What happens if the new iomap_zero_range() call fails?
Both the initialized-size update above and this data-size rollback are skipped, leaving the extended sizes in place. Setting NVolErrors and logging the failure does not prevent subsequent reads.
I made folio acquisition return -ENOMEM during zeroing after a short write, to simulate an allocation failure. With an old file size of 16 KiB, a 32 KiB request returned 8192 but left the file size at 32 KiB. fsync() succeeded, and the size was still 32 KiB after remount. Without the injected failure, it was restored to 16 KiB.
This test only checked the sizes, not the contents of the unwritten range. But if zeroing fails, we cannot assume that the range covered by the published initialized_size has been initialized.
Could we handle this failure without leaving that range exposed?
Simply returning an error would not repair the initialized size that has already been published.
Thanks,
Baolin.
struct ntfs_inode *mrec_ni = ntfs_base_inode(ni);
- truncate_setsize(vi, old_data_size);
+ truncate_setsize(vi, new_data_size);
mutex_lock(&mrec_ni->mrec_lock);
- data_err = __ntfs_attr_truncate_vfs(ni, old_data_size,
- old_data_size);
+ data_err = __ntfs_attr_truncate_vfs(ni, new_data_size,
+ new_data_size);
mutex_unlock(&mrec_ni->mrec_lock);
/* Like ntfs_setattr_size(): stay on the durable size. */
if (data_err)
base-commit: b025f526c5d04929212c9d5fb7ffc96608f3eacc