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

Message ID	20240126132903.2700077-18-hch@lst.de (mailing list archive)
State	New
Headers	show Return-Path: <owner-linux-mm@kvack.org> From: Christoph Hellwig <hch@lst.de> To: Chandan Babu R <chandan.babu@oracle.com>, "Darrick J. Wong" <djwong@kernel.org>, Hugh Dickins <hughd@google.com>, Andrew Morton <akpm@linux-foundation.org> Cc: linux-xfs@vger.kernel.org, linux-mm@kvack.org Subject: [PATCH 17/21] xfs: add file_{get,put}_folio Date: Fri, 26 Jan 2024 14:28:59 +0100 Message-Id: <20240126132903.2700077-18-hch@lst.de> In-Reply-To: <20240126132903.2700077-1-hch@lst.de> References: <20240126132903.2700077-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org Precedence: bulk
Series	[01/21] mm: move mapping_set_update out of <linux/swap.h> \| expand [01/21] mm: move mapping_set_update out of <linux/swap.h> [02/21] shmem: move shmem_mapping out of line [03/21] shmem: set a_ops earlier in shmem_symlink [04/21] shmem: move the shmem_mapping assert into shmem_get_folio_gfp [05/21] shmem: export shmem_get_folio [06/21] shmem: export shmem_kernel_file_setup [07/21] shmem: document how to "persist" data when using shmem_*file_setup [08/21] xfs: remove xfile_stat [09/21] xfs: remove the xfile_pread/pwrite APIs [10/21] xfs: don't try to handle non-update pages in xfile_obj_load [11/21] xfs: shmem_file_setup can't return NULL [12/21] xfs: don't modify file and inode flags for shmem files [13/21] xfs: don't allow highmem pages in xfile mappings [14/21] xfs: use shmem_get_folio in xfile_obj_store [15/21] xfs: use shmem_get_folio in in xfile_load [16/21] xfs: improve detection of lost xfile contents [17/21] xfs: add file_{get,put}_folio [18/21] xfs: remove xfarray_sortinfo.page_kaddr [19/21] xfs: fix a comment in xfarray.c [20/21] xfs: convert xfarray_pagesort to deal with large folios [21/21] xfs: remove xfile_{get,put}_page

Message ID

20240126132903.2700077-18-hch@lst.de (mailing list archive)

State

New

Headers

From: Christoph Hellwig <hch@lst.de>
To: Chandan Babu R <chandan.babu@oracle.com>,
	"Darrick J. Wong" <djwong@kernel.org>,
	Hugh Dickins <hughd@google.com>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: linux-xfs@vger.kernel.org,
	linux-mm@kvack.org
Subject: [PATCH 17/21] xfs: add file_{get,put}_folio
Date: Fri, 26 Jan 2024 14:28:59 +0100
Message-Id: <20240126132903.2700077-18-hch@lst.de>
In-Reply-To: <20240126132903.2700077-1-hch@lst.de>
References: <20240126132903.2700077-1-hch@lst.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: owner-linux-mm@kvack.org
Precedence: bulk

Series

[01/21] mm: move mapping_set_update out of <linux/swap.h> | expand

Commit Message

Christoph Hellwig Jan. 26, 2024, 1:28 p.m. UTC

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>
---
 fs/xfs/scrub/trace.h |  2 ++
 fs/xfs/scrub/xfile.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/scrub/xfile.h |  7 +++++
 3 files changed, 83 insertions(+)

Comments

Matthew Wilcox Jan. 26, 2024, 4:40 p.m. UTC | #1

On Fri, Jan 26, 2024 at 02:28:59PM +0100, Christoph Hellwig wrote:
> +	/*
> +	 * 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);

What I can't tell from skimming the code is whether we ever get the folio
and don't modify it.  If we do, it might make sense to not set dirty here,
but instead pass a bool to xfile_put_folio().  Or have the caller dirty
the folio if they actually modify it.  But perhaps that never happens
in practice and this is simple and works every time.

Darrick J. Wong Jan. 26, 2024, 4:55 p.m. UTC | #2

On Fri, Jan 26, 2024 at 04:40:57PM +0000, Matthew Wilcox wrote:
> On Fri, Jan 26, 2024 at 02:28:59PM +0100, Christoph Hellwig wrote:
> > +	/*
> > +	 * 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);
> 
> What I can't tell from skimming the code is whether we ever get the folio
> and don't modify it.  If we do, it might make sense to not set dirty here,
> but instead pass a bool to xfile_put_folio().  Or have the caller dirty
> the folio if they actually modify it.  But perhaps that never happens
> in practice and this is simple and works every time.

Generally we won't be allocating an xfile folio without storing data to it.
It's possible that there could be dirty folios containing zeroes (e.g.
an xfarray that stored a bunch of array elements then nullified all of
them) but setting dirty early is simpler.

(and all the users of xfiles are only interested in ephemeral datasets
so most of the dirty pages and the entire file will get flushed out
quickly)

--D

Kent Overstreet Jan. 27, 2024, 1:26 a.m. UTC | #3

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?

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
>

Darrick J. Wong Jan. 27, 2024, 1:32 a.m. UTC | #4

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
> > 
>

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__ */

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

Commit Message

Comments

Patch