Re: [Question] About XFS random buffer write performance

From: Matthew Wilcox <willy@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>,
	Zhengyuan Liu <liuzhengyuang521@gmail.com>,
	linux-xfs@vger.kernel.org,
	Zhengyuan Liu <liuzhengyuan@kylinos.cn>,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [Question] About XFS random buffer write performance
Date: Wed, 29 Jul 2020 03:12:31 +0100	[thread overview]
Message-ID: <20200729021231.GV23808@casper.infradead.org> (raw)
In-Reply-To: <20200729015458.GY2005@dread.disaster.area>

On Wed, Jul 29, 2020 at 11:54:58AM +1000, Dave Chinner wrote:
> On Tue, Jul 28, 2020 at 04:47:53PM +0100, Matthew Wilcox wrote:
> > I propose we do away with the 'uptodate' bit-array and replace it with an
> > 'writeback' bit-array.  We set the page uptodate bit whenever the reads to
> 
> That's just per-block dirty state tracking. But when we set a single
> bit, we still need to set the page dirty flag.

It's not exactly dirty, though.  It's 'present' (ie the opposite
of hole).  I'm not attached to the name.  So it can be used to
implement iomap_is_partially_uptodate.  If the page is dirty, the chunks
corresponding to the present bits get written back, but we don't track
a per-block dirty state.

> > fill the page have completed rather than checking the 'writeback' array.
> > In page_mkwrite, we fill the writeback bit-array on the grounds that we
> > have no way to track a block's non-dirtiness and we don't want to scan
> > each block at writeback time to see if it's been written to.
> 
> You're talking about mmap() access to the file here, not
> read/write() syscall access. If page_mkwrite() sets all the
> blocks in a page as "needing writeback", how is that different in
> any way to just using a single dirty bit? So why wouldn't we just do
> this in iomap_set_page_dirty()?

iomap_set_page_dirty() is called from iomap_page_mkwrite_actor(), so
sure!

> The only place we wouldn't want to set the entire page dirty is
> the call from __iomap_write_end() which knows the exact range of the
> page that was dirtied. In which case, iomap_set_page_dirty_range()
> would be appropriate, right? i.e. we still have to do all the same
> page/page cache/inode dirtying, but only that would set a sub-page
> range of dirty bits in the iomap_page?
> 
> /me doesn't see the point of calling dirty tracking "writeback bits"
> when "writeback" is a specific page state that comes between the
> "dirty" and "clean" states...

I don't want to get it confused with page states.  This is a different
thing.  It's just tracking which blocks are holes (and have definitely
not been written to), so those blocks can remain as holes when the page
gets written back.