Re: [PATCH 2/3] fs/buffer: avoid races with folio migrations on __find_get_block_slow()

[Jan Kara <jack@suse.cz>]

On Sat 29-03-25 23:47:31, Luis Chamberlain wrote:
> diff --git a/fs/buffer.c b/fs/buffer.c
> index c7abb4a029dc..a4e4455a6ce2 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -208,6 +208,15 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
>  	head = folio_buffers(folio);
>  	if (!head)
>  		goto out_unlock;
> +
> +	if (folio->mapping->a_ops->migrate_folio &&
> +	    folio->mapping->a_ops->migrate_folio == buffer_migrate_folio_norefs) {

This is always true for bdev mapping we have here, isn't it?

> +		if (folio_test_lru(folio) &&

Do you expect bdev page cache to contain non-LRU folios? I thought every
pagecache folio is on LRU so this seems pointless as well? But I may be
missing something here.

> +		    folio_test_locked(folio) &&
> +		    !folio_test_writeback(folio))
> +			goto out_unlock;

I find this problematic. It fixes the race with migration, alright
(although IMO we should have a comment very well explaining the interplay
of folio lock and mapping->private_lock to make this work - probably in
buffer_migrate_folio_norefs() - and reference it from here), but there are
places which expect that if __find_get_block() doesn't return anything,
this block is not cached in the buffer cache. And your change breaks this
assumption. Look for example at write_boundary_block(), that will fail to
write the block it should write if it races with someone locking the folio
after your changes. Similarly the code tracking state of deleted metadata
blocks in fs/jbd2/revoke.c will fail to properly update buffer's state if
__find_get_block() suddently starts returning NULL although the buffer is
present in cache. 

> +	}
> +
>  	bh = head;
>  	do {
>  		if (!buffer_mapped(bh))

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR