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.
