[v2] iomap: some minor non-critical fixes and improvements when block size < folio size

[PATCH v2 6/6] iomap: reduce unnecessary state_lock when setting ifs uptodate and dirty bits

Posted by Zhang Yi 1 year, 4 months ago

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>
---
 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

Re: [PATCH v2 6/6] iomap: reduce unnecessary state_lock when setting ifs uptodate and dirty bits

Posted by Matthew Wilcox 1 year, 4 months ago

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);

Re: [PATCH v2 6/6] iomap: reduce unnecessary state_lock when setting ifs uptodate and dirty bits

Posted by Zhang Yi 1 year, 4 months ago

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.

Re: [PATCH v2 6/6] iomap: reduce unnecessary state_lock when setting ifs uptodate and dirty bits

Posted by Darrick J. Wong 1 year, 4 months ago

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
> 
>