Uses of ->write_begin/write_end

Uses of ->write_begin/write_end

[Matthew Wilcox]

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()

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]
 - nothing [34]
 - 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?

Re: Uses of ->write_begin/write_end

[Matthew Wilcox]

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?
> 
> 

Re: Uses of ->write_begin/write_end

[Christoph Hellwig]

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()

These really need to use actual shmem exported APIs, probably
shmem_get_folio, instead of abusing the aops.  With that we can
then easily kill ->write_begin() / ->write_end() for shmem.

> fs/affs/file.c:

Most of these fs-specific ones should really hardcode the calls to the
usually once or sometimes few potential instances that could be called
so that we can devirtualize the alls.

> fs/buffer.c:
> [4]	generic_cont_expand_simple()
> [5]	cont_expand_zero()
> [6]	cont_expand_zero()

> fs/namei.c:
> [B]	page_symlink()

> 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().
> 

They often sit much lower in the stack and/or are used for files that
don't have a ->write_iter.  e.g. page_symlink is obviously used for
symlinks that don't have ->write_iter.

For generic_cont_expand_simple goins through write_iter might be an
option, but instead of going through file ops the better idea might be
to just pass a write_iter-prototyped callback directly to it.

cont_expand_zero is a helper for cont_write_begin, which is used to
implement ->write_begin, so this actually already is a recursion, adding
another indirect to it is probably not helpful.