[PATCH -next 0/4] fs/buffer: misc optimizations

[PATCH -next 0/4] fs/buffer: misc optimizations

[Davidlohr Bueso]

Hi,

Four small patches - the first could be sent to Linus for v6.15 considering
it is a missing nonblocking lookup conversion in the getblk slowpath I
had missed. The other two patches are small optimizations found while reading
the code, and one rocket science cleanup patch.

Thanks!

Davidlohr Bueso (4):
  fs/buffer: use sleeping lookup in __getblk_slowpath()
  fs/buffer: avoid redundant lookup in getblk slowpath
  fs/buffer: remove superfluous statements
  fs/buffer: optimize discard_buffer()

 fs/buffer.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

--
2.39.5

[PATCH 1/4] fs/buffer: use sleeping lookup in __getblk_slowpath()

[Davidlohr Bueso]

Just as with the fast path, call the lookup variant depending
on the gfp flags.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 fs/buffer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index b8e1e6e325cd..5a4342881f3b 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1122,6 +1122,8 @@ static struct buffer_head *
 __getblk_slow(struct block_device *bdev, sector_t block,
 	     unsigned size, gfp_t gfp)
 {
+	bool blocking = gfpflags_allow_blocking(gfp);
+
 	/* Size must be multiple of hard sectorsize */
 	if (unlikely(size & (bdev_logical_block_size(bdev)-1) ||
 			(size < 512 || size > PAGE_SIZE))) {
@@ -1137,7 +1139,10 @@ __getblk_slow(struct block_device *bdev, sector_t block,
 	for (;;) {
 		struct buffer_head *bh;
 
-		bh = __find_get_block(bdev, block, size);
+		if (blocking)
+			bh = __find_get_block_nonatomic(bdev, block, size);
+		else
+			bh = __find_get_block(bdev, block, size);
 		if (bh)
 			return bh;
 
-- 
2.39.5

[PATCH 2/4] fs/buffer: avoid redundant lookup in getblk slowpath

[Davidlohr Bueso]

__getblk_slow() already implies failing a first lookup
as the fastpath, so try to create the buffers immediately
and avoid the redundant lookup. This saves 5-10% of the
total cost/latency of the slowpath.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 fs/buffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 5a4342881f3b..b02cced96529 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1139,15 +1139,15 @@ __getblk_slow(struct block_device *bdev, sector_t block,
 	for (;;) {
 		struct buffer_head *bh;
 
+		if (!grow_buffers(bdev, block, size, gfp))
+			return NULL;
+
 		if (blocking)
 			bh = __find_get_block_nonatomic(bdev, block, size);
 		else
 			bh = __find_get_block(bdev, block, size);
 		if (bh)
 			return bh;
-
-		if (!grow_buffers(bdev, block, size, gfp))
-			return NULL;
 	}
 }
 
-- 
2.39.5

[PATCH 3/4] fs/buffer: remove superfluous statements

[Davidlohr Bueso]

Get rid of those unnecessary return statements.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 fs/buffer.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index b02cced96529..210b43574a10 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -297,7 +297,6 @@ static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
 
 still_busy:
 	spin_unlock_irqrestore(&first->b_uptodate_lock, flags);
-	return;
 }
 
 struct postprocess_bh_ctx {
@@ -422,7 +421,6 @@ static void end_buffer_async_write(struct buffer_head *bh, int uptodate)
 
 still_busy:
 	spin_unlock_irqrestore(&first->b_uptodate_lock, flags);
-	return;
 }
 
 /*
@@ -1684,7 +1682,6 @@ void block_invalidate_folio(struct folio *folio, size_t offset, size_t length)
 		filemap_release_folio(folio, 0);
 out:
 	folio_clear_mappedtodisk(folio);
-	return;
 }
 EXPORT_SYMBOL(block_invalidate_folio);
 
-- 
2.39.5

[PATCH 4/4] fs/buffer: optimize discard_buffer()

[Davidlohr Bueso]

While invalidating, the clearing of the bits in discard_buffer()
is done in one fully ordered CAS operation. In the past this was
done via individual clear_bit(), until e7470ee89f0 (fs: buffer:
do not use unnecessary atomic operations when discarding buffers).
This implies that there were never strong ordering requirements
outside of being serialized by the buffer lock.

As such relax the ordering for archs that can benefit. Further,
the implied ordering in buffer_unlock() makes current cmpxchg
implied barrier redundant due to release semantics. And while in
theory the unlock could be part of the bulk clearing, it is
best to leave it explicit, but without the double barriers.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 fs/buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 210b43574a10..f0fc78910abf 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1616,8 +1616,8 @@ static void discard_buffer(struct buffer_head * bh)
 	bh->b_bdev = NULL;
 	b_state = READ_ONCE(bh->b_state);
 	do {
-	} while (!try_cmpxchg(&bh->b_state, &b_state,
-			      b_state & ~BUFFER_FLAGS_DISCARD));
+	} while (!try_cmpxchg_relaxed(&bh->b_state, &b_state,
+				      b_state & ~BUFFER_FLAGS_DISCARD));
 	unlock_buffer(bh);
 }
 
-- 
2.39.5

Re: [PATCH 1/4] fs/buffer: use sleeping lookup in __getblk_slowpath()

[Jan Kara]

On Thu 15-05-25 10:39:22, Davidlohr Bueso wrote:
> Just as with the fast path, call the lookup variant depending
> on the gfp flags.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/buffer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b8e1e6e325cd..5a4342881f3b 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1122,6 +1122,8 @@ static struct buffer_head *
>  __getblk_slow(struct block_device *bdev, sector_t block,
>  	     unsigned size, gfp_t gfp)
>  {
> +	bool blocking = gfpflags_allow_blocking(gfp);
> +
>  	/* Size must be multiple of hard sectorsize */
>  	if (unlikely(size & (bdev_logical_block_size(bdev)-1) ||
>  			(size < 512 || size > PAGE_SIZE))) {
> @@ -1137,7 +1139,10 @@ __getblk_slow(struct block_device *bdev, sector_t block,
>  	for (;;) {
>  		struct buffer_head *bh;
>  
> -		bh = __find_get_block(bdev, block, size);
> +		if (blocking)
> +			bh = __find_get_block_nonatomic(bdev, block, size);
> +		else
> +			bh = __find_get_block(bdev, block, size);
>  		if (bh)
>  			return bh;
>  
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/4] fs/buffer: avoid redundant lookup in getblk slowpath

[Jan Kara]

On Thu 15-05-25 10:39:23, Davidlohr Bueso wrote:
> __getblk_slow() already implies failing a first lookup
> as the fastpath, so try to create the buffers immediately
> and avoid the redundant lookup. This saves 5-10% of the
> total cost/latency of the slowpath.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Makes sense. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/buffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 5a4342881f3b..b02cced96529 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1139,15 +1139,15 @@ __getblk_slow(struct block_device *bdev, sector_t block,
>  	for (;;) {
>  		struct buffer_head *bh;
>  
> +		if (!grow_buffers(bdev, block, size, gfp))
> +			return NULL;
> +
>  		if (blocking)
>  			bh = __find_get_block_nonatomic(bdev, block, size);
>  		else
>  			bh = __find_get_block(bdev, block, size);
>  		if (bh)
>  			return bh;
> -
> -		if (!grow_buffers(bdev, block, size, gfp))
> -			return NULL;
>  	}
>  }
>  
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/4] fs/buffer: remove superfluous statements

[Jan Kara]

On Thu 15-05-25 10:39:24, Davidlohr Bueso wrote:
> Get rid of those unnecessary return statements.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Sure. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/buffer.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b02cced96529..210b43574a10 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -297,7 +297,6 @@ static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
>  
>  still_busy:
>  	spin_unlock_irqrestore(&first->b_uptodate_lock, flags);
> -	return;
>  }
>  
>  struct postprocess_bh_ctx {
> @@ -422,7 +421,6 @@ static void end_buffer_async_write(struct buffer_head *bh, int uptodate)
>  
>  still_busy:
>  	spin_unlock_irqrestore(&first->b_uptodate_lock, flags);
> -	return;
>  }
>  
>  /*
> @@ -1684,7 +1682,6 @@ void block_invalidate_folio(struct folio *folio, size_t offset, size_t length)
>  		filemap_release_folio(folio, 0);
>  out:
>  	folio_clear_mappedtodisk(folio);
> -	return;
>  }
>  EXPORT_SYMBOL(block_invalidate_folio);
>  
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/4] fs/buffer: optimize discard_buffer()

[Jan Kara]

On Thu 15-05-25 10:39:25, Davidlohr Bueso wrote:
> While invalidating, the clearing of the bits in discard_buffer()
> is done in one fully ordered CAS operation. In the past this was
> done via individual clear_bit(), until e7470ee89f0 (fs: buffer:
> do not use unnecessary atomic operations when discarding buffers).
> This implies that there were never strong ordering requirements
> outside of being serialized by the buffer lock.
> 
> As such relax the ordering for archs that can benefit. Further,
> the implied ordering in buffer_unlock() makes current cmpxchg
> implied barrier redundant due to release semantics. And while in
> theory the unlock could be part of the bulk clearing, it is
> best to leave it explicit, but without the double barriers.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Yes, we just want to clear several flag bits as cheaply as possible while
other tasks can be modifying other flags. You change makes sense so feel
free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 210b43574a10..f0fc78910abf 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1616,8 +1616,8 @@ static void discard_buffer(struct buffer_head * bh)
>  	bh->b_bdev = NULL;
>  	b_state = READ_ONCE(bh->b_state);
>  	do {
> -	} while (!try_cmpxchg(&bh->b_state, &b_state,
> -			      b_state & ~BUFFER_FLAGS_DISCARD));
> +	} while (!try_cmpxchg_relaxed(&bh->b_state, &b_state,
> +				      b_state & ~BUFFER_FLAGS_DISCARD));
>  	unlock_buffer(bh);
>  }
>  
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH -next 0/4] fs/buffer: misc optimizations

[Christian Brauner]

On Thu, 15 May 2025 10:39:21 -0700, Davidlohr Bueso wrote:
> Four small patches - the first could be sent to Linus for v6.15 considering
> it is a missing nonblocking lookup conversion in the getblk slowpath I
> had missed. The other two patches are small optimizations found while reading
> the code, and one rocket science cleanup patch.
> 
> Thanks!
> 
> [...]

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[1/4] fs/buffer: use sleeping lookup in __getblk_slowpath()
      https://git.kernel.org/vfs/vfs/c/f13865bcff54
[2/4] fs/buffer: avoid redundant lookup in getblk slowpath
      https://git.kernel.org/vfs/vfs/c/545f109630fc
[3/4] fs/buffer: remove superfluous statements
      https://git.kernel.org/vfs/vfs/c/fd7bedc81a2e
[4/4] fs/buffer: optimize discard_buffer()
      https://git.kernel.org/vfs/vfs/c/a09d25918f3d