Received-SPF: pass (google.com: domain of linux-ext4+bounces-1050-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 1 Feb 2024 04:36:09 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 1/3] fs: Introduce buffered_write_operations
Message-ID: <ZbsfuaANd4DIVb4w@casper.infradead.org>
References: <20240130055414.2143959-1-willy@infradead.org>
 <20240130055414.2143959-2-willy@infradead.org>
 <20240130081252.GC22621@lst.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130081252.GC22621@lst.de>

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.