Re: [RFC PATCH] mm/filemap: make release_folio mandatory and add block_release_folio

[Jan Kara]

On Sat 20-12-25 01:07:52, Deepakkumar Karn wrote:
> Currently, filemap_release_folio() falls back to try_to_free_buffers()
> when a filesystem's address_space_operations doesn't provide a
> release_folio callback. This creates an inconsistent API where some
> buffer_head-based filesystems define release_folio while others rely
> on the fallback behavior.
> 
> Make the release_folio callback mandatory by:
> 
> 1. Adding block_release_folio() in fs/buffer.c as the default
>    implementation for buffer_head-based filesystems. This simply
>    calls try_to_free_buffers().
> 
> 2. Updating all buffer_head-based filesystems that use
>    buffer_migrate_folio() to set .release_folio = block_release_folio
>    in their address_space_operations.
> 
> 3. Removing the NULL check and try_to_free_buffers() fallback in
>    filemap_release_folio(). Now it always calls the release_folio
>    callback when a mapping exists.
> 
> With these changes:
> - Makes the pattern explicit across all buffer_head filesystems
> - Prevents future bugs from missing release_folio implementations
> 
> Filesystems updated: adfs, affs, bfs, exfat, ext2, fat, hpfs, isofs,
> jfs, minix, nilfs2, ntfs3, omfs, udf, ufs, plus block/fops.c
> 
> Filesystems with existing custom implementations (ext4, gfs2, hfs,
> hfsplus, jfs metapage, ocfs2) are unchanged.
> 
> Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Deepakkumar Karn <dkarn@xxxxxxxxxx>

...

> diff --git a/fs/affs/file.c b/fs/affs/file.c
> index 765c3443663e..c03ac926c3a5 100644
> --- a/fs/affs/file.c
> +++ b/fs/affs/file.c
> @@ -464,7 +464,8 @@ const struct address_space_operations affs_aops = {
>  	.write_end = affs_write_end,
>  	.direct_IO = affs_direct_IO,
>  	.migrate_folio = buffer_migrate_folio,
> -	.bmap = _affs_bmap
> +	.bmap = _affs_bmap,
> +	.release_folio = block_release_folio,
>  };
>  

I think affs_aops_ofs need .release_folio as well...

Also befs_aops, ecryptfs_aops, efs_aops, vxfs_aops, hfs_aops, hfsplus_aops,
nilfs_buffer_cache_aops, qnx4_aops, qnx6_aops definitely need .release_folio. ntfs_aops_cmpr
might need it as well - not sure.

> diff --git a/fs/buffer.c b/fs/buffer.c
> index 838c0c571022..eb856901ac3a 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2914,6 +2914,23 @@ drop_buffers(struct folio *folio, struct buffer_head **buffers_to_free)
>  	return false;
>  }
>  
> +/**
> + * block_release_folio - Release buffers attached to a BH-based filesystems.
> + * @folio: The folio.
	      ^^ "The folio with attached buffers."

> + * @gfp: The gfp parameter.
	    ^^ "Memory allocation flags (and I/O mode)."

> + *
> + * This is the default release_folio implementation for buffer_head based
> + * filesystems. Filesystems with special requirements
> + * should implement their own release_folio callback.
> + *
> + * Return: true if buffers were released, false otherwise.
> + */

Otherwise the patch looks good to me.

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR