Re: [PATCH v2 06/11] fuse: use iomap for writeback cache buffered writes

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Aug 29, 2024 at 01:50:59PM +1000, Dave Chinner wrote:
> On Wed, Aug 28, 2024 at 05:13:56PM -0400, Josef Bacik wrote:
> > We're currently using the old ->write_begin()/->write_end() method of
> > doing buffered writes.  This isn't a huge deal for fuse since we
> > basically just want to copy the pages and move on, but the iomap
> > infrastructure gives us access to having huge folios.  Rework the
> > buffered write path when we have writeback cache to use the iomap
> > buffered write code, the ->get_folio() callback now handles the work
> > that we did in ->write_begin(), the rest of the work is handled inside
> > of iomap so we don't need a replacement for ->write_end.
> > 
> > This does bring BLOCK as a dependency, as the buffered write part of
> > iomap requires CONFIG_BLOCK.  This could be shed if we reworked the file
> > write iter portion of the buffered write path was separated out to not
> > need BLOCK.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/fuse/Kconfig |   2 +
> >  fs/fuse/file.c  | 154 +++++++++++++++++++++---------------------------
> >  2 files changed, 68 insertions(+), 88 deletions(-)
> > 
> > diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
> > index 8674dbfbe59d..8a799324d7bd 100644
> > --- a/fs/fuse/Kconfig
> > +++ b/fs/fuse/Kconfig
> > @@ -1,7 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  config FUSE_FS
> >  	tristate "FUSE (Filesystem in Userspace) support"
> > +	depends on BLOCK
> >  	select FS_POSIX_ACL
> > +	select FS_IOMAP
> >  	help
> >  	  With FUSE it is possible to implement a fully functional filesystem
> >  	  in a userspace program.
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index ab531a4694b3..af91043b44d7 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/filelock.h>
> >  #include <linux/splice.h>
> >  #include <linux/task_io_accounting_ops.h>
> > +#include <linux/iomap.h>
> >  
> >  static int fuse_send_open(struct fuse_mount *fm, u64 nodeid,
> >  			  unsigned int open_flags, int opcode,
> > @@ -1420,6 +1421,63 @@ static void fuse_dio_unlock(struct kiocb *iocb, bool exclusive)
> >  	}
> >  }
> >  
> > +static struct folio *fuse_iomap_get_folio(struct iomap_iter *iter,
> > +					  loff_t pos, unsigned len)
> > +{
> > +	struct file *file = (struct file *)iter->private;
> > +	struct inode *inode = iter->inode;
> > +	struct folio *folio;
> > +	loff_t fsize;
> > +
> > +	folio = iomap_get_folio(iter, pos, len);
> > +	if (IS_ERR(folio))
> > +		return folio;
> > +
> > +	fuse_wait_on_folio_writeback(inode, folio);
> > +
> > +	if (folio_test_uptodate(folio))
> > +		return folio;
> > +
> > +	/*
> > +	 * If we're going to write past EOF then avoid the read, but zero the
> > +	 * whole thing and mark it uptodate so that if we get a short write we
> > +	 * don't try to re-read this page, we just carry on.
> > +	 */
> > +	fsize = i_size_read(inode);
> > +	if (fsize <= folio_pos(folio)) {
> > +		folio_zero_range(folio, 0, folio_size(folio));
> 
> The comment doesn't match what this does - the folio is not marked
> uptodate at all.

I'll update the comment, it gets marked uptodate in __iomap_write_end() once the
write is complete.

> 
> > +	} else {
> > +		int err = fuse_do_readpage(file, &folio->page);
> 
> readpage on a large folio? does that work?

I haven't done the work to enable large folios yet, this is just the prep stuff.
Supporting large folios is going to take a fair bit of work, so I'm getting the
ball rolling with this prep series.

> 
> > +		if (err) {
> > +			folio_unlock(folio);
> > +			folio_put(folio);
> > +			return ERR_PTR(err);
> > +		}
> > +	}
> 
> Also, why do this here when __iomap_write_begin() will do all the
> sub-folio zeroing and read IO on the folio?
> 

I looked long and hard at iomap because I thought it would, but it turns out it
won't work for fuse.  I could be totally wrong, but looking at iomap it will
allocate an ifs because it assumes this is sub-folio blocksize, but we aren't,
and don't really want to incur that pain.  Additionally it does
iomap_read_folio_sync() to read in the folio, which just does a bio, which
obviously doesn't work on fuse.  Again totally expecting to be told I'm stupid
in some way that I missed, but it seemed like iomap won't do what we need it to
do here, and it's simple enough to handle the zeroing here for ourselves.

> > +
> > +	return folio;
> > +}
> > +
> > +static const struct iomap_folio_ops fuse_iomap_folio_ops = {
> > +	.get_folio = fuse_iomap_get_folio,
> > +};
> > +
> > +static int fuse_iomap_begin_write(struct inode *inode, loff_t pos, loff_t length,
> > +				  unsigned flags, struct iomap *iomap,
> > +				  struct iomap *srcmap)
> > +{
> > +	iomap->type = IOMAP_DELALLOC;
> > +	iomap->addr = IOMAP_NULL_ADDR;
> > +	iomap->offset = pos;
> > +	iomap->length = length;
> > +	iomap->folio_ops = &fuse_iomap_folio_ops;
> > +	return 0;
> > +}
> 
> What's the reason for using IOMAP_DELALLOC for these mappings? I'm
> not saying it is wrong, I just don't know enough about fuse to
> understand is this is valid or not because there are no iomap-based
> writeback hooks being added here....

At the time it was "oh we're doing what equates to delalloc, clearly this should
be marked as delalloc".  Now that I have been in this code longer I realize this
is supposed to be "what does this range look like now", so I suppose the "right"
thing to do here is use IOMAP_HOLE?  Thanks,

Josef