| Message ID | 20240812121159.3775074-7-yi.zhang@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | iomap: some minor non-critical fixes and improvements when block size < folio size | expand |
On Mon, Aug 12, 2024 at 08:11:59PM +0800, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > When doing buffered write, we set uptodate and drity bits of the written > range separately, it holds the ifs->state_lock twice when blocksize < > folio size, which is redundant. After large folio is supported, the > spinlock could affect more about the performance, merge them could > reduce some unnecessary locking overhead and gets some performance gain. > > Suggested-by: Dave Chinner <david@fromorbit.com> > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Seems reasonable to me Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/iomap/buffered-io.c | 38 +++++++++++++++++++++++++++++++++++--- > 1 file changed, 35 insertions(+), 3 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 96600405dbb5..67d7c1c22c98 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -182,6 +182,37 @@ static void iomap_set_range_dirty(struct folio *folio, size_t off, size_t len) > ifs_set_range_dirty(folio, ifs, off, len); > } > > +static void ifs_set_range_dirty_uptodate(struct folio *folio, > + struct iomap_folio_state *ifs, size_t off, size_t len) > +{ > + struct inode *inode = folio->mapping->host; > + unsigned int blks_per_folio = i_blocks_per_folio(inode, folio); > + unsigned int first_blk = (off >> inode->i_blkbits); > + unsigned int last_blk = (off + len - 1) >> inode->i_blkbits; > + unsigned int nr_blks = last_blk - first_blk + 1; > + unsigned long flags; > + > + spin_lock_irqsave(&ifs->state_lock, flags); > + bitmap_set(ifs->state, first_blk, nr_blks); > + if (ifs_is_fully_uptodate(folio, ifs)) > + folio_mark_uptodate(folio); > + bitmap_set(ifs->state, first_blk + blks_per_folio, nr_blks); > + spin_unlock_irqrestore(&ifs->state_lock, flags); > +} > + > +static void iomap_set_range_dirty_uptodate(struct folio *folio, > + size_t off, size_t len) > +{ > + struct iomap_folio_state *ifs = folio->private; > + > + if (ifs) > + ifs_set_range_dirty_uptodate(folio, ifs, off, len); > + else > + folio_mark_uptodate(folio); > + > + filemap_dirty_folio(folio->mapping, folio); > +} > + > static struct iomap_folio_state *ifs_alloc(struct inode *inode, > struct folio *folio, unsigned int flags) > { > @@ -851,6 +882,8 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, > static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, > size_t copied, struct folio *folio) > { > + size_t from = offset_in_folio(folio, pos); > + > flush_dcache_folio(folio); > > /* > @@ -866,9 +899,8 @@ static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, > */ > if (unlikely(copied < len && !folio_test_uptodate(folio))) > return false; > - iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len); > - iomap_set_range_dirty(folio, offset_in_folio(folio, pos), copied); > - filemap_dirty_folio(inode->i_mapping, folio); > + > + iomap_set_range_dirty_uptodate(folio, from, copied); > return true; > } > > -- > 2.39.2 > >
On Mon, Aug 12, 2024 at 08:11:59PM +0800, Zhang Yi wrote: > @@ -866,9 +899,8 @@ static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, > */ > if (unlikely(copied < len && !folio_test_uptodate(folio))) > return false; > - iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len); > - iomap_set_range_dirty(folio, offset_in_folio(folio, pos), copied); > - filemap_dirty_folio(inode->i_mapping, folio); > + > + iomap_set_range_dirty_uptodate(folio, from, copied); > return true; I wonder how often we overwrite a completely uptodate folio rather than writing new data to a fresh folio? iow, would this be a measurable optimisation? if (folio_test_uptodate(folio)) iomap_set_range_dirty(folio, from, copied); else iomap_set_range_dirty_uptodate(folio, from, copied);
On 2024/8/13 1:00, Matthew Wilcox wrote: > On Mon, Aug 12, 2024 at 08:11:59PM +0800, Zhang Yi wrote: >> @@ -866,9 +899,8 @@ static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, >> */ >> if (unlikely(copied < len && !folio_test_uptodate(folio))) >> return false; >> - iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len); >> - iomap_set_range_dirty(folio, offset_in_folio(folio, pos), copied); >> - filemap_dirty_folio(inode->i_mapping, folio); >> + >> + iomap_set_range_dirty_uptodate(folio, from, copied); >> return true; > > I wonder how often we overwrite a completely uptodate folio rather than > writing new data to a fresh folio? iow, would this be a measurable > optimisation? > > if (folio_test_uptodate(folio)) > iomap_set_range_dirty(folio, from, copied); > else > iomap_set_range_dirty_uptodate(folio, from, copied); > Thanks for the suggestion, I'm not sure how often as well, but I suppose we could do this optimisation since I've tested it and found this is harmless for the case of writing new data to a fresh folio. However, this can further improves the overwrite performance, the UnixBench tests result shows the performance gain can be increased to about ~15% on my machine with 50GB ramdisk and xfs filesystem. UnixBench test cmd: ./Run -i 1 -c 1 fstime-w Base: x86 File Write 1024 bufsize 2000 maxblocks 524708.0 KBps arm64 File Write 1024 bufsize 2000 maxblocks 801965.0 KBps After this series: x86 File Write 1024 bufsize 2000 maxblocks 569218.0 KBps arm64 File Write 1024 bufsize 2000 maxblocks 871605.0 KBps After this measurable optimisation: x86 File Write 1024 bufsize 2000 maxblocks 609620.0 KBps arm64 File Write 1024 bufsize 2000 maxblocks 910534.0 KBps Thanks, Yi.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 96600405dbb5..67d7c1c22c98 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -182,6 +182,37 @@ static void iomap_set_range_dirty(struct folio *folio, size_t off, size_t len) ifs_set_range_dirty(folio, ifs, off, len); } +static void ifs_set_range_dirty_uptodate(struct folio *folio, + struct iomap_folio_state *ifs, size_t off, size_t len) +{ + struct inode *inode = folio->mapping->host; + unsigned int blks_per_folio = i_blocks_per_folio(inode, folio); + unsigned int first_blk = (off >> inode->i_blkbits); + unsigned int last_blk = (off + len - 1) >> inode->i_blkbits; + unsigned int nr_blks = last_blk - first_blk + 1; + unsigned long flags; + + spin_lock_irqsave(&ifs->state_lock, flags); + bitmap_set(ifs->state, first_blk, nr_blks); + if (ifs_is_fully_uptodate(folio, ifs)) + folio_mark_uptodate(folio); + bitmap_set(ifs->state, first_blk + blks_per_folio, nr_blks); + spin_unlock_irqrestore(&ifs->state_lock, flags); +} + +static void iomap_set_range_dirty_uptodate(struct folio *folio, + size_t off, size_t len) +{ + struct iomap_folio_state *ifs = folio->private; + + if (ifs) + ifs_set_range_dirty_uptodate(folio, ifs, off, len); + else + folio_mark_uptodate(folio); + + filemap_dirty_folio(folio->mapping, folio); +} + static struct iomap_folio_state *ifs_alloc(struct inode *inode, struct folio *folio, unsigned int flags) { @@ -851,6 +882,8 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, size_t copied, struct folio *folio) { + size_t from = offset_in_folio(folio, pos); + flush_dcache_folio(folio); /* @@ -866,9 +899,8 @@ static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, */ if (unlikely(copied < len && !folio_test_uptodate(folio))) return false; - iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len); - iomap_set_range_dirty(folio, offset_in_folio(folio, pos), copied); - filemap_dirty_folio(inode->i_mapping, folio); + + iomap_set_range_dirty_uptodate(folio, from, copied); return true; }