Re: [PATCH 1/3] fs: Introduce buffered_write_operations

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jan 30, 2024 at 09:12:52AM +0100, Christoph Hellwig wrote:
> > +struct buffered_write_operations {
> > +	int (*write_begin)(struct file *, struct address_space *mapping,
> > +			loff_t pos, size_t len, struct folio **foliop,
> > +			void **fsdata);
> > +	int (*write_end)(struct file *, struct address_space *mapping,
> > +			loff_t pos, size_t len, size_t copied,
> > +			struct folio *folio, void **fsdata);
> > +};
> 
> Should write_begin simply return the folio or an ERR_PTR instead of
> the return by reference?

OK, I've done that.  It's a _lot_ more intrusive for the ext4
conversion.  There's a higher risk of bugs.  BUT I think it does end up
looking a bit cleaner.  I also did the same conversion to iomap; ie

-static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
-               size_t len, struct folio **foliop)
+static struct folio *iomap_write_begin(struct iomap_iter *iter, loff_t pos,
+               size_t len)

with corresponding changes.  Again, ends up looking slightly cleaner.

> I also wonder if the fsdata paramter should go away - if a fs needs
> to pass forth and back fsdata, generic/filemap_perform_write is
> probably the wrong abstraction for it.

So I could get rid of fsdata in ext4; that works out fine.

We have three filesystems actually using fsdata (unless they call fsdata
something else ...)

fs/bcachefs/fs-io-buffered.c:   *fsdata = res;
fs/f2fs/compress.c:             *fsdata = cc->rpages;
fs/ocfs2/aops.c:        *fsdata = wc;

bcachefs seems to actually be using it for its intended purpose --
passing a reservation between write_begin and write_end.

f2fs also doesn't seem terribly objectional; passing rpages between
begin & end.

ocfs2 is passing a ocfs2_write_ctxt between the two.

I don't know that it's a win to remove fsdata from these callbacks,
only to duplicate __generic_file_write_iter() into ocfs2,
generic_perform_write() into f2fs and ... er, I don't think bcachefs
uses any of the functions which would call back through
write_begin/write_end.  So maybe that one's dead code?

Anyway, I'm most inclined to leave fsdata andling the way I had it
in v1, unless you have a better suggestion.