Re: [RFC PATCH v3 04/11] iomap: Add initial support for buffered RWF_WRITETHROUGH

[Ojaswin Mujoo]

On Mon, Aug 17, 2026 at 03:20:12PM +0200, Pankaj Raghav (Samsung) wrote:
> I have added some minor comments. I haven't checked Sashiko's review, so
> ignore them if it is a repeat.

Hi Pankaj,

Thanks for the review!
> 
> > +
> > +	if (!wt_ctx->nr_bvecs)
> > +		goto exit;
> > +
> > +	for (i = 0; i < wt_ctx->nr_bvecs; i++)
> > +		len += wt_ctx->bvec[i].bv_len;
> > +
> > +	bio = bio_alloc(iomap->bdev, wt_ctx->nr_bvecs, opf, GFP_NOFS);
> > +	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);
> 
> bio_add_folio_nofail()?

Hmm actually your suggestion is right but it also got me thinking about
if we are actually ensuring that bio_add_folio_nofail() will never face
any issues.

We make sure that we don't let the bvecs grow more than BIO_MAX_VECS ie
we can at max have 256 pages in the bio, however on higher page size
systems will a larger large page size, i think we can actually cross the
BIO_MAX_SIZE limit causing the bi_size to wrap around.

I think in addition to adding the helper, we should also add a check to
make sure we are not crossing that limit. I'll look into this a bit
more.
> 
> > +
> > +	if (!error && wt_ops->writethrough_submit)
> > +		error = wt_ops->writethrough_submit(wt_ctx->inode, iomap,
> > +						    wt_ctx->bio_pos, len);
> > +
> > +
> > +	atomic_inc(&wt_ctx->ref);
> > +
> > +	/*
> > +	 * In case of error we still need the I/O completion to run so we can
> > +	 * release references and end writeback on the folios.
> > +	 */
> > +	if (error) {
> > +		bio->bi_status = errno_to_blk_status(error);
> > +		bio_endio(bio);
> > +		return error;
> > +	}
> > +
> > +	submit_bio(bio);
> > +	wt_ctx->nr_bvecs = 0;
> > +
> > +exit:
> > +	return 0;
> <snip>
> > +
> > +		if (!wt_ctx->nr_bvecs) {
> > +			wt_ctx->bio_pos = round_down(pos, bs);
> > +			submit_start_pos = pos;
> > +		}
> > +
> > +		bvec_set_folio(&wt_ctx->bvec[wt_ctx->nr_bvecs], folio,
> > +			       len_aligned, off_aligned);
> > +		wt_ctx->nr_bvecs++;
> > +
> > +put_folio:
> > +		__iomap_put_folio(iter, wt_ops->write_ops, written, folio);
> > +
> > +		if (old_size < pos)
> > +			pagecache_isize_extended(iter->inode, old_size, pos);
> 
> Some new code has been added upstream to include VERITY. Probably the
> same thing needs to be done here?
> 
> if (pos + written > old_size &&
>      !(iter->iomap.flags & IOMAP_F_FSVERITY)) {
>          i_size_write(iter->inode, pos + written);
>          iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
>  }
>  __iomap_put_folio(iter, write_ops, written, folio);
>  
>  if (old_size < pos && !(iter->iomap.flags & IOMAP_F_FSVERITY))
>          pagecache_isize_extended(iter->inode, old_size, pos);
> 

Oh right, I'll take care of it, thanks for pointing it out.

> > +
> > +		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;
> > +			pending += written;
> > +			iomap_iter_advance(iter, written);
> > +		}
> > +
> > +		/*
> > +		 * If we fail to submit the bio, we immediately call the
> > +		 * IO completion handler that records the error. We
> > +		 * shall not retry anymore cause this could lead to
> > +		 * infinite loops in case of non-transient errors.
> > +		 */
> > +		if (wt_ctx->nr_bvecs == wt_ctx->max_bvecs) {
> > +			status = iomap_writethrough_try_submit(wt_ctx,
> > +					&iter->iomap, wt_ops, &pending);
> > +			if (status)
> > +				goto submit_failed;
> > +		}
> > +
> > +	} while (iov_iter_count(i) && iomap_length(iter));
> > +
> > +	if (wt_ctx->nr_bvecs) {
> > +		status = iomap_writethrough_try_submit(wt_ctx,
> > +				&iter->iomap, wt_ops, &pending);
> > +		if (status)
> > +			goto submit_failed;
> > +	}
> > +
> > +	/*
> > +	 * In case of an error, we only consider the bytes we were actually able
> > +	 * to submit IO for as valid data and revert the iters accordingly
> > +	 */
> > +	if (status) {
> > +		/*
> > +		 * we still need to run the endio completion for cleanup work
> > +		 * hence call the below helper to take care of it, if we haven't
> > +		 * already done so. We can ignore the return value here.
> > +		 */
> > +		iomap_writethrough_submit_bio(wt_ctx, &iter->iomap, wt_ops, status);
> 
> Do we even need to call iomap_writethrough_submit_bio() here? 
> 
> In all the failure scenario we reach submit_failed directly right? In
> the cases where we fail before copy_folio_from_iter_atomic(), we will
> not have anything to do anyway, therefore, making the above call a no
> op? Am I missing something?

Actually if we break of of the loop due to some error and nr_bvecs
is empty then I added this call so that we could handle the cleanup and
even if there are not bvecs to submit, it is still responsible for
calling bio_endio to set wt_ctx->error = status, so that later we can
return it to the user in iomap_writethrough_complete().

But as you said I think we might not need the complete endio flow and
further, I think in this case even wt_ctx->error is set in the higher
layer. I'll take a look at it again, thanks for pointing it out.

> 
> > +
> > +submit_failed:
> > +		iomap_write_failed(iter->inode, submit_start_pos, pending);
> > +		iomap_iter_revert(iter, pending);
> > +		iov_iter_revert(i, pending);
> > +	}
> > +
> > +	return status;
> > +}
> > +
> >  static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
> >  		const struct iomap_write_ops *write_ops)
> >  {
> > @@ -1345,6 +1700,88 @@ int iomap_fsverity_write(struct file *file, loff_t pos, size_t length,
> >  }
> >  EXPORT_SYMBOL_GPL(iomap_fsverity_write);
> >  
> > +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);
> 
> mapping_min_folio_nrbytes can be used here.

Right, I will make the change.
> 
> > +
> > +	/*
> > +	 * For now we don't support any other flag with WRITETHROUGH
> > +	 */
> >  		__folio_cancel_dirty(folio);
> >  }
> >  bool folio_clear_dirty_for_io(struct folio *folio);
> > +bool folio_clear_dirty_for_writethrough(struct folio *folio);
> >  bool clear_page_dirty_for_io(struct page *page);
> >  void folio_invalidate(struct folio *folio, size_t offset, size_t length);
> >  bool noop_dirty_folio(struct address_space *mapping, struct folio *folio);
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index bd87262f2e34..9c8d91b926a7 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -451,10 +451,13 @@ typedef int __bitwise __kernel_rwf_t;
> >  /* prevent pipe and socket writes from raising SIGPIPE */
> >  #define RWF_NOSIGNAL	((__force __kernel_rwf_t)0x00000100)
> >  
> > +/* buffered IO that is asynchronously written through to disk after write */
> 
> Isn't it synchronously written?

Oh right, I think it got left over for an earlier variant. I'll fix this
thanks!

Regards,
ojaswin
> 
> > +#define RWF_WRITETHROUGH	((__force __kernel_rwf_t)0x00000200)
> > +
> >  /* mask of flags supported by the kernel */
> >  #define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\
> >  			 RWF_APPEND | RWF_NOAPPEND | RWF_ATOMIC |\
> > -			 RWF_DONTCACHE | RWF_NOSIGNAL)
> > +			 RWF_DONTCACHE | RWF_NOSIGNAL | RWF_WRITETHROUGH)
> >  
> 
> --
> Pankaj