Re: [RFC PATCH v4 05/12] iomap: Add initial support for buffered RWF_WRITETHROUGH

[Ojaswin Mujoo]

On Wed, Oct 07, 2026 at 11:32:36AM +0200, Pankaj Raghav (Samsung) wrote:
> One general comment, all your commit headers are starting with
> uppercase.
> 
> iomap: Add initial support for buffered RWF_WRITETHROUGH
> 
> s/Add/add

Hi Pankaj, thanks for the review. Sure I'll take care of this.
> 
> Overall this looks good to me. I will run some tests via fio. Do you
> have a branch with WRITETHROUGH by any chance?

yes, I've this branch: 
https://github.com/OjaswinM/xfstests/tree/iomap-buf-writethrough2

The patches are a bit rough but it has writethrough support for fsx and
fsstress so we can stress the feature.

> 
> On Mon, Sep 28, 2026 at 05:33:06PM +0530, Ojaswin Mujoo wrote:
> > +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, pending = 0;
> > +	loff_t submit_start_pos;
> > +	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 how 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;
> > +
> > +	/*
> > +	 * IOMAP_INLINE mappings have NULL bdev and would cause
> > +	 * iomap_sector() to dereference invalid memory. Reject them.
> > +	 */
> 
> Nit:
> I noticed that gfs2 sets the bdev even for IOMAP_INLINE. A better
> filesystem agnostic comment might be something like this?
> 
> /*
>  * Inline data lives in the inode's metadata buffer, so it cannot be
>  * written via a bio built from the folio.
>  */

Yes that sounds better, thanks.
> 
> > +	if (iter->iomap.type == IOMAP_INLINE)
> > +		return -EINVAL;
> > +
> > +	do {
> > +		 * blocks in the bvec again.
> > +		 */
> > +		if (wt_ctx->nr_bvecs && prev_pos + prev_len > pos_aligned) {
> > +			size_t delta = prev_pos + prev_len - pos_aligned;
> > +
> > +			/* Everything already added to bvec, nothing to do */
> > +			if (delta >= len_aligned)
> > +				goto put_folio;
> > +
> > +			pos_aligned += delta;
> > +			off_aligned += delta;
> > +			len_aligned -= delta;
> > +		}
> > +
> > +		prev_pos = off_aligned;
> 
> I think this is a mistake?
> prev_pos = pos_aligned; ?

oh right, I guess I didn't hit this in my testing. Thanks, I'll fix it.
> 
> > +		prev_len = len_aligned;
> > +
> > +		iomap_folio_prepare_writethrough(folio, off_aligned,
> > +						 len_aligned);
> > +
> > +		if (!wt_ctx->nr_bvecs) {
> > +			wt_ctx->bio_pos = round_down(pos, bs);
> 
> We could reuse pos_aligned variable here instead of recalculating?

Yes, I will fix it.
> 
> > +			submit_start_pos = pos;
> > +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;
> > +	struct blk_plug plug;
> > +	size_t min_folio_bytes = PAGE_SIZE
> > +				 << mapping_min_folio_order(inode->i_mapping);
> 
> min_folio_nr_bytes could be used here.

Noted.

> > +
> > +	/*
> > +	 * For now we don't support any other flag with WRITETHROUGH
> > +	 */
> > +	if (!(iocb->ki_flags & IOCB_WRITETHROUGH))
> > +		return -EINVAL;
> > +	if (iocb->ki_flags & (IOCB_DONTCACHE))
> > +		return -EINVAL;
> > +	if (iocb_is_dsync(iocb))
> > +		/* D_SYNC support not implemented yet */
> > +		return -EOPNOTSUPP;
> > +	if (!is_sync_kiocb(iocb))
> > +		/* aio support not implemented yet */
> > +		return -EOPNOTSUPP;
> > +
> 
> -- 
> Pankaj