Re: Uses of ->write_begin/write_end

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jul 15, 2024 at 04:59:36PM +0100, Matthew Wilcox wrote:
> I'm looking at ->write_begin() / ->write_end() again.  Here are our
> current callers:
> 
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c:
> [1]	shmem_pwrite()
> [2]	i915_gem_object_create_shmem_from_data()
> fs/affs/file.c:
> [3]	affs_truncate()
> fs/buffer.c:
> [4]	generic_cont_expand_simple()
> [5]	cont_expand_zero()
> [6]	cont_expand_zero()
> fs/exfat/file.c:
> [7]	exfat_file_zeroed_range()
> fs/ext4/verity.c:
> [8]	pagecache_write()
> fs/f2fs/super.c:
> [9]	f2fs_quota_write()
> fs/f2fs/verity.c:
> [A]	pagecache_write()
> fs/namei.c:
> [B]	page_symlink()
> mm/filemap.c:
> [C]	generic_perform_write()

I found a few variants of the same pattern:

fs/hfs/extent.c:
[D]	hfs_file_truncate()
fs/hfsplus/extents.c:
[E]	hfsplus_file_truncate()
fs/ntfs3/file.c:
[F]	ntfs_extend_initialized_size()

> There are essentially four things that happen between ->write_begin()
> and ->write_end() in these 12 callers:
> 
>  - copy_from_user [1]
>  - memcpy [289AB]
>  - zero [567]

 - zero [567F]

>  - nothing [34]

 - nothing [34DE]

>  - copy_from_iter [C]
> 
> I suspect that exfat_file_zeroed_range() should be calling
> cont_expand_zero(), which means it would need to be exported, but
> that seems like an improvement over calling write_begin/write_end
> itself.
> 
> The copy_from_user() / memcpy() users feel like they should all end
> up calling ->write_iter().  One way they could do this is by calling
> kernel_write() / __kernel_write(), but I'm not sure whether they
> should have the various accounting things (add_wchar(), inc_syscw())
> that happen inside __kernel_write_iter().
> 
> So should we add:
> 
> ssize_t filemap_write_iter(struct file *file, struct iov_iter *from)
> { ... }
> 
> which contains the guts of __kernel_write_iter?
> ext4's verity code needs a minor refactor to pass down the file
> (but note comment about how it's a RO file descriptor)
> f2fs_quota_write doesn't have a struct file and looks generally awkward.
> page_symlink() is also awkward.
> 
> I think that means we need something that _doesn't work_ for iomap-based
> filesystems.  All of these callers know the filesystem they're working
> on doesn't use iomap, so perhaps filemap_write_iter() just takes a
> struct address_space and assumes the existance of
> ->write_begin/->write_end.
> 
> Thoughts?
> 
>