Re: [PATCH 17/21] xfs: add file_{get,put}_folio

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Jan 26, 2024 at 08:26:22PM -0500, Kent Overstreet wrote:
> On Fri, Jan 26, 2024 at 02:28:59PM +0100, Christoph Hellwig wrote:
> > From: "Darrick J. Wong" <djwong@kernel.org>
> > 
> > Add helper similar to file_{get,set}_page, but which deal with folios
> > and don't allocate new folio unless explicitly asked to, which map
> > to shmem_get_folio instead of calling into the aops.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> looks boilerplatey to my eyes, but this is all new conceptual stuff and
> the implementation will be evolving, so...
> 
> one nit: that's really not the right place for memalloc_nofs_save(), can
> we try to start figuring out the proper locations for those?

I /think/ this is actually unnecessary since the xfs scrub code will
have already allocated a (possibly empty) transaction, which will have
set PF_MEMALLOC_NOFS.  But.  I'd rather concentrate on merging the code
and fixing the correctness bugs; and later we can find and remove the
unnecessary bits.

(Yeah shameful copy pasta from shmem.c.)

--D

> Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
> 
> > ---
> >  fs/xfs/scrub/trace.h |  2 ++
> >  fs/xfs/scrub/xfile.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/scrub/xfile.h |  7 +++++
> >  3 files changed, 83 insertions(+)
> > 
> > diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> > index 0327cab606b070..c61fa7a95ef522 100644
> > --- a/fs/xfs/scrub/trace.h
> > +++ b/fs/xfs/scrub/trace.h
> > @@ -908,6 +908,8 @@ DEFINE_XFILE_EVENT(xfile_store);
> >  DEFINE_XFILE_EVENT(xfile_seek_data);
> >  DEFINE_XFILE_EVENT(xfile_get_page);
> >  DEFINE_XFILE_EVENT(xfile_put_page);
> > +DEFINE_XFILE_EVENT(xfile_get_folio);
> > +DEFINE_XFILE_EVENT(xfile_put_folio);
> >  
> >  TRACE_EVENT(xfarray_create,
> >  	TP_PROTO(struct xfarray *xfa, unsigned long long required_capacity),
> > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c
> > index 2d802c20a8ddfe..1c1db4ae1ba6ee 100644
> > --- a/fs/xfs/scrub/xfile.c
> > +++ b/fs/xfs/scrub/xfile.c
> > @@ -365,3 +365,77 @@ xfile_put_page(
> >  		return -EIO;
> >  	return 0;
> >  }
> > +
> > +/*
> > + * Grab the (locked) folio for a memory object.  The object cannot span a folio
> > + * boundary.  Returns the locked folio if successful, NULL if there was no
> > + * folio or it didn't cover the range requested, or an ERR_PTR on failure.
> > + */
> > +struct folio *
> > +xfile_get_folio(
> > +	struct xfile		*xf,
> > +	loff_t			pos,
> > +	size_t			len,
> > +	unsigned int		flags)
> > +{
> > +	struct inode		*inode = file_inode(xf->file);
> > +	struct folio		*folio = NULL;
> > +	unsigned int		pflags;
> > +	int			error;
> > +
> > +	if (inode->i_sb->s_maxbytes - pos < len)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	trace_xfile_get_folio(xf, pos, len);
> > +
> > +	/*
> > +	 * Increase the file size first so that shmem_get_folio(..., SGP_CACHE),
> > +	 * actually allocates a folio instead of erroring out.
> > +	 */
> > +	if ((flags & XFILE_ALLOC) && pos + len > i_size_read(inode))
> > +		i_size_write(inode, pos + len);
> > +
> > +	pflags = memalloc_nofs_save();
> > +	error = shmem_get_folio(inode, pos >> PAGE_SHIFT, &folio,
> > +			(flags & XFILE_ALLOC) ? SGP_CACHE : SGP_READ);
> > +	memalloc_nofs_restore(pflags);
> > +	if (error)
> > +		return ERR_PTR(error);
> > +
> > +	if (!folio)
> > +		return NULL;
> > +
> > +	if (len > folio_size(folio) - offset_in_folio(folio, pos)) {
> > +		folio_unlock(folio);
> > +		folio_put(folio);
> > +		return NULL;
> > +	}
> > +
> > +	if (xfile_has_lost_data(inode, folio)) {
> > +		folio_unlock(folio);
> > +		folio_put(folio);
> > +		return ERR_PTR(-EIO);
> > +	}
> > +
> > +	/*
> > +	 * Mark the folio dirty so that it won't be reclaimed once we drop the
> > +	 * (potentially last) reference in xfile_put_folio.
> > +	 */
> > +	if (flags & XFILE_ALLOC)
> > +		folio_set_dirty(folio);
> > +	return folio;
> > +}
> > +
> > +/*
> > + * Release the (locked) folio for a memory object.
> > + */
> > +void
> > +xfile_put_folio(
> > +	struct xfile		*xf,
> > +	struct folio		*folio)
> > +{
> > +	trace_xfile_put_folio(xf, folio_pos(folio), folio_size(folio));
> > +
> > +	folio_unlock(folio);
> > +	folio_put(folio);
> > +}
> > diff --git a/fs/xfs/scrub/xfile.h b/fs/xfs/scrub/xfile.h
> > index 465b10f492b66d..afb75e9fbaf265 100644
> > --- a/fs/xfs/scrub/xfile.h
> > +++ b/fs/xfs/scrub/xfile.h
> > @@ -39,4 +39,11 @@ int xfile_get_page(struct xfile *xf, loff_t offset, unsigned int len,
> >  		struct xfile_page *xbuf);
> >  int xfile_put_page(struct xfile *xf, struct xfile_page *xbuf);
> >  
> > +#define XFILE_MAX_FOLIO_SIZE	(PAGE_SIZE << MAX_PAGECACHE_ORDER)
> > +
> > +#define XFILE_ALLOC		(1 << 0) /* allocate folio if not present */
> > +struct folio *xfile_get_folio(struct xfile *xf, loff_t offset, size_t len,
> > +		unsigned int flags);
> > +void xfile_put_folio(struct xfile *xf, struct folio *folio);
> > +
> >  #endif /* __XFS_SCRUB_XFILE_H__ */
> > -- 
> > 2.39.2
> > 
>