Re: [PATCH 1/7] fs/buffer: split locking for pagecache lookups

[Jan Kara <jack@suse.cz>]

On Tue 15-04-25 16:16:29, Davidlohr Bueso wrote:
> Callers of __find_get_block() may or may not allow for blocking
> semantics, and is currently assumed that it will not. Layout
> two paths based on this. The the private_lock scheme will
> continued to be used for atomic contexts. Otherwise take the
> folio lock instead, which protects the buffers, such as
> vs migration and try_to_free_buffers().
> 
> Per the "hack idea", the latter can alleviate contention on
> the private_lock for bdev mappings. For reasons of determinism
> and avoid making bugs hard to reproduce, the trylocking is not
> attempted.
> 
> No change in semantics. All lookup users still take the spinlock.
> 
> 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 | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b99dc69dba37..c72ebff1b3f0 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -176,18 +176,8 @@ void end_buffer_write_sync(struct buffer_head *bh, int uptodate)
>  }
>  EXPORT_SYMBOL(end_buffer_write_sync);
>  
> -/*
> - * Various filesystems appear to want __find_get_block to be non-blocking.
> - * But it's the page lock which protects the buffers.  To get around this,
> - * we get exclusion from try_to_free_buffers with the blockdev mapping's
> - * i_private_lock.
> - *
> - * Hack idea: for the blockdev mapping, i_private_lock contention
> - * may be quite high.  This code could TryLock the page, and if that
> - * succeeds, there is no need to take i_private_lock.
> - */
>  static struct buffer_head *
> -__find_get_block_slow(struct block_device *bdev, sector_t block)
> +__find_get_block_slow(struct block_device *bdev, sector_t block, bool atomic)
>  {
>  	struct address_space *bd_mapping = bdev->bd_mapping;
>  	const int blkbits = bd_mapping->host->i_blkbits;
> @@ -204,7 +194,16 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
>  	if (IS_ERR(folio))
>  		goto out;
>  
> -	spin_lock(&bd_mapping->i_private_lock);
> +	/*
> +	 * Folio lock protects the buffers. Callers that cannot block
> +	 * will fallback to serializing vs try_to_free_buffers() via
> +	 * the i_private_lock.
> +	 */
> +	if (atomic)
> +		spin_lock(&bd_mapping->i_private_lock);
> +	else
> +		folio_lock(folio);
> +
>  	head = folio_buffers(folio);
>  	if (!head)
>  		goto out_unlock;
> @@ -236,7 +235,10 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
>  		       1 << blkbits);
>  	}
>  out_unlock:
> -	spin_unlock(&bd_mapping->i_private_lock);
> +	if (atomic)
> +		spin_unlock(&bd_mapping->i_private_lock);
> +	else
> +		folio_unlock(folio);
>  	folio_put(folio);
>  out:
>  	return ret;
> @@ -1388,14 +1390,15 @@ lookup_bh_lru(struct block_device *bdev, sector_t block, unsigned size)
>   * it in the LRU and mark it as accessed.  If it is not present then return
>   * NULL
>   */
> -struct buffer_head *
> -__find_get_block(struct block_device *bdev, sector_t block, unsigned size)
> +static struct buffer_head *
> +find_get_block_common(struct block_device *bdev, sector_t block,
> +			unsigned size, bool atomic)
>  {
>  	struct buffer_head *bh = lookup_bh_lru(bdev, block, size);
>  
>  	if (bh == NULL) {
>  		/* __find_get_block_slow will mark the page accessed */
> -		bh = __find_get_block_slow(bdev, block);
> +		bh = __find_get_block_slow(bdev, block, atomic);
>  		if (bh)
>  			bh_lru_install(bh);
>  	} else
> @@ -1403,6 +1406,12 @@ __find_get_block(struct block_device *bdev, sector_t block, unsigned size)
>  
>  	return bh;
>  }
> +
> +struct buffer_head *
> +__find_get_block(struct block_device *bdev, sector_t block, unsigned size)
> +{
> +	return find_get_block_common(bdev, block, size, true);
> +}
>  EXPORT_SYMBOL(__find_get_block);
>  
>  /**
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR