Re: [PATCH 1/4] mm: fix IOCB_DONTCACHE write performance with rate-limited writeback

[IBM]

Jeff Layton <jlayton@xxxxxxxxxx> writes:

> IOCB_DONTCACHE calls filemap_flush_range() with nr_to_write=LONG_MAX
> on every write, which flushes all dirty pages in the written range.
> Under concurrent writers this creates severe serialization on the
> writeback submission path, causing throughput to collapse to ~47% of
> buffered I/O with multi-second tail latency.

Yes, between concurrent writers, I agree with the theory.


> Even single-client
> sequential writes suffer: on a 512GB file with 256GB RAM, the
> aggressive flushing triggers dirty throttling that limits throughput
> to 575 MB/s vs 1442 MB/s with rate-limited writeback.

I am not sure if this 2.5x performance penalty in a "single" sequential
writer is due to throttling logic. On giving it some thoughts, I suspect
if this is because, the submission side and the completion side both
takes the xa_lock and hence could be contending on that.

For e.g. since this patch skips doing the flush the second time, (note
that writeback is active when the same writer dirtied the page during
previous write), this allows the writer to do more work of writing data
to page cache pages, instead of waiting on the xa_lock which the
completion callback could be holding (folio_end_writeback() -> folio_end_dropbehind())

If I see Peak Dirty data from the link you shared [1] in single writer case...

Mode                    MB/s	p50 (ms)	p99 (ms)	p99.9 (ms)	Peak Dirty	Peak Cache
dontcache (unpatched)	1179	3.2	    103.3	    170.9	    14 MB	    4.7 GB
dontcache (patched)	1453	5.4	    43.8	    57.4	    36 GB	    45 GB

... this too shows that the submission side is writing more dirty pages,
then the completion side able to write it... 

I suspect this contention (between submission and completion) could more
in IOCB_DONTCACHE case, since the completion side also removes the folio
from the page cache within the same xa_lock, which is not the same with
normal buffered writes.

Maybe a perf callgraph showing the contention would be nicer thing to add
here [1] ;). 

[1]: https://markdownpastebin.com/?id=96249deb897a401ba32acbce05312dcc

>
> Replace the filemap_flush_range() call in generic_write_sync() with a
> new filemap_dontcache_writeback_range() that uses two rate-limiting
> mechanisms:
>
>   1. Skip-if-busy: check mapping_tagged(PAGECACHE_TAG_WRITEBACK)
>      before flushing.  If writeback is already in progress on the
>      mapping, skip the flush entirely.  This eliminates writeback
>      submission contention between concurrent writers.
>
>   2. Proportional cap: when flushing does occur, cap nr_to_write to
>      the number of pages just written.  This prevents any single
>      write from triggering a large flush that would starve concurrent
>      readers.
>
> Both mechanisms are necessary: skip-if-busy alone causes I/O bursts
> when the tag clears (reader p99.9 spikes 83x); proportional cap alone
> still serializes on xarray locks regardless of submission size.
>
> Pages touched under IOCB_DONTCACHE continue to be marked for eviction
> (dropbehind), so page cache usage remains bounded.  Ranges skipped by
> the busy check are eventually flushed by background writeback or by
> the next writer to find the tag clear.

Yes, but the next writer may not write the dirty pages, of the previous
writer which skipped the flush call right (even if it finds the tag
clear)? Because filemap_dontcache_writeback_range( ) passes the range
and nr_to_write that means, unless the previous writer dirtied the same
range, the new writer won't be able to write the dirty pages of the
previous writer correct? So, it is mainly only the background writeback
now, which will flush this dirty pages of the writer which skipped the
flush (unless of course a fsync/sync call is made).

But having said that, I agree, this patch series is a nice performance
improvement overall :)

>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
>  include/linux/fs.h |  7 +++++--
>  mm/filemap.c       | 29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 8b3dd145b25ec12b00ac1df17a952d9116b88047..53e9cca1b50a946a1276c49902294c3ae0ab3500 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2610,6 +2610,8 @@ extern int __must_check file_write_and_wait_range(struct file *file,
>  						loff_t start, loff_t end);
>  int filemap_flush_range(struct address_space *mapping, loff_t start,
>  		loff_t end);
> +int filemap_dontcache_writeback_range(struct address_space *mapping,
> +		loff_t start, loff_t end, ssize_t nr_written);
>  
>  static inline int file_write_and_wait(struct file *file)
>  {
> @@ -2645,8 +2647,9 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
>  	} else if (iocb->ki_flags & IOCB_DONTCACHE) {
>  		struct address_space *mapping = iocb->ki_filp->f_mapping;
>  
> -		filemap_flush_range(mapping, iocb->ki_pos - count,
> -				iocb->ki_pos - 1);
> +		filemap_dontcache_writeback_range(mapping,
> +				iocb->ki_pos - count,
> +				iocb->ki_pos - 1, count);
>  	}
>  
>  	return count;
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 406cef06b684a84a1e0c27d8267e95f32282ffdc..af2024b736bef74571cc22ab7e3cde2c8e872efe 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -437,6 +437,35 @@ int filemap_flush_range(struct address_space *mapping, loff_t start,
>  }
>  EXPORT_SYMBOL_GPL(filemap_flush_range);
>  
> +/**
> + * filemap_dontcache_writeback_range - rate-limited writeback for dontcache I/O
> + * @mapping:	target address_space
> + * @start:	byte offset to start writeback
> + * @end:	last byte offset (inclusive) for writeback
> + * @nr_written:	number of bytes just written by the caller
> + *
> + * Rate-limited writeback for IOCB_DONTCACHE writes.  Skips the flush
> + * entirely if writeback is already in progress on the mapping (skip-if-busy),
> + * and when flushing, caps nr_to_write to the number of pages just written
> + * (proportional cap).  Together these avoid writeback contention between
> + * concurrent writers and prevent I/O bursts that starve readers.
> + *
> + * Return: %0 on success, negative error code otherwise.
> + */
> +int filemap_dontcache_writeback_range(struct address_space *mapping,
> +		loff_t start, loff_t end, ssize_t nr_written)
> +{
> +	long nr;
> +
> +	if (mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
> +		return 0;
> +
> +	nr = (nr_written + PAGE_SIZE - 1) >> PAGE_SHIFT;
> +	return filemap_writeback(mapping, start, end, WB_SYNC_NONE, &nr,
> +			WB_REASON_BACKGROUND);

Was this rebased against some other tree? I couldn't find it in
linux-next. I think, that last argument is wrong. 

> +}
> +EXPORT_SYMBOL_GPL(filemap_dontcache_writeback_range);
> +
>  /**
>   * filemap_flush - mostly a non-blocking flush
>   * @mapping:	target address_space
>
> -- 
> 2.53.0