Add buffered write-through support to iomap & xfs

mm/page-writeback.c EXPORT_SYMBOL(__folio_cancel_dirty); · line 2853

Quoted 8 lines · Ojaswin Mujoo

mm/page-writeback.c +3 -0

@@ -2847,8 +2847,11 @@ EXPORT_SYMBOL(__folio_cancel_dirty);
  *
  * This incoherency between the folio's dirty flag and xarray tag is
  * unfortunate, but it only exists while the folio is locked.
+ *
+ * For some cases we might not want to do mkclean, eg, if we've already taken
+ * care of it, hence pass the should_mkclean flag to indicate if its needed.
  */

The kerneldoc should move down to the exported version.

This adds initial support for performing buffered non-aio RWF_WRITETHROUGH write. The rough flow for a writethrough write is as follows:

1. Acquire inode lock
2. initialize writethrough context (wt_ctx) and mark
mapping as stable.
3. Start the iomap_iter() loop. For each iomap:
3.1. Acquire folio and folio_lock.
3.2. perform memcpy from user buffer to the folio and mark it
dirty
3.3. Wait for any current writeback to complete and then call
folio_mkclean() to prevent mmap writes from changing it.
3.4. Start writeback on the folio
3.5. Add the folio range under write to wt_ctx->bvec and folio_unlock() 3.6. If bvec is full, submit the current bvecs for IO.
3.7. Repeat 3.2 to 3.6 till the whole iomap is processed. Submit
the final set of bvecs for IO.
4. Repeat step 3 till we have no more data to write.
5. Finally, sleep in the syscall thread till all the IOs are completed (refcount == 0). Once that happens, the end io handler will wake us up.
6. Upon waking up, call fs ->end_io() callback (which updates inode size), record any errors and return.
7. inode_unlock()

This design gives buffered writethrough the same semantics as dio and any error in the IO is directly returned to the caller. The design has delibrately open coded the IO submission and completion flow (inspired by dio) rather than reusing the dio functions as accomodating buffered writethrough logic in dio code was polluting it with too many if else conditionals and special cases.

Suggested-by Jan Kara <[email protected]>

Suggested-by Dave Chinner <[email protected]>

Co-developed-by Ritesh Harjani (IBM) <[email protected]>

Signed-off-by Ritesh Harjani (IBM) <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

fs/iomap/buffered-io.c +28 -0

+ssize_t iomap_file_writethrough_write(struct kiocb *iocb, struct iov_iter *i,
+				      const struct iomap_writethrough_ops *wt_ops,
+				      void *private)
+{
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
+	struct iomap_iter iter = {
+		.inode		= inode,
+		.pos		= iocb->ki_pos,
+		.len		= iov_iter_count(i),
+		.flags		= IOMAP_WRITE | IOMAP_WRITETHROUGH,
+		.private	= private,
+	};
+	struct iomap_writethrough_ctx *wt_ctx;
+	unsigned int max_bvecs;
+	ssize_t ret;
+
+
+	/*
+	 * For now we don't support any other flag with WRITETHROUGH
+	 */
+	if (!(iocb->ki_flags & IOCB_WRITETHROUGH))
+		return -EINVAL;
+	if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_DONTCACHE))
+		return -EINVAL;
+	if (iocb_is_dsync(iocb))
+		/* D_SYNC support not implemented yet */
+		return -EOPNOTSUPP;
+	if (!is_sync_kiocb(iocb))

fs/iomap/buffered-io.c +6 -0

+
+	wt_ctx = kzalloc(struct_size(wt_ctx, bvec, max_bvecs), GFP_NOFS);
+	if (!wt_ctx)
+		return -ENOMEM;
+
+	wt_ctx->iocb = iocb;

fs/iomap/buffered-io.c +10 -0

+	while ((ret = iomap_iter(&iter, wt_ops->ops)) > 0) {
+		WARN_ON(iter.iomap.type != IOMAP_UNWRITTEN &&
+			iter.iomap.type != IOMAP_MAPPED);
+		iter.status = iomap_writethrough_iter(wt_ctx, &iter, i, wt_ops);
+	}
+	if (ret < 0)
+		cmpxchg(&wt_ctx->error, 0, ret);
+
+	if (!atomic_dec_and_test(&wt_ctx->ref)) {
+		for (;;) {

fs/iomap/buffered-io.c +0 -0

@@ -1096,6 +1097,276 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
 	return __iomap_write_end(iter->inode, pos, len, copied, folio);
 }
 

fs/iomap/buffered-io.c +14 -0

+		if (err)
+			ret = err;
+	}
+
+	mapping_clear_stable_writes(inode->i_mapping);
+
+	if (!ret) {
+		ret = wt_ctx->written;
+		iocb->ki_pos = wt_ctx->pos + ret;
+	}
+
+	kfree(wt_ctx);
+	return ret;
+}

fs/iomap/buffered-io.c +88 -0

+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{
+	ssize_t total_written = 0;
+	int status = 0;
+	struct address_space *mapping = iter->inode->i_mapping;
+	size_t chunk = mapping_max_folio_size(mapping);
+	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
+	unsigned int bs = i_blocksize(iter->inode);
+
+	/* copied over based on DIO handles these flags */
+	if (iter->iomap.type == IOMAP_UNWRITTEN)
+		wt_ctx->flags |= IOMAP_DIO_UNWRITTEN;
+	if (iter->iomap.flags & IOMAP_F_SHARED)
+		wt_ctx->flags |= IOMAP_DIO_COW;
+
+	if (!(iter->flags & IOMAP_WRITETHROUGH))
+		return -EINVAL;
+
+	do {
+		struct folio *folio;
+		size_t offset;		/* Offset into folio */
+		u64 bytes;		/* Bytes to write to folio */
+		size_t copied;		/* Bytes copied from user */
+		u64 written;		/* Bytes have been written */
+		loff_t pos;
+		size_t off_aligned, len_aligned;
+
+		bytes = iov_iter_count(i);
+retry:
+		offset = iter->pos & (chunk - 1);
+		bytes = min(chunk - offset, bytes);
+		status = balance_dirty_pages_ratelimited_flags(mapping,
+							       bdp_flags);
+		if (unlikely(status))
+			break;
+
+		/*
+		 * If completions already occurred and reported errors, give up
+		 * now and don't bother submitting more bios.
+		 */
+		if (unlikely(data_race(wt_ctx->error))) {
+			wt_ctx->nr_bvecs = 0;
+			break;
+		}
+
+		if (bytes > iomap_length(iter))
+			bytes = iomap_length(iter);
+
+		/*
+		 * Bring in the user page that we'll copy from _first_.
+		 * Otherwise there's a nasty deadlock on copying from the
+		 * same page as we're writing to, without it being marked
+		 * up-to-date.
+		 *
+		 * For async buffered writes the assumption is that the user
+		 * page has already been faulted in. This can be optimized by
+		 * faulting the user page.
+		 */
+		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
+			status = -EFAULT;
+			break;
+		}
+
+		status = iomap_write_begin(iter, wt_ops->write_ops, &folio,
+					   &offset, &bytes);
+		if (unlikely(status)) {
+			iomap_write_failed(iter->inode, iter->pos, bytes);
+			break;
+		}
+		if (iter->iomap.flags & IOMAP_F_STALE)
+			break;
+
+		pos = iter->pos;
+
+		if (mapping_writably_mapped(mapping))
+			flush_dcache_folio(folio);
+
+		copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
+		written = iomap_write_end(iter, bytes, copied, folio) ?
+			  copied : 0;
+
+		if (!written)
+			goto put_folio;
+
+		off_aligned = round_down(offset, bs);

fs/iomap/buffered-io.c +13 -0

+
+		if (wt_ctx->nr_bvecs == wt_ctx->max_bvecs)
+			iomap_writethrough_submit_bio(wt_ctx, &iter->iomap, wt_ops);
+
+put_folio:
+		__iomap_put_folio(iter, wt_ops->write_ops, written, folio);
+
+		cond_resched();
+		if (unlikely(written == 0)) {
+			iomap_write_failed(iter->inode, pos, bytes);
+			iov_iter_revert(i, copied);
+
+			if (chunk > PAGE_SIZE)

Some more thoughs... :)

Quoted 5 lines

fs/iomap/buffered-io.c +0 -0

@@ -1096,6 +1097,276 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
 	return __iomap_write_end(iter->inode, pos, len, copied, folio);
 }
 

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1109

Quoted 10 lines

fs/iomap/buffered-io.c +10 -0

+static ssize_t iomap_writethrough_complete(struct iomap_writethrough_ctx *wt_ctx)
+{
+	struct kiocb *iocb	= wt_ctx->iocb;
+	struct inode *inode	= wt_ctx->inode;
+	ssize_t ret		= wt_ctx->error;
+
+	if (wt_ctx->dops && wt_ctx->dops->end_io) {
+		int err = wt_ctx->dops->end_io(iocb, wt_ctx->written,
+						wt_ctx->error,
+						wt_ctx->flags);

It's a bit odd to use only ->end_io from dops also because we don't really use direct IO submission path. So perhaps you can have just an end_io handler pointer in iomap_writethrough_ops similarly as you have a submission one?

Quoted 15 lines

fs/iomap/buffered-io.c +14 -0

+		if (err)
+			ret = err;
+	}
+
+	mapping_clear_stable_writes(inode->i_mapping);
+
+	if (!ret) {
+		ret = wt_ctx->written;
+		iocb->ki_pos = wt_ctx->pos + ret;
+	}
+
+	kfree(wt_ctx);
+	return ret;
+}

...

Quoted 93 lines

fs/iomap/buffered-io.c +93 -0

+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{
+	ssize_t total_written = 0;
+	int status = 0;
+	struct address_space *mapping = iter->inode->i_mapping;
+	size_t chunk = mapping_max_folio_size(mapping);
+	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
+	unsigned int bs = i_blocksize(iter->inode);
+
+	/* copied over based on DIO handles these flags */
+	if (iter->iomap.type == IOMAP_UNWRITTEN)
+		wt_ctx->flags |= IOMAP_DIO_UNWRITTEN;
+	if (iter->iomap.flags & IOMAP_F_SHARED)
+		wt_ctx->flags |= IOMAP_DIO_COW;
+
+	if (!(iter->flags & IOMAP_WRITETHROUGH))
+		return -EINVAL;
+
+	do {
+		struct folio *folio;
+		size_t offset;		/* Offset into folio */
+		u64 bytes;		/* Bytes to write to folio */
+		size_t copied;		/* Bytes copied from user */
+		u64 written;		/* Bytes have been written */
+		loff_t pos;
+		size_t off_aligned, len_aligned;
+
+		bytes = iov_iter_count(i);
+retry:
+		offset = iter->pos & (chunk - 1);
+		bytes = min(chunk - offset, bytes);
+		status = balance_dirty_pages_ratelimited_flags(mapping,
+							       bdp_flags);
+		if (unlikely(status))
+			break;
+
+		/*
+		 * If completions already occurred and reported errors, give up
+		 * now and don't bother submitting more bios.
+		 */
+		if (unlikely(data_race(wt_ctx->error))) {
+			wt_ctx->nr_bvecs = 0;
+			break;
+		}
+
+		if (bytes > iomap_length(iter))
+			bytes = iomap_length(iter);
+
+		/*
+		 * Bring in the user page that we'll copy from _first_.
+		 * Otherwise there's a nasty deadlock on copying from the
+		 * same page as we're writing to, without it being marked
+		 * up-to-date.
+		 *
+		 * For async buffered writes the assumption is that the user
+		 * page has already been faulted in. This can be optimized by
+		 * faulting the user page.
+		 */
+		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
+			status = -EFAULT;
+			break;
+		}
+
+		status = iomap_write_begin(iter, wt_ops->write_ops, &folio,
+					   &offset, &bytes);
+		if (unlikely(status)) {
+			iomap_write_failed(iter->inode, iter->pos, bytes);
+			break;
+		}
+		if (iter->iomap.flags & IOMAP_F_STALE)
+			break;
+
+		pos = iter->pos;
+
+		if (mapping_writably_mapped(mapping))
+			flush_dcache_folio(folio);
+
+		copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
+		written = iomap_write_end(iter, bytes, copied, folio) ?
+			  copied : 0;
+
+		if (!written)
+			goto put_folio;
+
+		off_aligned = round_down(offset, bs);
+		len_aligned = round_up(offset + written, bs) - off_aligned;
+
+		iomap_folio_prepare_writethrough(folio, off_aligned,
+						 len_aligned);
+

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1334

Quoted 5 lines

fs/iomap/buffered-io.c +5 -0

+		if (!wt_ctx->nr_bvecs)
+			wt_ctx->bio_pos = round_down(pos, bs);
+
+		bvec_set_folio(&wt_ctx->bvec[wt_ctx->nr_bvecs], folio,
+			       len_aligned, off_aligned);

Shouldn't we zero out the tail of the folio if we are submitting partial folio for write?

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1339

Quoted 6 lines

fs/iomap/buffered-io.c +5 -0

+		wt_ctx->nr_bvecs++;
+		wt_ctx->written += written;
+
+		if (pos + written > wt_ctx->new_i_size)
+			wt_ctx->new_i_size = pos + written;

I'm probably missing something here but where is i_size update handled? I don't see new_i_size used anywhere?

Also why is it OK to not call pagecache_isize_extended() but that goes with the i_size update...

Quoted 22 lines

fs/iomap/buffered-io.c +21 -0

+
+		if (wt_ctx->nr_bvecs == wt_ctx->max_bvecs)
+			iomap_writethrough_submit_bio(wt_ctx, &iter->iomap, wt_ops);
+
+put_folio:
+		__iomap_put_folio(iter, wt_ops->write_ops, written, folio);
+
+		cond_resched();
+		if (unlikely(written == 0)) {
+			iomap_write_failed(iter->inode, pos, bytes);
+			iov_iter_revert(i, copied);
+
+			if (chunk > PAGE_SIZE)
+				chunk /= 2;
+			if (copied) {
+				bytes = copied;
+				goto retry;
+			}
+		} else {
+			total_written += written;
+			iomap_iter_advance(iter, written);

Review

Quoted 2 lines

(quoted diff) +2 -0

+		}
+	} while (iov_iter_count(i) && iomap_length(iter));

Overall the differences of this function from iomap_write_iter() seem relatively small so maybe it would be possible to just extend iomap_write_iter() to support writethrough IO as well? Basically once we've copied data into the folio and called iomap_write_end() we can have "if writethrough, call function to prepare & submit the folio for IO".

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1369

Quoted 8 lines

fs/iomap/buffered-io.c +7 -0

+
+	if (wt_ctx->nr_bvecs)
+		iomap_writethrough_submit_bio(wt_ctx, &iter->iomap, wt_ops);
+
+	return total_written ? 0 : status;
+}
+

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 94 lines · Jan Kara

Quoted 94 lines

fs/iomap/buffered-io.c +93 -0

@@ -1096,6 +1097,276 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{
+	ssize_t total_written = 0;
+	int status = 0;
+	struct address_space *mapping = iter->inode->i_mapping;
+	size_t chunk = mapping_max_folio_size(mapping);
+	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
+	unsigned int bs = i_blocksize(iter->inode);
+
+	/* copied over based on DIO handles these flags */
+	if (iter->iomap.type == IOMAP_UNWRITTEN)
+		wt_ctx->flags |= IOMAP_DIO_UNWRITTEN;
+	if (iter->iomap.flags & IOMAP_F_SHARED)
+		wt_ctx->flags |= IOMAP_DIO_COW;
+
+	if (!(iter->flags & IOMAP_WRITETHROUGH))
+		return -EINVAL;
+
+	do {
+		struct folio *folio;
+		size_t offset;		/* Offset into folio */
+		u64 bytes;		/* Bytes to write to folio */
+		size_t copied;		/* Bytes copied from user */
+		u64 written;		/* Bytes have been written */
+		loff_t pos;
+		size_t off_aligned, len_aligned;
+
+		bytes = iov_iter_count(i);
+retry:
+		offset = iter->pos & (chunk - 1);
+		bytes = min(chunk - offset, bytes);
+		status = balance_dirty_pages_ratelimited_flags(mapping,
+							       bdp_flags);
+		if (unlikely(status))
+			break;
+
+		/*
+		 * If completions already occurred and reported errors, give up
+		 * now and don't bother submitting more bios.
+		 */
+		if (unlikely(data_race(wt_ctx->error))) {
+			wt_ctx->nr_bvecs = 0;
+			break;
+		}
+
+		if (bytes > iomap_length(iter))
+			bytes = iomap_length(iter);
+
+		/*
+		 * Bring in the user page that we'll copy from _first_.
+		 * Otherwise there's a nasty deadlock on copying from the
+		 * same page as we're writing to, without it being marked
+		 * up-to-date.
+		 *
+		 * For async buffered writes the assumption is that the user
+		 * page has already been faulted in. This can be optimized by
+		 * faulting the user page.
+		 */
+		if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
+			status = -EFAULT;
+			break;
+		}
+
+		status = iomap_write_begin(iter, wt_ops->write_ops, &folio,
+					   &offset, &bytes);
+		if (unlikely(status)) {
+			iomap_write_failed(iter->inode, iter->pos, bytes);
+			break;
+		}
+		if (iter->iomap.flags & IOMAP_F_STALE)
+			break;
+
+		pos = iter->pos;
+
+		if (mapping_writably_mapped(mapping))
+			flush_dcache_folio(folio);
+
+		copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
+		written = iomap_write_end(iter, bytes, copied, folio) ?
+			  copied : 0;
+
+		if (!written)
+			goto put_folio;
+
+		off_aligned = round_down(offset, bs);
+		len_aligned = round_up(offset + written, bs) - off_aligned;
+
+		iomap_folio_prepare_writethrough(folio, off_aligned,
+						 len_aligned);
+

Quoted 8 lines

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1334

Quoted 5 lines

fs/iomap/buffered-io.c +5 -0

+		if (!wt_ctx->nr_bvecs)
+			wt_ctx->bio_pos = round_down(pos, bs);
+
+		bvec_set_folio(&wt_ctx->bvec[wt_ctx->nr_bvecs], folio,
+			       len_aligned, off_aligned);

Shouldn't we zero out the tail of the folio if we are submitting partial folio for write?

Hmm, so for the folio range we zeroout if needed in __iomap_write_begin(). I think that should take care of this right?

Quoted 8 lines

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1339

Quoted 5 lines

fs/iomap/buffered-io.c +5 -0

+		wt_ctx->nr_bvecs++;
+		wt_ctx->written += written;
+
+		if (pos + written > wt_ctx->new_i_size)
+			wt_ctx->new_i_size = pos + written;

I'm probably missing something here but where is i_size update handled? I don't see new_i_size used anywhere?

So the i_size update happens in endio(), similar to dio. We initially had the update in iomap_writethrough_iter in v1 however based on Dave's feedback [1], moved it to the endio. The idea is for writethrough semantics to be closer to dio hence we either update isize when we succeffuly write, or return an error to user without update isize.

[1] https://lore.kernel.org/linux-fsdevel/aa--rBKQG7ck5nuM@dread/

Quoted 2 lines

Also why is it OK to not call pagecache_isize_extended() but that goes with the i_size update...

As for pagecache_isize_extended(), (this might be a bit tangential from your comment but) after this email, I started diggin a bit more into why it is needed. As per my understanding, it tackles 2 things:

Problem 1. mkclean's the old EOF folio so that the FS can fault again. This allows us to allocate new blocks which previously might not be allocated if bs < ps.

Problem 2. Since mmap writes can dirty data beyond EOF, we zero the range from old EOF to end of that folio so that readers dont read junk data after isize extension.

Another thing I noticed is that most users of iomap_file_buffered_write() do their own eof zeroing in the FS layer (eg, xfs_file_write_zero_eof(), ext4's new changes, ntfs_extend_initialized_size() etc).
I think this FS level zerooing should take care of mkcleaning the eof folio (problem 1), as they call iomap_zero_range() which would flush the eof range anyways. So am I right in assuming that for FSes that do their own zeroing, 1. is already taken care of?

As for 2, I think after the EOF zeroing of the FS, there might be a window before iomap_write_iter() where an mmap writer can still dirty EOF blocks, hence the pagecache_isize_extended() would be needed here. But doesn't that then make the eof zeroing in the FS layer redundant? Am I missing something here?

Regardless, for our case I think we will also need to do the pagecache_isize_extended(), mainly to take care of problem 2, but where exactly should we do it now? We currently change the isize in endio() but for aio, it can run outside inode or folio lock. I think this function needs to be called under inode lock(). Hmm.. its a bit late here so I'll revisit this tomorrow with a fresh mind :)

Quoted 7 lines · Ojaswin Mujoo

Quoted 6 lines · Jan Kara

Quoted 6 lines

fs/iomap/buffered-io.c +5 -0

@@ -1096,6 +1097,276 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{

Quoted 10 lines

your comment but) after this email, I started diggin a bit more into why it is needed. As per my understanding, it tackles 2 things:

Problem 1. mkclean's the old EOF folio so that the FS can fault again. This allows us to allocate new blocks which previously might not be allocated if bs < ps.

Problem 2. Since mmap writes can dirty data beyond EOF, we zero the range from old EOF to end of that folio so that readers dont read junk data after isize extension.

Correct.

Quoted 8 lines

Another thing I noticed is that most users of iomap_file_buffered_write() do their own eof zeroing in the FS layer (eg, xfs_file_write_zero_eof(), ext4's new changes, ntfs_extend_initialized_size() etc).
I think this FS level zerooing should take care of mkcleaning the eof folio (problem 1), as they call iomap_zero_range() which would flush the eof range anyways. So am I right in assuming that for FSes that do their own zeroing, 1. is already taken care of?

Well, I don't see anything that would writeprotect the old tail page in iomap_zero_range(). I think iomap_zero_range() calls are there mostly to address 2. Not only due to mmap but also possibly to clear whatever junk there can be in the blocks after EOF.

Quoted 6 lines

As for 2, I think after the EOF zeroing of the FS, there might be a window before iomap_write_iter() where an mmap writer can still dirty EOF blocks, hence the pagecache_isize_extended() would be needed here. But doesn't that then make the eof zeroing in the FS layer redundant? Am I missing something here?

Hmm, I agree the zeroing looks duplicit (for some users of pagecache_isize_extended()). And yes, doing the zeroing from xfs_file_write_zero_eof() is somewhat racy (mmap writer can still come and write non-zeros before we update i_size) but I'd have hard time to argue it really practically matters - you are racing mmap writes with buffered writes so any kind of write atomicity guarantees are not there.

Quoted 7 lines

Regardless, for our case I think we will also need to do the pagecache_isize_extended(), mainly to take care of problem 2, but where exactly should we do it now? We currently change the isize in endio() but for aio, it can run outside inode or folio lock. I think this function needs to be called under inode lock(). Hmm.. its a bit late here so I'll revisit this tomorrow with a fresh mind :)

I think mainly to take care of problem 1... You are correct about inode_lock but since we are updating i_size, we should be better holding it, shouldn't we?

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 8 lines · Jan Kara

Quoted 7 lines · Ojaswin Mujoo

Quoted 6 lines · Jan Kara

Quoted 6 lines

fs/iomap/buffered-io.c +5 -0

@@ -1096,6 +1097,276 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{

<...>

Quoted 22 lines

Quoted 10 lines

your comment but) after this email, I started diggin a bit more into why it is needed. As per my understanding, it tackles 2 things:

Problem 1. mkclean's the old EOF folio so that the FS can fault again. This allows us to allocate new blocks which previously might not be allocated if bs < ps.

Problem 2. Since mmap writes can dirty data beyond EOF, we zero the range from old EOF to end of that folio so that readers dont read junk data after isize extension.

Correct.

Quoted 8 lines

Another thing I noticed is that most users of iomap_file_buffered_write() do their own eof zeroing in the FS layer (eg, xfs_file_write_zero_eof(), ext4's new changes, ntfs_extend_initialized_size() etc).
I think this FS level zerooing should take care of mkcleaning the eof folio (problem 1), as they call iomap_zero_range() which would flush the eof range anyways. So am I right in assuming that for FSes that do their own zeroing, 1. is already taken care of?

Quoted 4 lines

Well, I don't see anything that would writeprotect the old tail page in iomap_zero_range(). I think iomap_zero_range() calls are there mostly to address 2. Not only due to mmap but also possibly to clear whatever junk there can be in the blocks after EOF.

Well I was thinking more like if the EOF page was mmap'd it would be dirty and blocks beyond EOF would be unmapped, so iomap_zero_range() will write it back which shall mkclean() the folio.

But I think the same race we discussed for problem 2 can also occur here.

Thread 1 (extending write) Thread 2 (mmap writer)

iomap_zero_range()
filemap_write_and_wait_range()

                                       // mmaps & writes EOF range

iomap_write_iter()
isize = new_size
// pagecache_isize_extended() is
needed to mkclean() old EOF page.

Quoted 6 lines

Quoted 5 lines

As for 2, I think after the EOF zeroing of the FS, there might be a window before iomap_write_iter() where an mmap writer can still dirty EOF blocks, hence the pagecache_isize_extended() would be needed here. But doesn't that then make the eof zeroing in the FS layer redundant? Am I missing something here?

Quoted 6 lines

Hmm, I agree the zeroing looks duplicit (for some users of pagecache_isize_extended()). And yes, doing the zeroing from xfs_file_write_zero_eof() is somewhat racy (mmap writer can still come and write non-zeros before we update i_size) but I'd have hard time to argue it really practically matters - you are racing mmap writes with buffered writes so any kind of write atomicity guarantees are not there.

Yeah, seems like it is not enough to take care of either 1 or 2 and pagecache_isize_extended() should maybe be enough. I was just wondering if we could optimize it away even for normal extend path (no racing mmap), we can avoid the expensive folio_zero_range() calls.

Regardless, Ive not looked at this more closely and its a separate issue so we can revisit it later. For now I wanted some clarity around pagecache_isize_extended() so thanks for that.

Quoted 7 lines

Quoted 6 lines

Regardless, for our case I think we will also need to do the pagecache_isize_extended(), mainly to take care of problem 2, but where exactly should we do it now? We currently change the isize in endio() but for aio, it can run outside inode or folio lock. I think this function needs to be called under inode lock(). Hmm.. its a bit late here so I'll revisit this tomorrow with a fresh mind :)

Quoted 3 lines

I think mainly to take care of problem 1... You are correct about inode_lock but since we are updating i_size, we should be better holding it, shouldn't we?

Yes you are correct. In the aio writethrough codepath, the inode update is happening without the inode lock which is wrong. I overlooked the fact that even aio dio uses IOMAP_DIO_FORCE_WAIT to force isize update under inode lock, and we should do something similar as well.

So in v3, I make the change that for extending writes we shall always finish them in "sync" fashion so ->endio() runs under inode lock. Then, after ->endio() in iomap_dio_complete(), I will call pagecache_isize_extended() to take care of this. Just like isize update right now, the isize_extension only runs when the IO was successful otherwise we return an error to the user. This gives us semantics like dio while handling extension properly.

Does that sound okay?

Quoted 48 lines · Ojaswin Mujoo

Quoted 8 lines · Jan Kara

Quoted 7 lines · Ojaswin Mujoo

Quoted 6 lines · Jan Kara

Quoted 6 lines

fs/iomap/buffered-io.c +5 -0

@@ -1096,6 +1097,276 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{

<...>

Quoted 22 lines

Quoted 10 lines

your comment but) after this email, I started diggin a bit more into why it is needed. As per my understanding, it tackles 2 things:

Problem 1. mkclean's the old EOF folio so that the FS can fault again. This allows us to allocate new blocks which previously might not be allocated if bs < ps.

Problem 2. Since mmap writes can dirty data beyond EOF, we zero the range from old EOF to end of that folio so that readers dont read junk data after isize extension.

Correct.

Quoted 8 lines

Another thing I noticed is that most users of iomap_file_buffered_write() do their own eof zeroing in the FS layer (eg, xfs_file_write_zero_eof(), ext4's new changes, ntfs_extend_initialized_size() etc).
I think this FS level zerooing should take care of mkcleaning the eof folio (problem 1), as they call iomap_zero_range() which would flush the eof range anyways. So am I right in assuming that for FSes that do their own zeroing, 1. is already taken care of?

Quoted 4 lines

Well, I don't see anything that would writeprotect the old tail page in iomap_zero_range(). I think iomap_zero_range() calls are there mostly to address 2. Not only due to mmap but also possibly to clear whatever junk there can be in the blocks after EOF.

Well I was thinking more like if the EOF page was mmap'd it would be dirty and blocks beyond EOF would be unmapped, so iomap_zero_range() will write it back which shall mkclean() the folio.

But I think the same race we discussed for problem 2 can also occur here.

Thread 1 (extending write) Thread 2 (mmap writer)

Quoted 7 lines

iomap_zero_range()
filemap_write_and_wait_range()

                                       // mmaps & writes EOF range

iomap_write_iter()
isize = new_size
// pagecache_isize_extended() is
needed to mkclean() old EOF page.

Yes, this race exists and unlike in the case of zeroing where it is mostly harmless not guranteeing calling page_mkwrite() with updated i_size can lead to filesystem tripping on assertions, data loss or similar.

Quoted 19 lines

Quoted 7 lines

Quoted 6 lines

As for 2, I think after the EOF zeroing of the FS, there might be a window before iomap_write_iter() where an mmap writer can still dirty EOF blocks, hence the pagecache_isize_extended() would be needed here. But doesn't that then make the eof zeroing in the FS layer redundant? Am I missing something here?

Quoted 6 lines

Hmm, I agree the zeroing looks duplicit (for some users of pagecache_isize_extended()). And yes, doing the zeroing from xfs_file_write_zero_eof() is somewhat racy (mmap writer can still come and write non-zeros before we update i_size) but I'd have hard time to argue it really practically matters - you are racing mmap writes with buffered writes so any kind of write atomicity guarantees are not there.

Yeah, seems like it is not enough to take care of either 1 or 2 and pagecache_isize_extended() should maybe be enough. I was just wondering if we could optimize it away even for normal extend path (no racing mmap), we can avoid the expensive folio_zero_range() calls.

Quoted 3 lines

Regardless, Ive not looked at this more closely and its a separate issue so we can revisit it later. For now I wanted some clarity around pagecache_isize_extended() so thanks for that.

Well, but pagecache_isize_extended() doesn't guarantee on disk blocks are zeroed out as well as it doesn't dirty the page. Also xfs_file_write_zero_eof() potentially handles zeroing of more than a tail page. So you cannot simply drop one of these.

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Regardless, for our case I think we will also need to do the pagecache_isize_extended(), mainly to take care of problem 2, but where exactly should we do it now? We currently change the isize in endio() but for aio, it can run outside inode or folio lock. I think this function needs to be called under inode lock(). Hmm.. its a bit late here so I'll revisit this tomorrow with a fresh mind :)

I think mainly to take care of problem 1... You are correct about inode_lock but since we are updating i_size, we should be better holding it, shouldn't we?

Quoted 4 lines

Yes you are correct. In the aio writethrough codepath, the inode update is happening without the inode lock which is wrong. I overlooked the fact that even aio dio uses IOMAP_DIO_FORCE_WAIT to force isize update under inode lock, and we should do something similar as well.

Yes.

Quoted 3 lines

So in v3, I make the change that for extending writes we shall always finish them in "sync" fashion so ->endio() runs under inode lock. Then, after ->endio() in iomap_dio_complete(), I will call

Quoted 6 lines

pagecache_isize_extended() to take care of this. Just like isize update right now, the isize_extension only runs when the IO was successful otherwise we return an error to the user. This gives us semantics like dio while handling extension properly.

Does that sound okay?

Yep, sounds fine.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

3.3. Wait for any current writeback to complete and then call
folio_mkclean() to prevent mmap writes from changing it.
3.4. Start writeback on the folio

fs/iomap/buffered-io.c +25 -0

+
+/**
+ * iomap_writethrough_begin - prepare the various structures for writethrough
+ * @folio: folio to prepare for writethrough
+ * @off: offset of write within folio
+ * @len: len of write within folio
+ *
+ * This function does the major preparation work needed before starting the
+ * writethrough. The main task is to prepare folio for writeththrough by blocking
+ * mmap writes and setting writeback on it. Further, we must clear the write range
+ * to non-dirty. If this results in the complete folio becoming non-dirty, then we
+ * need to clear the master dirty bit.
+ */
+static void iomap_folio_prepare_writethrough(struct folio *folio, size_t off,
+					     size_t len)
+{
+	bool fully_written;
+	u64 zero = 0;
+
+	if (folio_test_writeback(folio))
+		folio_wait_writeback(folio);
+
+	if (folio_mkclean(folio))
+		folio_mark_dirty(folio);
+

<snip>

Quoted 3 lines

3.3. Wait for any current writeback to complete and then call
folio_mkclean() to prevent mmap writes from changing it.
3.4. Start writeback on the folio

<snip>

Quoted 25 lines

fs/iomap/buffered-io.c +25 -0

+
+/**
+ * iomap_writethrough_begin - prepare the various structures for writethrough
+ * @folio: folio to prepare for writethrough
+ * @off: offset of write within folio
+ * @len: len of write within folio
+ *
+ * This function does the major preparation work needed before starting the
+ * writethrough. The main task is to prepare folio for writeththrough by blocking
+ * mmap writes and setting writeback on it. Further, we must clear the write range
+ * to non-dirty. If this results in the complete folio becoming non-dirty, then we
+ * need to clear the master dirty bit.
+ */
+static void iomap_folio_prepare_writethrough(struct folio *folio, size_t off,
+					     size_t len)
+{
+	bool fully_written;
+	u64 zero = 0;
+
+	if (folio_test_writeback(folio))
+		folio_wait_writeback(folio);
+
+	if (folio_mkclean(folio))
+		folio_mark_dirty(folio);
+

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1217

Quoted 10 lines

fs/iomap/buffered-io.c +10 -0

+	/*
+	 * We might either write through the complete folio or a partial folio
+	 * writethrough might result in all blocks becoming non-dirty, so we need to
+	 * check and mark the folio clean if that is the case.
+	 */
+	fully_written = (off == 0 && len == folio_size(folio));
+	iomap_clear_range_dirty(folio, off, len);
+	if (fully_written ||
+	    !iomap_find_dirty_range(folio, &zero, folio_size(folio)))
+		folio_clear_dirty_for_writethrough(folio);

I get the point where you want to prevent mmap writes before the current writeback completes. It is not very obvious to me why folio_mkclean and folio_mark_dirty had to be moved out and then we call the folio_clear_dirty_for_writethrough wrapper. Anyway those calls are made after we do folio_wait_writeback right?
Is to cover the cases where we do partial write in which case we don't end up calling folio_clear_dirty_for_io? If that is the case, then a small comment on top of folio_mkclean would make it a bit more clear.

--
Pankaj

fs/iomap/buffered-io.c +14 -0

+	bio->bi_iter.bi_sector	= iomap_sector(iomap, wt_ctx->bio_pos);
+	bio->bi_end_io		= iomap_writethrough_bio_end_io;
+	bio->bi_private		= wt_ctx;
+
+	for (i = 0; i < wt_ctx->nr_bvecs; i++)
+		__bio_add_page(bio, wt_ctx->bvec[i].bv_page,
+				wt_ctx->bvec[i].bv_len,
+				wt_ctx->bvec[i].bv_offset);
+
+	atomic_inc(&wt_ctx->ref);
+	submit_bio(bio);
+	wt_ctx->nr_bvecs = 0;
+}
+

fs/iomap/buffered-io.c +50 -0

+
+/**
+ * iomap_writethrough_iter - perform RWF_WRITETHROUGH buffered write
+ * @wt_ctx: writethrough context
+ * @iter: iomap iter holding mapping information
+ * @i: iov_iter for write
+ * @wt_ops: the fs callbacks needed for writethrough
+ *
+ * This function copies the user buffer to folio similar to usual buffered
+ * IO path, with the difference that we immediately issue the IO. For this we
+ * utilize IO submission and completion mechanism that is inspired by dio.
+ *
+ * Folio handling note: We might be writing through a partial folio so we need
+ * to be careful to not clear the folio dirty bit unless there are no dirty blocks
+ * in the folio after the writethrough.
+ */
+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{
+	ssize_t total_written = 0;
+	int status = 0;
+	struct address_space *mapping = iter->inode->i_mapping;
+	size_t chunk = mapping_max_folio_size(mapping);
+	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
+	unsigned int bs = i_blocksize(iter->inode);
+
+	/* copied over based on DIO handles these flags */
+	if (iter->iomap.type == IOMAP_UNWRITTEN)
+		wt_ctx->flags |= IOMAP_DIO_UNWRITTEN;
+	if (iter->iomap.flags & IOMAP_F_SHARED)
+		wt_ctx->flags |= IOMAP_DIO_COW;
+
+	if (!(iter->flags & IOMAP_WRITETHROUGH))
+		return -EINVAL;
+
+	do {
+		struct folio *folio;
+		size_t offset;		/* Offset into folio */
+		u64 bytes;		/* Bytes to write to folio */
+		size_t copied;		/* Bytes copied from user */
+		u64 written;		/* Bytes have been written */
+		loff_t pos;
+		size_t off_aligned, len_aligned;
+
+		bytes = iov_iter_count(i);
+retry:
+		offset = iter->pos & (chunk - 1);
+		bytes = min(chunk - offset, bytes);

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1169

Quoted 6 lines

fs/iomap/buffered-io.c +6 -0

+
+	if (wt_ops->writethrough_submit)
+		wt_ops->writethrough_submit(wt_ctx->inode, iomap, wt_ctx->bio_pos,
+					    len);
+
+	bio = bio_alloc(iomap->bdev, wt_ctx->nr_bvecs, REQ_OP_WRITE, GFP_NOFS);

We might want to check if bio_alloc succeeded here.

Quoted 15 lines

fs/iomap/buffered-io.c +14 -0

+	bio->bi_iter.bi_sector	= iomap_sector(iomap, wt_ctx->bio_pos);
+	bio->bi_end_io		= iomap_writethrough_bio_end_io;
+	bio->bi_private		= wt_ctx;
+
+	for (i = 0; i < wt_ctx->nr_bvecs; i++)
+		__bio_add_page(bio, wt_ctx->bvec[i].bv_page,
+				wt_ctx->bvec[i].bv_len,
+				wt_ctx->bvec[i].bv_offset);
+
+	atomic_inc(&wt_ctx->ref);
+	submit_bio(bio);
+	wt_ctx->nr_bvecs = 0;
+}
+

<snip>

Quoted 50 lines

fs/iomap/buffered-io.c +50 -0

+
+/**
+ * iomap_writethrough_iter - perform RWF_WRITETHROUGH buffered write
+ * @wt_ctx: writethrough context
+ * @iter: iomap iter holding mapping information
+ * @i: iov_iter for write
+ * @wt_ops: the fs callbacks needed for writethrough
+ *
+ * This function copies the user buffer to folio similar to usual buffered
+ * IO path, with the difference that we immediately issue the IO. For this we
+ * utilize IO submission and completion mechanism that is inspired by dio.
+ *
+ * Folio handling note: We might be writing through a partial folio so we need
+ * to be careful to not clear the folio dirty bit unless there are no dirty blocks
+ * in the folio after the writethrough.
+ */
+static int iomap_writethrough_iter(struct iomap_writethrough_ctx *wt_ctx,
+				   struct iomap_iter *iter, struct iov_iter *i,
+				   const struct iomap_writethrough_ops *wt_ops)
+
+{
+	ssize_t total_written = 0;
+	int status = 0;
+	struct address_space *mapping = iter->inode->i_mapping;
+	size_t chunk = mapping_max_folio_size(mapping);
+	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
+	unsigned int bs = i_blocksize(iter->inode);
+
+	/* copied over based on DIO handles these flags */
+	if (iter->iomap.type == IOMAP_UNWRITTEN)
+		wt_ctx->flags |= IOMAP_DIO_UNWRITTEN;
+	if (iter->iomap.flags & IOMAP_F_SHARED)
+		wt_ctx->flags |= IOMAP_DIO_COW;
+
+	if (!(iter->flags & IOMAP_WRITETHROUGH))
+		return -EINVAL;
+
+	do {
+		struct folio *folio;
+		size_t offset;		/* Offset into folio */
+		u64 bytes;		/* Bytes to write to folio */
+		size_t copied;		/* Bytes copied from user */
+		u64 written;		/* Bytes have been written */
+		loff_t pos;
+		size_t off_aligned, len_aligned;
+
+		bytes = iov_iter_count(i);
+retry:
+		offset = iter->pos & (chunk - 1);
+		bytes = min(chunk - offset, bytes);

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1280

Quoted 10 lines

fs/iomap/buffered-io.c +10 -0

+		status = balance_dirty_pages_ratelimited_flags(mapping,
+							       bdp_flags);
+		if (unlikely(status))
+			break;
+
+		/*
+		 * If completions already occurred and reported errors, give up
+		 * now and don't bother submitting more bios.
+		 */
+		if (unlikely(data_race(wt_ctx->error))) {

In the unlikely scenario where we encounter an error, do we have to also clear the writeback flag on all the folios that is part of this bvec until now?

Something like explicitly iterate over wt_ctx->bvec[0] through wt_ctx->bvec[nr_bvecs - 1], manually call folio_end_writeback(bvec[i].bv_page) on them, and then discard the bvecs by setting the nr_bvecs = 0;

I am wondering if the folios that were processed until now will be in PG_WRITEBACK state which can affect reclaim as we never clear the flag.

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1284

Quoted 5 lines

fs/iomap/buffered-io.c +4 -0

+			wt_ctx->nr_bvecs = 0;
+			break;
+		}
+

--
Pankaj

Quoted 8 lines · Pankaj Raghav (Samsung)

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1169

Quoted 6 lines

fs/iomap/buffered-io.c +6 -0

+
+	if (wt_ops->writethrough_submit)
+		wt_ops->writethrough_submit(wt_ctx->inode, iomap, wt_ctx->bio_pos,
+					    len);
+
+	bio = bio_alloc(iomap->bdev, wt_ctx->nr_bvecs, REQ_OP_WRITE, GFP_NOFS);

We might want to check if bio_alloc succeeded here.

Hi Pankaj, so we pass GFP_NOFS which has GFP_DIRECT_RECLAIM and according to comment over bio_alloc()

* If %__GFP_DIRECT_RECLAIM is set then bio_alloc will always be able to
* allocate a bio. This is due to the mempool guarantees. To make this work,
* callers must never allocate more than 1 bio at a time from the general pool.

And we seem to be following this.

Quoted 14 lines

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1174

Quoted 6 lines

fs/iomap/buffered-io.c +5 -0

+	bio->bi_iter.bi_sector	= iomap_sector(iomap, wt_ctx->bio_pos);
+	bio->bi_end_io		= iomap_writethrough_bio_end_io;
+	bio->bi_private		= wt_ctx;
+
+	for (i = 0; i < wt_ctx->nr_bvecs; i++)

In the unlikely scenario where we encounter an error, do we have to also clear the writeback flag on all the folios that is part of this bvec until now?

Something like explicitly iterate over wt_ctx->bvec[0] through wt_ctx->bvec[nr_bvecs - 1], manually call folio_end_writeback(bvec[i].bv_page) on them, and then discard the bvecs by setting the nr_bvecs = 0;

Quoted 2 lines

I am wondering if the folios that were processed until now will be in PG_WRITEBACK state which can affect reclaim as we never clear the flag.

Hey Pankaj, yes you are right. I think the error handling is a bit buggy and Sashiko has also pointed some of these. I'll take care of this in v3, thanks for pointing this out.

Quoted 10 lines · Ojaswin Mujoo

Quoted 8 lines · Pankaj Raghav (Samsung)

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1169

Quoted 6 lines

fs/iomap/buffered-io.c +6 -0

+
+	if (wt_ops->writethrough_submit)
+		wt_ops->writethrough_submit(wt_ctx->inode, iomap, wt_ctx->bio_pos,
+					    len);
+
+	bio = bio_alloc(iomap->bdev, wt_ctx->nr_bvecs, REQ_OP_WRITE, GFP_NOFS);

We might want to check if bio_alloc succeeded here.

Quoted 9 lines

Hi Pankaj, so we pass GFP_NOFS which has GFP_DIRECT_RECLAIM and according to comment over bio_alloc()

* If %__GFP_DIRECT_RECLAIM is set then bio_alloc will always be able to
* allocate a bio. This is due to the mempool guarantees. To make this work,
* callers must never allocate more than 1 bio at a time from the general pool.

And we seem to be following this.

Makes sense. Thanks for the clarification.

Quoted 17 lines

Quoted 16 lines

fs/iomap/buffered-io.c static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied, · line 1174

Quoted 6 lines

fs/iomap/buffered-io.c +5 -0

+	bio->bi_iter.bi_sector	= iomap_sector(iomap, wt_ctx->bio_pos);
+	bio->bi_end_io		= iomap_writethrough_bio_end_io;
+	bio->bi_private		= wt_ctx;
+
+	for (i = 0; i < wt_ctx->nr_bvecs; i++)

In the unlikely scenario where we encounter an error, do we have to also clear the writeback flag on all the folios that is part of this bvec until now?

Something like explicitly iterate over wt_ctx->bvec[0] through wt_ctx->bvec[nr_bvecs - 1], manually call folio_end_writeback(bvec[i].bv_page) on them, and then discard the bvecs by setting the nr_bvecs = 0;

I am wondering if the folios that were processed until now will be in PG_WRITEBACK state which can affect reclaim as we never clear the flag.

Quoted 4 lines

Hey Pankaj, yes you are right. I think the error handling is a bit buggy and Sashiko has also pointed some of these. I'll take care of this in v3, thanks for pointing this out.

FWIW, I got the following panic on xfs/011 (not reproducible all the time) when I was running the xfstests with 16k block size with the writethrough patches:

[76313.736356] INFO: task fsstress:1845687 blocked for more than 122 seconds.
[76313.738751]       Not tainted 7.0.0-08885-g97cbd56b7479 #43
[76313.740650] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this 
message.
[76313.743311] task:fsstress        state:D stack:0     pid:1845687 tgid:1845687 
ppid:1845685 task_flags:0x400140 flags:0x00080000
[76313.747137] Call Trace:
[76313.748000]  <TASK>
[76313.748830]  __schedule+0xcc2/0x3c40
[76313.750129]  ? __pfx___schedule+0x10/0x10
[76313.751479]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.753214]  schedule+0x78/0x2e0
[76313.754334]  io_schedule+0x92/0x100
[76313.755597]  folio_wait_bit_common+0x26a/0x6f0
[76313.757156]  ? __pfx_folio_wait_bit_common+0x10/0x10
[76313.758873]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.760508]  ? xas_load+0x19/0x260
[76313.761693]  ? __pfx_wake_page_function+0x10/0x10
[76313.763386]  ? __pfx_filemap_get_entry+0x10/0x10
[76313.764948]  folio_wait_writeback+0x58/0x190
[76313.766499]  __filemap_get_folio_mpol+0x56d/0x800
[76313.768085]  ? kvm_read_and_reset_apf_flags+0x4a/0x70
[76313.769899]  iomap_write_begin+0xea7/0x1e90
[76313.771304]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.773016]  ? asm_exc_page_fault+0x22/0x30

[76313.774427]  ? __pfx_iomap_write_begin+0x10/0x10
[76313.776100]  ? fault_in_readable+0x80/0xe0
[76313.777476]  ? __pfx_fault_in_readable+0x10/0x10
[76313.779106]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.780765]  ? balance_dirty_pages_ratelimited_flags+0x549/0xcb0
[76313.782861]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.784457]  ? fault_in_iov_iter_readable+0xe5/0x250
[76313.786221]  iomap_file_writethrough_write+0x9fd/0x1ce0

[76313.787978]  ? __pfx_iomap_file_writethrough_write+0x10/0x10
[76313.789991]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.791589]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.793314]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.794932]  ? current_time+0x73/0x2b0
[76313.796132]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.797276]  ? xfs_file_write_checks+0x420/0x900 [xfs]
[76313.798786]  xfs_file_buffered_write+0x195/0xae0 [xfs]
[76313.800243]  ? __pfx_xfs_file_buffered_write+0x10/0x10 [xfs]
[76313.801775]  ? kasan_save_track+0x14/0x40
[76313.802843]  ? kasan_save_free_info+0x3b/0x70
[76313.803908]  ? __kasan_slab_free+0x4f/0x80
[76313.804894]  ? vfs_fstatat+0x55/0xa0
[76313.805835]  ? __do_sys_newfstatat+0x7b/0xe0
[76313.806899]  ? do_syscall_64+0x5b/0x540
[76313.807829]  ? srso_alias_return_thunk+0x5/0xfbef5
[76313.809052]  ? xfs_file_write_iter+0x22e/0xa80 [xfs]
[76313.810451]  do_iter_readv_writev+0x453/0xa70

I have a feeling this has to do with the error handling as we are stuck waiting for writeback to complete. It is not reproducible because it might be dependent on the state of the system before this triggers. Let me see if I can find a way to reliably reproduce this so that we have something to verify against once we make these changes.

--
Pankaj

The implementation of write-through combines the buffered IO frontend with dio backend, which leads to some interesting interactions. I've added most of the design notes in respective patches. Please note that this is an initial RFC to iron out any early design issues. This is largely based on suggestions from Dave an Jan in [1] so thanks for the pointers!

* Testing Notes (UPDATED) *

- I've added support for RWF_WRITETHROUGH to fsx and fsstress in xfstests and these patches survive fsx with integrity verification as well as fsstress parallel stressing.

- -g quick with blocks size == page size and blocksize < pagesize shows no new regressions.

* Design TODOs (UPDATED) *

- Evaluate if we need to tag page cache dirty bit in xarray, since PG_Writeback is already set on the folio.
- Look into a better way to refactor writethrough path by reusing common parts of dio code.

Thanks for the patches.

Quoted 10 lines

The implementation of write-through combines the buffered IO frontend with dio backend, which leads to some interesting interactions. I've added most of the design notes in respective patches. Please note that this is an initial RFC to iron out any early design issues. This is largely based on suggestions from Dave an Jan in [1] so thanks for the pointers!

* Testing Notes (UPDATED) *

Quoted 3 lines

- I've added support for RWF_WRITETHROUGH to fsx and fsstress in xfstests and these patches survive fsx with integrity verification as well as fsstress parallel stressing.

Could you point me to your xfstests repo where you have added this support? :)
I have been traveling the whole week. I can give the patches a spin in my setup in the coming days.

Quoted 12 lines

- -g quick with blocks size == page size and blocksize < pagesize shows no new regressions.

* Design TODOs (UPDATED) *

- Evaluate if we need to tag page cache dirty bit in xarray, since PG_Writeback is already set on the folio.
- Look into a better way to refactor writethrough path by reusing common parts of dio code.

* Testing Notes (UPDATED) *

- I've added support for RWF_WRITETHROUGH to fsx and fsstress in xfstests and these patches survive fsx with integrity verification as well as fsstress parallel stressing.
- -g quick with blocks size == page size and blocksize < pagesize shows no new regressions.

Quoted 9 lines

* Testing Notes (UPDATED) *

- I've added support for RWF_WRITETHROUGH to fsx and fsstress in xfstests and these patches survive fsx with integrity verification as well as fsstress parallel stressing.
- -g quick with blocks size == page size and blocksize < pagesize shows no new regressions.

I am hitting a very strange error in generic/127. I deconstructed it in to two commands that can reproduce the issue:

```
$ dd if=/dev/zero of=/media/test/fsx_lite_nommap bs=262144 count=1

$ /root/home/xfstests/ltp/fsx -l 262144 -o 65536 -S 191110531 -N 100000 -L -R -W /media/test/fsx_lite_nommap
mapped writes DISABLED
Seed set to 191110531
main: filesystem does not support exchange range, disabling!
main: atomic writes need O_DIRECT (-Z), disabling!
short read: 0x0 bytes instead of 0x6c7f
LOG DUMP (1 total operations):
1(  1 mod 256): READ     0x39381 thru 0x3ffff   (0x6c7f bytes)
Log of operations saved to "/media/test/fsx_lite_nommap.fsxops"; replay with --replay-ops
fsx: save_buffer: .fsxgood file too short... will save 0x0 bytes instead of 0x40000
: Operation not supported
Correct content saved for comparison
(maybe hexdump "/media/test/fsx_lite_nommap" vs "/media/test/fsx_lite_nommap.fsxgood")
```

This is on a bs == ps (4k block size) xfs filesystem.

```
$ xfs_info /media/test/
meta-data=/dev/nvme1n1           isize=512    agcount=4, agsize=2097152 blks
         =                       sectsz=4096  attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=1 inobtcount=1 nrext64=0
data     =                       bsize=4096   blocks=8388608, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=4096  sunit=1 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
```

The same test passes if I pass -G to fsx parameters(disabling writethrough).

The error is coming when we do a read operation which feels very strange. Could you run this in your setup and see if you can reproduce them? I still do not know where the issue is coming from or it is because of my test setup.

--
Pankaj

Quoted 46 lines · Pankaj Raghav (Samsung)

Quoted 9 lines

* Testing Notes (UPDATED) *

- I've added support for RWF_WRITETHROUGH to fsx and fsstress in xfstests and these patches survive fsx with integrity verification as well as fsstress parallel stressing.
- -g quick with blocks size == page size and blocksize < pagesize shows no new regressions.

I am hitting a very strange error in generic/127. I deconstructed it in to two commands that can reproduce the issue:

```
$ dd if=/dev/zero of=/media/test/fsx_lite_nommap bs=262144 count=1

$ /root/home/xfstests/ltp/fsx -l 262144 -o 65536 -S 191110531 -N 100000 -L -R -W /media/test/fsx_lite_nommap
mapped writes DISABLED
Seed set to 191110531
main: filesystem does not support exchange range, disabling!
main: atomic writes need O_DIRECT (-Z), disabling!
short read: 0x0 bytes instead of 0x6c7f
LOG DUMP (1 total operations):
1(  1 mod 256): READ     0x39381 thru 0x3ffff   (0x6c7f bytes)
Log of operations saved to "/media/test/fsx_lite_nommap.fsxops"; replay with --replay-ops
fsx: save_buffer: .fsxgood file too short... will save 0x0 bytes instead of 0x40000
: Operation not supported
Correct content saved for comparison
(maybe hexdump "/media/test/fsx_lite_nommap" vs "/media/test/fsx_lite_nommap.fsxgood")
```

This is on a bs == ps (4k block size) xfs filesystem.

```
$ xfs_info /media/test/
meta-data=/dev/nvme1n1           isize=512    agcount=4, agsize=2097152 blks
         =                       sectsz=4096  attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=1 inobtcount=1 nrext64=0
data     =                       bsize=4096   blocks=8388608, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=4096  sunit=1 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
```

Quoted 6 lines

The same test passes if I pass -G to fsx parameters(disabling writethrough).

The error is coming when we do a read operation which feels very strange. Could you run this in your setup and see if you can reproduce them? I still do not know where the issue is coming from or it is because of my test setup.

Hey Pankaj, I think the xfstests branch I shared might have been missing one small fix that can cause this issue. I've update the branch with the fix, can you give it a try:

https://github.com/OjaswinM/xfstests/tree/iomap-buf-writethrough2

Regards,
Ojaswin

Quoted 3 lines

--
Pankaj

Ohk, just noticed your reply, after sending a response...

Thinking it again, I agree we should just always ftruncate it back to the original file_size. Because we might have changed to file size while testing the writethrough support.

Thanks for fixing it! This fix looks right to me.

~# git diff

ltp/fsx.c +0 -0

index c9901f65..720bca83 100644

Quoted 9 lines

* Testing Notes (UPDATED) *

- I've added support for RWF_WRITETHROUGH to fsx and fsstress in xfstests and these patches survive fsx with integrity verification as well as fsstress parallel stressing.
- -g quick with blocks size == page size and blocksize < pagesize shows no new regressions.

I am hitting a very strange error in generic/127. I deconstructed it in to two commands that can reproduce the issue:

```
$ dd if=/dev/zero of=/media/test/fsx_lite_nommap bs=262144 count=1

$ /root/home/xfstests/ltp/fsx -l 262144 -o 65536 -S 191110531 -N 100000 -L -R -W /media/test/fsx_lite_nommap
mapped writes DISABLED
Seed set to 191110531
main: filesystem does not support exchange range, disabling!
main: atomic writes need O_DIRECT (-Z), disabling!
short read: 0x0 bytes instead of 0x6c7f

LOG DUMP (1 total operations):
1(  1 mod 256): READ     0x39381 thru 0x3ffff   (0x6c7f bytes)
Log of operations saved to "/media/test/fsx_lite_nommap.fsxops"; replay with --replay-ops
fsx: save_buffer: .fsxgood file too short... will save 0x0 bytes instead of 0x40000
: Operation not supported
Correct content saved for comparison
(maybe hexdump "/media/test/fsx_lite_nommap" vs "/media/test/fsx_lite_nommap.fsxgood")
```

This is on a bs == ps (4k block size) xfs filesystem.

```
$ xfs_info /media/test/
meta-data=/dev/nvme1n1           isize=512    agcount=4, agsize=2097152 blks
         =                       sectsz=4096  attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=1 inobtcount=1 nrext64=0
data     =                       bsize=4096   blocks=8388608, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=4096  sunit=1 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
```

The same test passes if I pass -G to fsx parameters(disabling writethrough).

The error is coming when we do a read operation which feels very strange. Could you run this in your setup and see if you can reproduce them? I still do not know where the issue is coming from or it is because of my test setup.

--
Pankaj

Hi all,

This is the v2 RFC to add buffered writethrough support to iomap and xfs. The changes made are mostly to get the writethrough implementation more inline with how dio handles writes.

Quoted 7 lines · Ojaswin Mujoo

Hi all,

This is the v2 RFC to add buffered writethrough support to iomap and xfs. The changes made are mostly to get the writethrough implementation more inline with how dio handles writes.

I gave these series a run with kdevops and fstests using the following
profiles: xfs-crc-logdev, xfs-crc, xfs-reflink-logdev, xfs-reflink.

Note that I applied all patches on top of v7.1-rc1 and xfstest-dev patches from Ojaswin have been rebased on latest master [1].

Link:
https://github.com/dagomez137/xfstests-dev/tree/iomap-buf-writethrough2 [1].

In general, some of the results here seem flaky. But let me know what you think. I'll try to confirm the possible regressions with another full run.

Summary per profile:

* xfs-crc-logdev: no regressions or fixes found
* xfs-crc:
* generic/753 fixed. This test failed on 7.1.0-rc1 run but succeeded
on 7.1.0-rc1-00005-gc6a39600d3c3.
* xfs-reflink-logdev:
* generic/415 failed on 7.1.0-rc1-00005-gc6a39600d3c3. I tried to
replicate the issue by just running generic/415 (not the entire test suite), and I couldn't replicate it.
* xfs-reflink:
* generic/457 failed on 7.1.0-rc1-00005-gc6a39600d3c3.
* generic/561 failed on 7.1.0-rc1 but not on
7.1.0-rc1-00005-gc6a39600d3c3.

Here the full report list for each A/B profile test run:

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-crc-logdev.html

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-crc.html

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-reflink-logdev.html

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-reflink.html

Quoted 25 lines · Daniel Gomez

Quoted 7 lines · Ojaswin Mujoo

Hi all,

This is the v2 RFC to add buffered writethrough support to iomap and xfs. The changes made are mostly to get the writethrough implementation more inline with how dio handles writes.

I gave these series a run with kdevops and fstests using the following
profiles: xfs-crc-logdev, xfs-crc, xfs-reflink-logdev, xfs-reflink.

Note that I applied all patches on top of v7.1-rc1 and xfstest-dev patches from Ojaswin have been rebased on latest master [1].

Link:
https://github.com/dagomez137/xfstests-dev/tree/iomap-buf-writethrough2 [1].

In general, some of the results here seem flaky. But let me know what you think. I'll try to confirm the possible regressions with another full run.

Summary per profile:

Quoted 12 lines

* xfs-crc-logdev: no regressions or fixes found
* xfs-crc:
* generic/753 fixed. This test failed on 7.1.0-rc1 run but succeeded
on 7.1.0-rc1-00005-gc6a39600d3c3.
* xfs-reflink-logdev:
* generic/415 failed on 7.1.0-rc1-00005-gc6a39600d3c3. I tried to
replicate the issue by just running generic/415 (not the entire test suite), and I couldn't replicate it.
* xfs-reflink:
* generic/457 failed on 7.1.0-rc1-00005-gc6a39600d3c3.
* generic/561 failed on 7.1.0-rc1 but not on
7.1.0-rc1-00005-gc6a39600d3c3.

Hey Daniel,

thanks so much for testing the patches. I'm back this week from travel so still catching up to my emails. I'll go trhough thi and get back to you soon.

Regards,
Ojaswin

Quoted 17 lines

Here the full report list for each A/B profile test run:

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-crc-logdev.html

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-crc.html

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-reflink-logdev.html

http://htmlpreview.github.io/?https://github.com/dagomez137/kdevops-results-archive-wt/blob/main/workflows/fstests/results/r0001/fstests-report-7.1.0-rc1-vs-7.1.0-rc1-00005-gc6a39600d3c3-r0001-qsu-xfs-reflink.html

So this week I spent some time looking into the failure of g/457. I was able to replicate it in my setup and after analysing it, this is actually an issue in my xfstests patch. The issue is that to verify RWF_WRITETHROUGH support I do a 4kb write on the fd during the test prep phase. However "out-of-band" write makes the actual contents of the file different from what fsx expected (It reads it in before the problematic write), causing the test failure.

Quoted 8 lines · Ojaswin Mujoo

So this week I spent some time looking into the failure of g/457. I was able to replicate it in my setup and after analysing it, this is actually an issue in my xfstests patch. The issue is that to verify RWF_WRITETHROUGH support I do a 4kb write on the fd during the test prep phase. However "out-of-band" write makes the actual contents of the file different from what fsx expected (It reads it in before the problematic write), causing the test failure.

Why does this get out of sync? One of the arguments for RWF_WRITETHROUGH vs direct I/O was that it avoids the content getting out of sync.

Quoted 2 lines · Ojaswin Mujoo

Quoted 1 line · Daniel Gomez

On 08/04/2026 20.45, Ojaswin Mujoo wrote:

ltp/fsx.c +15 -0

index 720bca83..1c76634b 100644
@@ -2077,16 +2077,34 @@ test_dontcache_io(void)
        return 1;
 }
 
+/*
+ * Use a temporary file to test RWF_WRITETHROUGH so as to not modify the
+ * original. This is so that fsx expected contents and the actual contents
+ * of the original file don't go out of sync
+ */
 int
 test_writethrough_io(void)
 {
        char buf[4096];
+       char tmpname[PATH_MAX];
+       int tmp_fd;
        struct iovec iov = { .iov_base = buf, .iov_len = sizeof(buf) };
        int ret, e;
 
+       snprintf(tmpname, sizeof(tmpname), "%s.fsx.XXXXXX", fname);
+
+       tmp_fd = mkstemp(tmpname);
+       if (fd < 0) {
+               perror("test_writethrough_io: mkstemp() failed\n");
+               exit(132);
+       }
+

Im unable to replicate the 415 failure, but this one is strange because it doesn't involve any writethrough IO. In you previous runs have you seen this be flaky?

Admittedly, I've not yet started thorough testing since this is in early RFC and the design is still getting finalized but I appriciate the help in testing this.

Quoted 34 lines · Ojaswin Mujoo

Quoted 2 lines · Ojaswin Mujoo

Quoted 1 line · Daniel Gomez

On 08/04/2026 20.45, Ojaswin Mujoo wrote:

ltp/fsx.c +15 -0

index 720bca83..1c76634b 100644
@@ -2077,16 +2077,34 @@ test_dontcache_io(void)
        return 1;
 }
 
+/*
+ * Use a temporary file to test RWF_WRITETHROUGH so as to not modify the
+ * original. This is so that fsx expected contents and the actual contents
+ * of the original file don't go out of sync
+ */
 int
 test_writethrough_io(void)
 {
        char buf[4096];
+       char tmpname[PATH_MAX];
+       int tmp_fd;
        struct iovec iov = { .iov_base = buf, .iov_len = sizeof(buf) };
        int ret, e;
 
+       snprintf(tmpname, sizeof(tmpname), "%s.fsx.XXXXXX", fname);
+
+       tmp_fd = mkstemp(tmpname);
+       if (fd < 0) {
+               perror("test_writethrough_io: mkstemp() failed\n");
+               exit(132);
+       }
+

ltp/fsx.c test_dontcache_io(void) · line 2110

Quoted 10 lines

ltp/fsx.c +4 -1

        memset(buf, '\0', sizeof(buf));
-       ret = pwritev2(fd, &iov, 1, 0, RWF_WRITETHROUGH);
+       ret = pwritev2(tmp_fd, &iov, 1, 0, RWF_WRITETHROUGH);
        e = ret < 0 ? errno : 0;
+
+       close(tmp_fd);
+
        if (e == EOPNOTSUPP) {
                if (!quiet)
                        fprintf(stderr,

I think this is missing:

(quoted diff) +0 -6

@@ -2158,12 +2175,6 @@ test_writethrough_io(void)
                return 0;
        }
 
-       if (ftruncate(fd, file_size) != 0) {
-               perror("test_writethrough_io: failed to ftruncate to 0\n");
-               exit(132);
-       }
-
-
 

The fix works for me after a stress loop test on generic/457.

Quoted 5 lines

Im unable to replicate the 415 failure, but this one is strange because it doesn't involve any writethrough IO. In you previous runs have you seen this be flaky?

I couldn't confirm back then, so I think we can ignore it. Also, unrelated.

Quoted 5 lines

Admittedly, I've not yet started thorough testing since this is in early RFC and the design is still getting finalized but I appriciate the help in testing this.

No problem. IMHO, testing makes a design more robust, once the path is agreed on. That said, do you think the path needs changing? Reading LWN and the patch series threads, I don't see any blocker. You can cc me for the next iteration and I'll follow up with more testing.