optimize local for and shortform directory handling

optimize local for and shortform directory handling

[Christoph Hellwig]

Hi all,

this series optimizes the local fork handling by making ownership of the
memory used for the local fork more flexible and thus removing various
extra buffer allocations.

Diffstat:
 xfs_attr_leaf.c  |   14 -
 xfs_bmap.c       |   13 -
 xfs_bmap.h       |    2 
 xfs_dir2_block.c |   46 ----
 xfs_dir2_priv.h  |   17 +
 xfs_dir2_sf.c    |  570 ++++++++++++++++++++++---------------------------------
 xfs_exchmaps.c   |    8 
 7 files changed, 260 insertions(+), 410 deletions(-)

[PATCH 01/16] xfs: allow non-empty forks in xfs_bmap_local_to_extents_empty

[Christoph Hellwig]

Currently xfs_bmap_local_to_extents_empty expects the caller to set the
for size to 0, which implies freeing if_data.  That is highly suboptimal
because the callers need the data in the local fork so that they can
copy it to the newly allocated extent.

Change xfs_bmap_local_to_extents_empty to return the old fork data and
clear if_bytes to zero instead and let the callers free the memory for
the local fork, which allows them to access the data directly while
formatting the extent format data.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 13 +++++++------
 fs/xfs/libxfs/xfs_bmap.h |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 6053f5e5c71eec..eb826fae8fd878 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -754,31 +754,32 @@ xfs_bmap_extents_to_btree(
 
 /*
  * Convert a local file to an extents file.
- * This code is out of bounds for data forks of regular files,
- * since the file data needs to get logged so things will stay consistent.
- * (The bmap-level manipulations are ok, though).
+ *
+ * Returns the content of the old data fork, which needs to be freed by the
+ * caller.
  */
-void
+void *
 xfs_bmap_local_to_extents_empty(
 	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	int			whichfork)
 {
 	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
+	void			*old_data = ifp->if_data;
 
 	ASSERT(whichfork != XFS_COW_FORK);
 	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
-	ASSERT(ifp->if_bytes == 0);
 	ASSERT(ifp->if_nextents == 0);
 
 	xfs_bmap_forkoff_reset(ip, whichfork);
 	ifp->if_data = NULL;
+	ifp->if_bytes = 0;
 	ifp->if_height = 0;
 	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	return old_data;
 }
 
-
 int					/* error */
 xfs_bmap_local_to_extents(
 	xfs_trans_t	*tp,		/* transaction pointer */
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index e98849eb9bbae3..6e0b6081bf3aa5 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -178,7 +178,7 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
 unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp);
 int	xfs_bmap_add_attrfork(struct xfs_trans *tp, struct xfs_inode *ip,
 		int size, int rsvd);
-void	xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
+void	*xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
 		struct xfs_inode *ip, int whichfork);
 int xfs_bmap_local_to_extents(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_extlen_t total, int *logflagsp, int whichfork,
-- 
2.39.2

Re: [PATCH 01/16] xfs: allow non-empty forks in xfs_bmap_local_to_extents_empty

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:11PM +0200, Christoph Hellwig wrote:
> Currently xfs_bmap_local_to_extents_empty expects the caller to set the
> for size to 0, which implies freeing if_data.  That is highly suboptimal
> because the callers need the data in the local fork so that they can
> copy it to the newly allocated extent.
> 
> Change xfs_bmap_local_to_extents_empty to return the old fork data and
> clear if_bytes to zero instead and let the callers free the memory for

But I don't see any changes in the callsites to do that freeing, is this
a memory leak?

--D

> the local fork, which allows them to access the data directly while
> formatting the extent format data.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 13 +++++++------
>  fs/xfs/libxfs/xfs_bmap.h |  2 +-
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6053f5e5c71eec..eb826fae8fd878 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -754,31 +754,32 @@ xfs_bmap_extents_to_btree(
>  
>  /*
>   * Convert a local file to an extents file.
> - * This code is out of bounds for data forks of regular files,
> - * since the file data needs to get logged so things will stay consistent.
> - * (The bmap-level manipulations are ok, though).
> + *
> + * Returns the content of the old data fork, which needs to be freed by the
> + * caller.
>   */
> -void
> +void *
>  xfs_bmap_local_to_extents_empty(
>  	struct xfs_trans	*tp,
>  	struct xfs_inode	*ip,
>  	int			whichfork)
>  {
>  	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
> +	void			*old_data = ifp->if_data;
>  
>  	ASSERT(whichfork != XFS_COW_FORK);
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(ifp->if_bytes == 0);
>  	ASSERT(ifp->if_nextents == 0);
>  
>  	xfs_bmap_forkoff_reset(ip, whichfork);
>  	ifp->if_data = NULL;
> +	ifp->if_bytes = 0;
>  	ifp->if_height = 0;
>  	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	return old_data;
>  }
>  
> -
>  int					/* error */
>  xfs_bmap_local_to_extents(
>  	xfs_trans_t	*tp,		/* transaction pointer */
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index e98849eb9bbae3..6e0b6081bf3aa5 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -178,7 +178,7 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
>  unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp);
>  int	xfs_bmap_add_attrfork(struct xfs_trans *tp, struct xfs_inode *ip,
>  		int size, int rsvd);
> -void	xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
> +void	*xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
>  		struct xfs_inode *ip, int whichfork);
>  int xfs_bmap_local_to_extents(struct xfs_trans *tp, struct xfs_inode *ip,
>  		xfs_extlen_t total, int *logflagsp, int whichfork,
> -- 
> 2.39.2
> 
> 

Re: [PATCH 01/16] xfs: allow non-empty forks in xfs_bmap_local_to_extents_empty

[Christoph Hellwig]

On Tue, Apr 30, 2024 at 08:51:31AM -0700, Darrick J. Wong wrote:
> > Change xfs_bmap_local_to_extents_empty to return the old fork data and
> > clear if_bytes to zero instead and let the callers free the memory for
> 
> But I don't see any changes in the callsites to do that freeing, is this
> a memory leak?

Even before this patch, xfs_bmap_local_to_extents_empty never frees any
memory, it just asserts the the local fork size has been changed to 0,
which implies that the caller already freed the memory.  With this
patch the caller can free the memory after calling
xfs_bmap_local_to_extents_empty instead of before it, which the callers
(all but one anyway) will make use of in the following patches.

I thought the commit log made that clear, but if you think it needs to
improved feel free to suggest edits.

Re: [PATCH 01/16] xfs: allow non-empty forks in xfs_bmap_local_to_extents_empty

[Darrick J. Wong]

On Wed, May 01, 2024 at 06:37:34AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 30, 2024 at 08:51:31AM -0700, Darrick J. Wong wrote:
> > > Change xfs_bmap_local_to_extents_empty to return the old fork data and
> > > clear if_bytes to zero instead and let the callers free the memory for
> > 
> > But I don't see any changes in the callsites to do that freeing, is this
> > a memory leak?
> 
> Even before this patch, xfs_bmap_local_to_extents_empty never frees any
> memory, it just asserts the the local fork size has been changed to 0,
> which implies that the caller already freed the memory.  With this
> patch the caller can free the memory after calling
> xfs_bmap_local_to_extents_empty instead of before it, which the callers
> (all but one anyway) will make use of in the following patches.
> 
> I thought the commit log made that clear, but if you think it needs to
> improved feel free to suggest edits.

Ah, I see, currently all the callers /do/ free ifp->if_data, having
snapshotted the contents before doing so.  I guess I needed the words
"all callers do that".

How about:

"Currently, xfs_bmap_local_to_extents_empty expects the caller to set
the fork size to 0 and free if_data.  All callers do that, but they also
allocate a temporary shadow buffer because they need the contents of the
fork so they can copy it to the newly allocated extent after the
transition to extents format.  This is highly suboptimal."

?

With some wordsmithing like that,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

[PATCH 02/16] xfs: remove an extra buffer allocation in xfs_attr_shortform_to_leaf

[Christoph Hellwig]

Make use of the new ability to call xfs_bmap_local_to_extents_empty on
a non-empty local fork to reuse the old inode fork data to build the leaf
block to replace the local temporary buffer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index b9e98950eb3d81..d9285614d7c21b 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -942,24 +942,16 @@ xfs_attr_shortform_to_leaf(
 	struct xfs_da_args		*args)
 {
 	struct xfs_inode		*dp = args->dp;
-	struct xfs_ifork		*ifp = &dp->i_af;
-	struct xfs_attr_sf_hdr		*sf = ifp->if_data;
+	struct xfs_attr_sf_hdr		*sf;
 	struct xfs_attr_sf_entry	*sfe;
-	int				size = be16_to_cpu(sf->totsize);
 	struct xfs_da_args		nargs;
-	char				*tmpbuffer;
 	int				error, i;
 	xfs_dablk_t			blkno;
 	struct xfs_buf			*bp;
 
 	trace_xfs_attr_sf_to_leaf(args);
 
-	tmpbuffer = kmalloc(size, GFP_KERNEL | __GFP_NOFAIL);
-	memcpy(tmpbuffer, ifp->if_data, size);
-	sf = (struct xfs_attr_sf_hdr *)tmpbuffer;
-
-	xfs_idata_realloc(dp, -size, XFS_ATTR_FORK);
-	xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK);
+	sf = xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK);
 
 	bp = NULL;
 	error = xfs_da_grow_inode(args, &blkno);
@@ -1003,7 +995,7 @@ xfs_attr_shortform_to_leaf(
 	}
 	error = 0;
 out:
-	kfree(tmpbuffer);
+	kfree(sf);
 	return error;
 }
 
-- 
2.39.2

Re: [PATCH 02/16] xfs: remove an extra buffer allocation in xfs_attr_shortform_to_leaf

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:12PM +0200, Christoph Hellwig wrote:
> Make use of the new ability to call xfs_bmap_local_to_extents_empty on
> a non-empty local fork to reuse the old inode fork data to build the leaf
> block to replace the local temporary buffer.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Makes sense,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index b9e98950eb3d81..d9285614d7c21b 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -942,24 +942,16 @@ xfs_attr_shortform_to_leaf(
>  	struct xfs_da_args		*args)
>  {
>  	struct xfs_inode		*dp = args->dp;
> -	struct xfs_ifork		*ifp = &dp->i_af;
> -	struct xfs_attr_sf_hdr		*sf = ifp->if_data;
> +	struct xfs_attr_sf_hdr		*sf;
>  	struct xfs_attr_sf_entry	*sfe;
> -	int				size = be16_to_cpu(sf->totsize);
>  	struct xfs_da_args		nargs;
> -	char				*tmpbuffer;
>  	int				error, i;
>  	xfs_dablk_t			blkno;
>  	struct xfs_buf			*bp;
>  
>  	trace_xfs_attr_sf_to_leaf(args);
>  
> -	tmpbuffer = kmalloc(size, GFP_KERNEL | __GFP_NOFAIL);
> -	memcpy(tmpbuffer, ifp->if_data, size);
> -	sf = (struct xfs_attr_sf_hdr *)tmpbuffer;
> -
> -	xfs_idata_realloc(dp, -size, XFS_ATTR_FORK);
> -	xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK);
> +	sf = xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK);
>  
>  	bp = NULL;
>  	error = xfs_da_grow_inode(args, &blkno);
> @@ -1003,7 +995,7 @@ xfs_attr_shortform_to_leaf(
>  	}
>  	error = 0;
>  out:
> -	kfree(tmpbuffer);
> +	kfree(sf);
>  	return error;
>  }
>  
> -- 
> 2.39.2
> 
> 

[PATCH 03/16] xfs: rationalize dir2_sf entry condition asserts

[Christoph Hellwig]

Various routines dealing with shortform directories have a similar
pre-condition ASSERT boilerplate that does look a bit weird.

Remove the assert that the inode fork is non-NULL as it doesn't buy
anything over the NULL pointer dereference if it is.

Remove the duplicate i_disk_size ASSERT that uses the less precise
location of the parent inode number over the one using
xfs_dir2_sf_hdr_size().

Remove the if_nextents assert in xfs_dir2_sf_to_block as that is implied
by the local formt (and not checked by the other functions either).

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_block.c |  4 ----
 fs/xfs/libxfs/xfs_dir2_sf.c    | 12 ++----------
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 0f93ed1a4a74f4..035a54dbdd7586 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -1105,12 +1105,8 @@ xfs_dir2_sf_to_block(
 	trace_xfs_dir2_sf_to_block(args);
 
 	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
-	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
-
 	ASSERT(ifp->if_bytes == dp->i_disk_size);
-	ASSERT(oldsfp != NULL);
 	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(oldsfp->i8count));
-	ASSERT(dp->i_df.if_nextents == 0);
 
 	/*
 	 * Copy the directory into a temporary buffer.
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 17a20384c8b719..1cd5228e1ce6af 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -378,9 +378,7 @@ xfs_dir2_sf_addname(
 
 	ASSERT(xfs_dir2_sf_lookup(args) == -ENOENT);
 	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
-	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
-	ASSERT(sfp != NULL);
 	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
 	/*
 	 * Compute entry (and change in) size.
@@ -855,9 +853,7 @@ xfs_dir2_sf_lookup(
 	xfs_dir2_sf_check(args);
 
 	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
-	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
-	ASSERT(sfp != NULL);
 	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
 	/*
 	 * Special case for .
@@ -920,21 +916,19 @@ xfs_dir2_sf_removename(
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
+	int			oldsize = dp->i_disk_size;
 	int			byteoff;	/* offset of removed entry */
 	int			entsize;	/* this entry's size */
 	int			i;		/* shortform entry index */
 	int			newsize;	/* new inode size */
-	int			oldsize;	/* old inode size */
 	xfs_dir2_sf_entry_t	*sfep;		/* shortform directory entry */
 
 	trace_xfs_dir2_sf_removename(args);
 
 	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
-	oldsize = (int)dp->i_disk_size;
-	ASSERT(oldsize >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == oldsize);
-	ASSERT(sfp != NULL);
 	ASSERT(oldsize >= xfs_dir2_sf_hdr_size(sfp->i8count));
+
 	/*
 	 * Loop over the old directory entries.
 	 * Find the one we're deleting.
@@ -1028,9 +1022,7 @@ xfs_dir2_sf_replace(
 	trace_xfs_dir2_sf_replace(args);
 
 	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
-	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
-	ASSERT(sfp != NULL);
 	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
 
 	/*
-- 
2.39.2

Re: [PATCH 03/16] xfs: rationalize dir2_sf entry condition asserts

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:13PM +0200, Christoph Hellwig wrote:
> Various routines dealing with shortform directories have a similar
> pre-condition ASSERT boilerplate that does look a bit weird.
> 
> Remove the assert that the inode fork is non-NULL as it doesn't buy
> anything over the NULL pointer dereference if it is.
> 
> Remove the duplicate i_disk_size ASSERT that uses the less precise
> location of the parent inode number over the one using
> xfs_dir2_sf_hdr_size().
> 
> Remove the if_nextents assert in xfs_dir2_sf_to_block as that is implied
> by the local formt (and not checked by the other functions either).

               format

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_block.c |  4 ----
>  fs/xfs/libxfs/xfs_dir2_sf.c    | 12 ++----------
>  2 files changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 0f93ed1a4a74f4..035a54dbdd7586 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1105,12 +1105,8 @@ xfs_dir2_sf_to_block(
>  	trace_xfs_dir2_sf_to_block(args);
>  
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
> -
>  	ASSERT(ifp->if_bytes == dp->i_disk_size);
> -	ASSERT(oldsfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(oldsfp->i8count));
> -	ASSERT(dp->i_df.if_nextents == 0);
>  
>  	/*
>  	 * Copy the directory into a temporary buffer.
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 17a20384c8b719..1cd5228e1ce6af 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -378,9 +378,7 @@ xfs_dir2_sf_addname(
>  
>  	ASSERT(xfs_dir2_sf_lookup(args) == -ENOENT);
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
> -	ASSERT(sfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  	/*
>  	 * Compute entry (and change in) size.
> @@ -855,9 +853,7 @@ xfs_dir2_sf_lookup(
>  	xfs_dir2_sf_check(args);
>  
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
> -	ASSERT(sfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  	/*
>  	 * Special case for .
> @@ -920,21 +916,19 @@ xfs_dir2_sf_removename(
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> +	int			oldsize = dp->i_disk_size;
>  	int			byteoff;	/* offset of removed entry */
>  	int			entsize;	/* this entry's size */
>  	int			i;		/* shortform entry index */
>  	int			newsize;	/* new inode size */
> -	int			oldsize;	/* old inode size */
>  	xfs_dir2_sf_entry_t	*sfep;		/* shortform directory entry */
>  
>  	trace_xfs_dir2_sf_removename(args);
>  
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	oldsize = (int)dp->i_disk_size;
> -	ASSERT(oldsize >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == oldsize);
> -	ASSERT(sfp != NULL);
>  	ASSERT(oldsize >= xfs_dir2_sf_hdr_size(sfp->i8count));
> +
>  	/*
>  	 * Loop over the old directory entries.
>  	 * Find the one we're deleting.
> @@ -1028,9 +1022,7 @@ xfs_dir2_sf_replace(
>  	trace_xfs_dir2_sf_replace(args);
>  
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
> -	ASSERT(sfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  
>  	/*
> -- 
> 2.39.2
> 
> 

[PATCH 04/16] xfs: remove an extra buffer allocation in xfs_dir2_sf_to_block

[Christoph Hellwig]

Make use of the new ability to call xfs_bmap_local_to_extents_empty on
a non-empty local fork to reuse the old inode fork data to build the
directory block to replace the local temporary buffer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_block.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 035a54dbdd7586..20d4e86e14ab08 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -1097,8 +1097,7 @@ xfs_dir2_sf_to_block(
 	int			newoffset;	/* offset from current entry */
 	unsigned int		offset = geo->data_entry_offset;
 	xfs_dir2_sf_entry_t	*sfep;		/* sf entry pointer */
-	struct xfs_dir2_sf_hdr	*oldsfp = ifp->if_data;
-	xfs_dir2_sf_hdr_t	*sfp;		/* shortform header  */
+	struct xfs_dir2_sf_hdr	*sfp = ifp->if_data;
 	__be16			*tagp;		/* end of data entry */
 	struct xfs_name		name;
 
@@ -1106,17 +1105,10 @@ xfs_dir2_sf_to_block(
 
 	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
 	ASSERT(ifp->if_bytes == dp->i_disk_size);
-	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(oldsfp->i8count));
+	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
 
-	/*
-	 * Copy the directory into a temporary buffer.
-	 * Then pitch the incore inode data so we can make extents.
-	 */
-	sfp = kmalloc(ifp->if_bytes, GFP_KERNEL | __GFP_NOFAIL);
-	memcpy(sfp, oldsfp, ifp->if_bytes);
+	sfp = xfs_bmap_local_to_extents_empty(tp, dp, XFS_DATA_FORK);
 
-	xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK);
-	xfs_bmap_local_to_extents_empty(tp, dp, XFS_DATA_FORK);
 	dp->i_disk_size = 0;
 
 	/*
-- 
2.39.2

Re: [PATCH 04/16] xfs: remove an extra buffer allocation in xfs_dir2_sf_to_block

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:14PM +0200, Christoph Hellwig wrote:
> Make use of the new ability to call xfs_bmap_local_to_extents_empty on
> a non-empty local fork to reuse the old inode fork data to build the
> directory block to replace the local temporary buffer.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks pretty straightforward,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_block.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 035a54dbdd7586..20d4e86e14ab08 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1097,8 +1097,7 @@ xfs_dir2_sf_to_block(
>  	int			newoffset;	/* offset from current entry */
>  	unsigned int		offset = geo->data_entry_offset;
>  	xfs_dir2_sf_entry_t	*sfep;		/* sf entry pointer */
> -	struct xfs_dir2_sf_hdr	*oldsfp = ifp->if_data;
> -	xfs_dir2_sf_hdr_t	*sfp;		/* shortform header  */
> +	struct xfs_dir2_sf_hdr	*sfp = ifp->if_data;
>  	__be16			*tagp;		/* end of data entry */
>  	struct xfs_name		name;
>  
> @@ -1106,17 +1105,10 @@ xfs_dir2_sf_to_block(
>  
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
>  	ASSERT(ifp->if_bytes == dp->i_disk_size);
> -	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(oldsfp->i8count));
> +	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  
> -	/*
> -	 * Copy the directory into a temporary buffer.
> -	 * Then pitch the incore inode data so we can make extents.
> -	 */
> -	sfp = kmalloc(ifp->if_bytes, GFP_KERNEL | __GFP_NOFAIL);
> -	memcpy(sfp, oldsfp, ifp->if_bytes);
> +	sfp = xfs_bmap_local_to_extents_empty(tp, dp, XFS_DATA_FORK);
>  
> -	xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK);
> -	xfs_bmap_local_to_extents_empty(tp, dp, XFS_DATA_FORK);
>  	dp->i_disk_size = 0;
>  
>  	/*
> -- 
> 2.39.2
> 
> 

[PATCH 05/16] xfs: move the "does it fit" check into xfs_dir2_block_to_sf

[Christoph Hellwig]

All callers of xfs_dir2_block_to_sf first check if the block format
directory would actually fit into the short format.  Move this code
into xfs_dir2_block_to_sf and rename the function to
xfs_dir2_try_block_to_sf.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_block.c | 24 ++----------------------
 fs/xfs/libxfs/xfs_dir2_priv.h  |  5 +----
 fs/xfs/libxfs/xfs_dir2_sf.c    | 25 +++++++++++++++++--------
 fs/xfs/libxfs/xfs_exchmaps.c   |  8 +-------
 4 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 20d4e86e14ab08..378d3aefdd9ced 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -795,8 +795,6 @@ xfs_dir2_block_removename(
 	int			error;		/* error return value */
 	int			needlog;	/* need to log block header */
 	int			needscan;	/* need to fixup bestfree */
-	xfs_dir2_sf_hdr_t	sfh;		/* shortform header */
-	int			size;		/* shortform size */
 	xfs_trans_t		*tp;		/* transaction pointer */
 
 	trace_xfs_dir2_block_removename(args);
@@ -845,17 +843,8 @@ xfs_dir2_block_removename(
 	if (needlog)
 		xfs_dir2_data_log_header(args, bp);
 	xfs_dir3_data_check(dp, bp);
-	/*
-	 * See if the size as a shortform is good enough.
-	 */
-	size = xfs_dir2_block_sfsize(dp, hdr, &sfh);
-	if (size > xfs_inode_data_fork_size(dp))
-		return 0;
 
-	/*
-	 * If it works, do the conversion.
-	 */
-	return xfs_dir2_block_to_sf(args, bp, size, &sfh);
+	return xfs_dir2_try_block_to_sf(args, bp);
 }
 
 /*
@@ -944,7 +933,6 @@ xfs_dir2_leaf_to_block(
 	xfs_mount_t		*mp;		/* file system mount point */
 	int			needlog;	/* need to log data header */
 	int			needscan;	/* need to scan for bestfree */
-	xfs_dir2_sf_hdr_t	sfh;		/* shortform header */
 	int			size;		/* bytes used */
 	__be16			*tagp;		/* end of entry (tag) */
 	int			to;		/* block/leaf to index */
@@ -1058,15 +1046,7 @@ xfs_dir2_leaf_to_block(
 	error = xfs_da_shrink_inode(args, args->geo->leafblk, lbp);
 	if (error)
 		return error;
-
-	/*
-	 * Now see if the resulting block can be shrunken to shortform.
-	 */
-	size = xfs_dir2_block_sfsize(dp, hdr, &sfh);
-	if (size > xfs_inode_data_fork_size(dp))
-		return 0;
-
-	return xfs_dir2_block_to_sf(args, dbp, size, &sfh);
+	return xfs_dir2_try_block_to_sf(args, dbp);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
index 3befb32509fa44..1e4401f9ec936e 100644
--- a/fs/xfs/libxfs/xfs_dir2_priv.h
+++ b/fs/xfs/libxfs/xfs_dir2_priv.h
@@ -167,10 +167,7 @@ uint8_t xfs_dir2_sf_get_ftype(struct xfs_mount *mp,
 		struct xfs_dir2_sf_entry *sfep);
 struct xfs_dir2_sf_entry *xfs_dir2_sf_nextentry(struct xfs_mount *mp,
 		struct xfs_dir2_sf_hdr *hdr, struct xfs_dir2_sf_entry *sfep);
-extern int xfs_dir2_block_sfsize(struct xfs_inode *dp,
-		struct xfs_dir2_data_hdr *block, struct xfs_dir2_sf_hdr *sfhp);
-extern int xfs_dir2_block_to_sf(struct xfs_da_args *args, struct xfs_buf *bp,
-		int size, xfs_dir2_sf_hdr_t *sfhp);
+int xfs_dir2_try_block_to_sf(struct xfs_da_args *args, struct xfs_buf *bp);
 extern int xfs_dir2_sf_addname(struct xfs_da_args *args);
 extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
 extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 1cd5228e1ce6af..fad3fd28175368 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -163,7 +163,7 @@ xfs_dir2_sf_put_ftype(
  * space currently present in the inode.  If it won't fit, the output
  * size is too big (but not accurate).
  */
-int						/* size for sf form */
+static int					/* size for sf form */
 xfs_dir2_block_sfsize(
 	xfs_inode_t		*dp,		/* incore inode pointer */
 	xfs_dir2_data_hdr_t	*hdr,		/* block directory data */
@@ -250,15 +250,12 @@ xfs_dir2_block_sfsize(
 }
 
 /*
- * Convert a block format directory to shortform.
- * Caller has already checked that it will fit, and built us a header.
+ * Try to convert a block format directory to shortform.
  */
 int						/* error */
-xfs_dir2_block_to_sf(
+xfs_dir2_try_block_to_sf(
 	struct xfs_da_args	*args,		/* operation arguments */
-	struct xfs_buf		*bp,
-	int			size,		/* shortform directory size */
-	struct xfs_dir2_sf_hdr	*sfhp)		/* shortform directory hdr */
+	struct xfs_buf		*bp)
 {
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
@@ -267,8 +264,20 @@ xfs_dir2_block_to_sf(
 	struct xfs_dir2_sf_entry *sfep;		/* shortform entry */
 	struct xfs_dir2_sf_hdr	*sfp;		/* shortform directory header */
 	unsigned int		offset = args->geo->data_entry_offset;
+	struct xfs_dir2_sf_hdr	sfh;
+	int			size;
 	unsigned int		end;
 
+	/*
+	 * See if it would fit into the shortform format.  If not we are done.
+	 */
+	size = xfs_dir2_block_sfsize(dp, bp->b_addr, &sfh);
+	if (size > xfs_inode_data_fork_size(dp))
+		return 0;
+
+	/*
+	 * It would fit into the shortform formt, do the conversion now.
+	 */
 	trace_xfs_dir2_block_to_sf(args);
 
 	/*
@@ -277,7 +286,7 @@ xfs_dir2_block_to_sf(
 	 * the block and copy the formatted data into the inode literal area.
 	 */
 	sfp = kmalloc(mp->m_sb.sb_inodesize, GFP_KERNEL | __GFP_NOFAIL);
-	memcpy(sfp, sfhp, xfs_dir2_sf_hdr_size(sfhp->i8count));
+	memcpy(sfp, &sfh, xfs_dir2_sf_hdr_size(sfh.i8count));
 
 	/*
 	 * Loop over the active and unused entries.  Stop when we reach the
diff --git a/fs/xfs/libxfs/xfs_exchmaps.c b/fs/xfs/libxfs/xfs_exchmaps.c
index 2021396651de27..bca6b6b0985464 100644
--- a/fs/xfs/libxfs/xfs_exchmaps.c
+++ b/fs/xfs/libxfs/xfs_exchmaps.c
@@ -463,9 +463,7 @@ xfs_exchmaps_dir_to_sf(
 		.trans		= tp,
 		.owner		= xmi->xmi_ip2->i_ino,
 	};
-	struct xfs_dir2_sf_hdr	sfh;
 	struct xfs_buf		*bp;
-	int			size;
 	int			error = 0;
 
 	if (xfs_dir2_format(&args, &error) != XFS_DIR2_FMT_BLOCK)
@@ -475,11 +473,7 @@ xfs_exchmaps_dir_to_sf(
 	if (error)
 		return error;
 
-	size = xfs_dir2_block_sfsize(xmi->xmi_ip2, bp->b_addr, &sfh);
-	if (size > xfs_inode_data_fork_size(xmi->xmi_ip2))
-		return 0;
-
-	return xfs_dir2_block_to_sf(&args, bp, size, &sfh);
+	return xfs_dir2_try_block_to_sf(&args, bp);
 }
 
 /* Convert inode2's remote symlink target back to shortform, if possible. */
-- 
2.39.2

Re: [PATCH 05/16] xfs: move the "does it fit" check into xfs_dir2_block_to_sf

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:15PM +0200, Christoph Hellwig wrote:
> All callers of xfs_dir2_block_to_sf first check if the block format
> directory would actually fit into the short format.  Move this code
> into xfs_dir2_block_to_sf and rename the function to
> xfs_dir2_try_block_to_sf.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_block.c | 24 ++----------------------
>  fs/xfs/libxfs/xfs_dir2_priv.h  |  5 +----
>  fs/xfs/libxfs/xfs_dir2_sf.c    | 25 +++++++++++++++++--------
>  fs/xfs/libxfs/xfs_exchmaps.c   |  8 +-------
>  4 files changed, 21 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 20d4e86e14ab08..378d3aefdd9ced 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -795,8 +795,6 @@ xfs_dir2_block_removename(
>  	int			error;		/* error return value */
>  	int			needlog;	/* need to log block header */
>  	int			needscan;	/* need to fixup bestfree */
> -	xfs_dir2_sf_hdr_t	sfh;		/* shortform header */
> -	int			size;		/* shortform size */
>  	xfs_trans_t		*tp;		/* transaction pointer */
>  
>  	trace_xfs_dir2_block_removename(args);
> @@ -845,17 +843,8 @@ xfs_dir2_block_removename(
>  	if (needlog)
>  		xfs_dir2_data_log_header(args, bp);
>  	xfs_dir3_data_check(dp, bp);
> -	/*
> -	 * See if the size as a shortform is good enough.
> -	 */
> -	size = xfs_dir2_block_sfsize(dp, hdr, &sfh);
> -	if (size > xfs_inode_data_fork_size(dp))
> -		return 0;
>  
> -	/*
> -	 * If it works, do the conversion.
> -	 */
> -	return xfs_dir2_block_to_sf(args, bp, size, &sfh);
> +	return xfs_dir2_try_block_to_sf(args, bp);
>  }
>  
>  /*
> @@ -944,7 +933,6 @@ xfs_dir2_leaf_to_block(
>  	xfs_mount_t		*mp;		/* file system mount point */
>  	int			needlog;	/* need to log data header */
>  	int			needscan;	/* need to scan for bestfree */
> -	xfs_dir2_sf_hdr_t	sfh;		/* shortform header */
>  	int			size;		/* bytes used */
>  	__be16			*tagp;		/* end of entry (tag) */
>  	int			to;		/* block/leaf to index */
> @@ -1058,15 +1046,7 @@ xfs_dir2_leaf_to_block(
>  	error = xfs_da_shrink_inode(args, args->geo->leafblk, lbp);
>  	if (error)
>  		return error;
> -
> -	/*
> -	 * Now see if the resulting block can be shrunken to shortform.
> -	 */
> -	size = xfs_dir2_block_sfsize(dp, hdr, &sfh);
> -	if (size > xfs_inode_data_fork_size(dp))
> -		return 0;
> -
> -	return xfs_dir2_block_to_sf(args, dbp, size, &sfh);
> +	return xfs_dir2_try_block_to_sf(args, dbp);
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> index 3befb32509fa44..1e4401f9ec936e 100644
> --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> @@ -167,10 +167,7 @@ uint8_t xfs_dir2_sf_get_ftype(struct xfs_mount *mp,
>  		struct xfs_dir2_sf_entry *sfep);
>  struct xfs_dir2_sf_entry *xfs_dir2_sf_nextentry(struct xfs_mount *mp,
>  		struct xfs_dir2_sf_hdr *hdr, struct xfs_dir2_sf_entry *sfep);
> -extern int xfs_dir2_block_sfsize(struct xfs_inode *dp,
> -		struct xfs_dir2_data_hdr *block, struct xfs_dir2_sf_hdr *sfhp);
> -extern int xfs_dir2_block_to_sf(struct xfs_da_args *args, struct xfs_buf *bp,
> -		int size, xfs_dir2_sf_hdr_t *sfhp);
> +int xfs_dir2_try_block_to_sf(struct xfs_da_args *args, struct xfs_buf *bp);
>  extern int xfs_dir2_sf_addname(struct xfs_da_args *args);
>  extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
>  extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 1cd5228e1ce6af..fad3fd28175368 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -163,7 +163,7 @@ xfs_dir2_sf_put_ftype(
>   * space currently present in the inode.  If it won't fit, the output
>   * size is too big (but not accurate).
>   */
> -int						/* size for sf form */
> +static int					/* size for sf form */
>  xfs_dir2_block_sfsize(
>  	xfs_inode_t		*dp,		/* incore inode pointer */
>  	xfs_dir2_data_hdr_t	*hdr,		/* block directory data */
> @@ -250,15 +250,12 @@ xfs_dir2_block_sfsize(
>  }
>  
>  /*
> - * Convert a block format directory to shortform.
> - * Caller has already checked that it will fit, and built us a header.
> + * Try to convert a block format directory to shortform.
>   */
>  int						/* error */
> -xfs_dir2_block_to_sf(
> +xfs_dir2_try_block_to_sf(
>  	struct xfs_da_args	*args,		/* operation arguments */
> -	struct xfs_buf		*bp,
> -	int			size,		/* shortform directory size */
> -	struct xfs_dir2_sf_hdr	*sfhp)		/* shortform directory hdr */
> +	struct xfs_buf		*bp)
>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
> @@ -267,8 +264,20 @@ xfs_dir2_block_to_sf(
>  	struct xfs_dir2_sf_entry *sfep;		/* shortform entry */
>  	struct xfs_dir2_sf_hdr	*sfp;		/* shortform directory header */
>  	unsigned int		offset = args->geo->data_entry_offset;
> +	struct xfs_dir2_sf_hdr	sfh;
> +	int			size;
>  	unsigned int		end;
>  
> +	/*
> +	 * See if it would fit into the shortform format.  If not we are done.
> +	 */
> +	size = xfs_dir2_block_sfsize(dp, bp->b_addr, &sfh);
> +	if (size > xfs_inode_data_fork_size(dp))
> +		return 0;
> +
> +	/*
> +	 * It would fit into the shortform formt, do the conversion now.
> +	 */
>  	trace_xfs_dir2_block_to_sf(args);
>  
>  	/*
> @@ -277,7 +286,7 @@ xfs_dir2_block_to_sf(
>  	 * the block and copy the formatted data into the inode literal area.
>  	 */
>  	sfp = kmalloc(mp->m_sb.sb_inodesize, GFP_KERNEL | __GFP_NOFAIL);
> -	memcpy(sfp, sfhp, xfs_dir2_sf_hdr_size(sfhp->i8count));
> +	memcpy(sfp, &sfh, xfs_dir2_sf_hdr_size(sfh.i8count));
>  
>  	/*
>  	 * Loop over the active and unused entries.  Stop when we reach the
> diff --git a/fs/xfs/libxfs/xfs_exchmaps.c b/fs/xfs/libxfs/xfs_exchmaps.c
> index 2021396651de27..bca6b6b0985464 100644
> --- a/fs/xfs/libxfs/xfs_exchmaps.c
> +++ b/fs/xfs/libxfs/xfs_exchmaps.c
> @@ -463,9 +463,7 @@ xfs_exchmaps_dir_to_sf(
>  		.trans		= tp,
>  		.owner		= xmi->xmi_ip2->i_ino,
>  	};
> -	struct xfs_dir2_sf_hdr	sfh;
>  	struct xfs_buf		*bp;
> -	int			size;
>  	int			error = 0;
>  
>  	if (xfs_dir2_format(&args, &error) != XFS_DIR2_FMT_BLOCK)
> @@ -475,11 +473,7 @@ xfs_exchmaps_dir_to_sf(
>  	if (error)
>  		return error;
>  
> -	size = xfs_dir2_block_sfsize(xmi->xmi_ip2, bp->b_addr, &sfh);
> -	if (size > xfs_inode_data_fork_size(xmi->xmi_ip2))
> -		return 0;
> -
> -	return xfs_dir2_block_to_sf(&args, bp, size, &sfh);
> +	return xfs_dir2_try_block_to_sf(&args, bp);
>  }
>  
>  /* Convert inode2's remote symlink target back to shortform, if possible. */
> -- 
> 2.39.2
> 
> 

[PATCH 06/16] xfs: remove the buffer allocation size in xfs_dir2_try_block_to_sf

[Christoph Hellwig]

We've already calculated the size of the short form directory in the
size variable when checking if it fits.  Use that for the temporary
buffer allocation instead of overallocating to the inode size.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index fad3fd28175368..d02f1ddb1da92c 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -281,11 +281,11 @@ xfs_dir2_try_block_to_sf(
 	trace_xfs_dir2_block_to_sf(args);
 
 	/*
-	 * Allocate a temporary destination buffer the size of the inode to
-	 * format the data into.  Once we have formatted the data, we can free
-	 * the block and copy the formatted data into the inode literal area.
+	 * Allocate a temporary destination buffer to format the data into.
+	 * Once we have formatted the data, we can free the block and copy the
+	 * formatted data into the inode literal area.
 	 */
-	sfp = kmalloc(mp->m_sb.sb_inodesize, GFP_KERNEL | __GFP_NOFAIL);
+	sfp = kmalloc(size, GFP_KERNEL | __GFP_NOFAIL);
 	memcpy(sfp, &sfh, xfs_dir2_sf_hdr_size(sfh.i8count));
 
 	/*
-- 
2.39.2

Re: [PATCH 06/16] xfs: remove the buffer allocation size in xfs_dir2_try_block_to_sf

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:16PM +0200, Christoph Hellwig wrote:
> We've already calculated the size of the short form directory in the
> size variable when checking if it fits.  Use that for the temporary
> buffer allocation instead of overallocating to the inode size.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Makes sense,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index fad3fd28175368..d02f1ddb1da92c 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -281,11 +281,11 @@ xfs_dir2_try_block_to_sf(
>  	trace_xfs_dir2_block_to_sf(args);
>  
>  	/*
> -	 * Allocate a temporary destination buffer the size of the inode to
> -	 * format the data into.  Once we have formatted the data, we can free
> -	 * the block and copy the formatted data into the inode literal area.
> +	 * Allocate a temporary destination buffer to format the data into.
> +	 * Once we have formatted the data, we can free the block and copy the
> +	 * formatted data into the inode literal area.
>  	 */
> -	sfp = kmalloc(mp->m_sb.sb_inodesize, GFP_KERNEL | __GFP_NOFAIL);
> +	sfp = kmalloc(size, GFP_KERNEL | __GFP_NOFAIL);
>  	memcpy(sfp, &sfh, xfs_dir2_sf_hdr_size(sfh.i8count));
>  
>  	/*
> -- 
> 2.39.2
> 
> 

[PATCH 07/16] xfs: remove a superfluous memory allocation in xfs_dir2_block_to_sf

[Christoph Hellwig]

Transfer the temporary buffer allocation to the inode fork instead of
copying to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index d02f1ddb1da92c..164ae1684816b6 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -281,9 +281,8 @@ xfs_dir2_try_block_to_sf(
 	trace_xfs_dir2_block_to_sf(args);
 
 	/*
-	 * Allocate a temporary destination buffer to format the data into.
-	 * Once we have formatted the data, we can free the block and copy the
-	 * formatted data into the inode literal area.
+	 * Allocate the shortform buffer now.  It will be transferred to the
+	 * inode fork once we are done.
 	 */
 	sfp = kmalloc(size, GFP_KERNEL | __GFP_NOFAIL);
 	memcpy(sfp, &sfh, xfs_dir2_sf_hdr_size(sfh.i8count));
@@ -341,25 +340,23 @@ xfs_dir2_try_block_to_sf(
 	error = xfs_dir2_shrink_inode(args, args->geo->datablk, bp);
 	if (error) {
 		ASSERT(error != -ENOSPC);
+		kfree(sfp);
 		goto out;
 	}
 
 	/*
-	 * The buffer is now unconditionally gone, whether
-	 * xfs_dir2_shrink_inode worked or not.
-	 *
-	 * Convert the inode to local format and copy the data in.
+	 * Update the data fork format and transfer buffer ownership to the
+	 * inode fork.
 	 */
-	ASSERT(dp->i_df.if_bytes == 0);
-	xfs_init_local_fork(dp, XFS_DATA_FORK, sfp, size);
 	dp->i_df.if_format = XFS_DINODE_FMT_LOCAL;
+	dp->i_df.if_data = sfp;
+	dp->i_df.if_bytes = size;
 	dp->i_disk_size = size;
 
 	logflags |= XFS_ILOG_DDATA;
 	xfs_dir2_sf_check(args);
 out:
 	xfs_trans_log_inode(args->trans, dp, logflags);
-	kfree(sfp);
 	return error;
 }
 
-- 
2.39.2

Re: [PATCH 07/16] xfs: remove a superfluous memory allocation in xfs_dir2_block_to_sf

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:17PM +0200, Christoph Hellwig wrote:
> Transfer the temporary buffer allocation to the inode fork instead of
> copying to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index d02f1ddb1da92c..164ae1684816b6 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -281,9 +281,8 @@ xfs_dir2_try_block_to_sf(
>  	trace_xfs_dir2_block_to_sf(args);
>  
>  	/*
> -	 * Allocate a temporary destination buffer to format the data into.
> -	 * Once we have formatted the data, we can free the block and copy the
> -	 * formatted data into the inode literal area.
> +	 * Allocate the shortform buffer now.  It will be transferred to the
> +	 * inode fork once we are done.
>  	 */
>  	sfp = kmalloc(size, GFP_KERNEL | __GFP_NOFAIL);
>  	memcpy(sfp, &sfh, xfs_dir2_sf_hdr_size(sfh.i8count));
> @@ -341,25 +340,23 @@ xfs_dir2_try_block_to_sf(
>  	error = xfs_dir2_shrink_inode(args, args->geo->datablk, bp);
>  	if (error) {
>  		ASSERT(error != -ENOSPC);
> +		kfree(sfp);
>  		goto out;
>  	}
>  
>  	/*
> -	 * The buffer is now unconditionally gone, whether
> -	 * xfs_dir2_shrink_inode worked or not.
> -	 *
> -	 * Convert the inode to local format and copy the data in.
> +	 * Update the data fork format and transfer buffer ownership to the
> +	 * inode fork.
>  	 */
> -	ASSERT(dp->i_df.if_bytes == 0);
> -	xfs_init_local_fork(dp, XFS_DATA_FORK, sfp, size);
>  	dp->i_df.if_format = XFS_DINODE_FMT_LOCAL;
> +	dp->i_df.if_data = sfp;
> +	dp->i_df.if_bytes = size;
>  	dp->i_disk_size = size;
>  
>  	logflags |= XFS_ILOG_DDATA;
>  	xfs_dir2_sf_check(args);
>  out:
>  	xfs_trans_log_inode(args->trans, dp, logflags);
> -	kfree(sfp);
>  	return error;
>  }
>  
> -- 
> 2.39.2
> 
> 

[PATCH 08/16] xfs: remove a superfluous memory allocation in xfs_dir2_sf_toino8

[Christoph Hellwig]

Transfer the temporary buffer allocation to the inode fork instead of
copying to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 164ae1684816b6..87552c01260a1c 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -1205,36 +1205,25 @@ xfs_dir2_sf_toino8(
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
-	char			*buf;		/* old dir's buffer */
+	int			oldsize = dp->i_df.if_bytes;
 	int			i;		/* entry index */
 	int			newsize;	/* new inode size */
 	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
-	int			oldsize;	/* old inode size */
 	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
 	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
 
 	trace_xfs_dir2_sf_toino8(args);
 
-	/*
-	 * Copy the old directory to the buffer.
-	 * Then nuke it from the inode, and add the new buffer to the inode.
-	 * Don't want xfs_idata_realloc copying the data here.
-	 */
-	oldsize = dp->i_df.if_bytes;
-	buf = kmalloc(oldsize, GFP_KERNEL | __GFP_NOFAIL);
 	ASSERT(oldsfp->i8count == 0);
-	memcpy(buf, oldsfp, oldsize);
+
 	/*
 	 * Compute the new inode size (nb: entry count + 1 for parent)
 	 */
 	newsize = oldsize + (oldsfp->count + 1) * XFS_INO64_DIFF;
-	xfs_idata_realloc(dp, -oldsize, XFS_DATA_FORK);
-	xfs_idata_realloc(dp, newsize, XFS_DATA_FORK);
-	/*
-	 * Reset our pointers, the data has moved.
-	 */
-	oldsfp = (xfs_dir2_sf_hdr_t *)buf;
-	sfp = dp->i_df.if_data;
+
+	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
+	dp->i_df.if_bytes = newsize;
+
 	/*
 	 * Fill in the new header.
 	 */
@@ -1257,10 +1246,8 @@ xfs_dir2_sf_toino8(
 		xfs_dir2_sf_put_ftype(mp, sfep,
 				xfs_dir2_sf_get_ftype(mp, oldsfep));
 	}
-	/*
-	 * Clean up the inode.
-	 */
-	kfree(buf);
+
+	kfree(oldsfp);
 	dp->i_disk_size = newsize;
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
 }
-- 
2.39.2

Re: [PATCH 08/16] xfs: remove a superfluous memory allocation in xfs_dir2_sf_toino8

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:18PM +0200, Christoph Hellwig wrote:
> Transfer the temporary buffer allocation to the inode fork instead of
> copying to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 164ae1684816b6..87552c01260a1c 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -1205,36 +1205,25 @@ xfs_dir2_sf_toino8(
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
> -	char			*buf;		/* old dir's buffer */
> +	int			oldsize = dp->i_df.if_bytes;
>  	int			i;		/* entry index */
>  	int			newsize;	/* new inode size */
>  	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
> -	int			oldsize;	/* old inode size */
>  	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
>  	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
>  
>  	trace_xfs_dir2_sf_toino8(args);
>  
> -	/*
> -	 * Copy the old directory to the buffer.
> -	 * Then nuke it from the inode, and add the new buffer to the inode.
> -	 * Don't want xfs_idata_realloc copying the data here.
> -	 */
> -	oldsize = dp->i_df.if_bytes;
> -	buf = kmalloc(oldsize, GFP_KERNEL | __GFP_NOFAIL);
>  	ASSERT(oldsfp->i8count == 0);
> -	memcpy(buf, oldsfp, oldsize);
> +
>  	/*
>  	 * Compute the new inode size (nb: entry count + 1 for parent)
>  	 */
>  	newsize = oldsize + (oldsfp->count + 1) * XFS_INO64_DIFF;
> -	xfs_idata_realloc(dp, -oldsize, XFS_DATA_FORK);
> -	xfs_idata_realloc(dp, newsize, XFS_DATA_FORK);
> -	/*
> -	 * Reset our pointers, the data has moved.
> -	 */
> -	oldsfp = (xfs_dir2_sf_hdr_t *)buf;
> -	sfp = dp->i_df.if_data;
> +
> +	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
> +	dp->i_df.if_bytes = newsize;

I sure am glad we never implemented inline data for regular files on
xfs,

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
>  	/*
>  	 * Fill in the new header.
>  	 */
> @@ -1257,10 +1246,8 @@ xfs_dir2_sf_toino8(
>  		xfs_dir2_sf_put_ftype(mp, sfep,
>  				xfs_dir2_sf_get_ftype(mp, oldsfep));
>  	}
> -	/*
> -	 * Clean up the inode.
> -	 */
> -	kfree(buf);
> +
> +	kfree(oldsfp);
>  	dp->i_disk_size = newsize;
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
>  }
> -- 
> 2.39.2
> 
> 

[PATCH 09/16] xfs: remove a superfluous memory allocation in xfs_dir2_sf_toino4

[Christoph Hellwig]

Transfer the temporary buffer allocation to the inode fork instead of
copying to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 87552c01260a1c..3b6d6dda92f29f 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -1133,36 +1133,25 @@ xfs_dir2_sf_toino4(
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
-	char			*buf;		/* old dir's buffer */
+	int			oldsize = dp->i_df.if_bytes;
 	int			i;		/* entry index */
 	int			newsize;	/* new inode size */
 	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
-	int			oldsize;	/* old inode size */
 	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
 	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
 
 	trace_xfs_dir2_sf_toino4(args);
 
-	/*
-	 * Copy the old directory to the buffer.
-	 * Then nuke it from the inode, and add the new buffer to the inode.
-	 * Don't want xfs_idata_realloc copying the data here.
-	 */
-	oldsize = dp->i_df.if_bytes;
-	buf = kmalloc(oldsize, GFP_KERNEL | __GFP_NOFAIL);
 	ASSERT(oldsfp->i8count == 1);
-	memcpy(buf, oldsfp, oldsize);
+
 	/*
 	 * Compute the new inode size.
 	 */
 	newsize = oldsize - (oldsfp->count + 1) * XFS_INO64_DIFF;
-	xfs_idata_realloc(dp, -oldsize, XFS_DATA_FORK);
-	xfs_idata_realloc(dp, newsize, XFS_DATA_FORK);
-	/*
-	 * Reset our pointers, the data has moved.
-	 */
-	oldsfp = (xfs_dir2_sf_hdr_t *)buf;
-	sfp = dp->i_df.if_data;
+
+	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
+	dp->i_df.if_bytes = newsize;
+
 	/*
 	 * Fill in the new header.
 	 */
@@ -1185,10 +1174,8 @@ xfs_dir2_sf_toino4(
 		xfs_dir2_sf_put_ftype(mp, sfep,
 				xfs_dir2_sf_get_ftype(mp, oldsfep));
 	}
-	/*
-	 * Clean up the inode.
-	 */
-	kfree(buf);
+
+	kfree(oldsfp);
 	dp->i_disk_size = newsize;
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
 }
-- 
2.39.2

Re: [PATCH 09/16] xfs: remove a superfluous memory allocation in xfs_dir2_sf_toino4

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:19PM +0200, Christoph Hellwig wrote:
> Transfer the temporary buffer allocation to the inode fork instead of
> copying to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks decent,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 87552c01260a1c..3b6d6dda92f29f 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -1133,36 +1133,25 @@ xfs_dir2_sf_toino4(
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
> -	char			*buf;		/* old dir's buffer */
> +	int			oldsize = dp->i_df.if_bytes;
>  	int			i;		/* entry index */
>  	int			newsize;	/* new inode size */
>  	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
> -	int			oldsize;	/* old inode size */
>  	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
>  	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
>  
>  	trace_xfs_dir2_sf_toino4(args);
>  
> -	/*
> -	 * Copy the old directory to the buffer.
> -	 * Then nuke it from the inode, and add the new buffer to the inode.
> -	 * Don't want xfs_idata_realloc copying the data here.
> -	 */
> -	oldsize = dp->i_df.if_bytes;
> -	buf = kmalloc(oldsize, GFP_KERNEL | __GFP_NOFAIL);
>  	ASSERT(oldsfp->i8count == 1);
> -	memcpy(buf, oldsfp, oldsize);
> +
>  	/*
>  	 * Compute the new inode size.
>  	 */
>  	newsize = oldsize - (oldsfp->count + 1) * XFS_INO64_DIFF;
> -	xfs_idata_realloc(dp, -oldsize, XFS_DATA_FORK);
> -	xfs_idata_realloc(dp, newsize, XFS_DATA_FORK);
> -	/*
> -	 * Reset our pointers, the data has moved.
> -	 */
> -	oldsfp = (xfs_dir2_sf_hdr_t *)buf;
> -	sfp = dp->i_df.if_data;
> +
> +	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
> +	dp->i_df.if_bytes = newsize;
> +
>  	/*
>  	 * Fill in the new header.
>  	 */
> @@ -1185,10 +1174,8 @@ xfs_dir2_sf_toino4(
>  		xfs_dir2_sf_put_ftype(mp, sfep,
>  				xfs_dir2_sf_get_ftype(mp, oldsfep));
>  	}
> -	/*
> -	 * Clean up the inode.
> -	 */
> -	kfree(buf);
> +
> +	kfree(oldsfp);
>  	dp->i_disk_size = newsize;
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
>  }
> -- 
> 2.39.2
> 
> 

[PATCH 10/16] xfs: optimize removing the last 8-byte inode from a shortform directory

[Christoph Hellwig]

When removing the last 8-byte inode, xfs_dir2_sf_removename calls
xfs_dir2_sf_toino4 after removing the entry.  This is rather inefficient
as it causes two buffer realloacations.  Instead of that pass a bool
argument to xfs_dir2_sf_toino4 so that it can remove the entry pointed
to by args as part of the conversion and use that to shortcut the
process.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 41 ++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 3b6d6dda92f29f..21e04594606b89 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -34,7 +34,7 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args);
 #define	xfs_dir2_sf_check(args)
 #endif /* DEBUG */
 
-static void xfs_dir2_sf_toino4(xfs_da_args_t *args);
+static void xfs_dir2_sf_toino4(struct xfs_da_args *args, bool remove);
 static void xfs_dir2_sf_toino8(xfs_da_args_t *args);
 
 int
@@ -935,6 +935,15 @@ xfs_dir2_sf_removename(
 	ASSERT(dp->i_df.if_bytes == oldsize);
 	ASSERT(oldsize >= xfs_dir2_sf_hdr_size(sfp->i8count));
 
+	/*
+	 * If this is the last 8-byte, directly convert to the 4-byte format
+	 * and just skip the removed entry when building the new fork.
+	 */
+	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM && sfp->i8count == 1) {
+		xfs_dir2_sf_toino4(args, true);
+		return 0;
+	}
+
 	/*
 	 * Loop over the old directory entries.
 	 * Find the one we're deleting.
@@ -980,10 +989,8 @@ xfs_dir2_sf_removename(
 	 * Are we changing inode number size?
 	 */
 	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM) {
-		if (sfp->i8count == 1)
-			xfs_dir2_sf_toino4(args);
-		else
-			sfp->i8count--;
+		ASSERT(sfp->i8count > 1);
+		sfp->i8count--;
 	}
 	xfs_dir2_sf_check(args);
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
@@ -1087,7 +1094,7 @@ xfs_dir2_sf_replace(
 		if (i == sfp->count) {
 			ASSERT(args->op_flags & XFS_DA_OP_OKNOENT);
 			if (i8elevated)
-				xfs_dir2_sf_toino4(args);
+				xfs_dir2_sf_toino4(args, false);
 			return -ENOENT;
 		}
 	}
@@ -1100,7 +1107,7 @@ xfs_dir2_sf_replace(
 		 * And the old count was one, so need to convert to small.
 		 */
 		if (sfp->i8count == 1)
-			xfs_dir2_sf_toino4(args);
+			xfs_dir2_sf_toino4(args, false);
 		else
 			sfp->i8count--;
 	}
@@ -1128,7 +1135,8 @@ xfs_dir2_sf_replace(
  */
 static void
 xfs_dir2_sf_toino4(
-	xfs_da_args_t		*args)		/* operation arguments */
+	struct xfs_da_args	*args,
+	bool			remove)
 {
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
@@ -1148,6 +1156,8 @@ xfs_dir2_sf_toino4(
 	 * Compute the new inode size.
 	 */
 	newsize = oldsize - (oldsfp->count + 1) * XFS_INO64_DIFF;
+	if (remove)
+		newsize -= xfs_dir2_sf_entsize(mp, oldsfp, args->namelen);
 
 	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
 	dp->i_df.if_bytes = newsize;
@@ -1166,11 +1176,22 @@ xfs_dir2_sf_toino4(
 	     i < sfp->count;
 	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
 		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
+		xfs_ino_t ino = xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep);
+
+		/*
+		 * Just skip over the entry that is removed if there is one.
+		 */
+		if (remove && args->inumber == ino) {
+			oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep);
+			sfp->count--;
+			if (++i == sfp->count)
+				break;
+		}
+
 		sfep->namelen = oldsfep->namelen;
 		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
 		memcpy(sfep->name, oldsfep->name, sfep->namelen);
-		xfs_dir2_sf_put_ino(mp, sfp, sfep,
-				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
+		xfs_dir2_sf_put_ino(mp, sfp, sfep, ino);
 		xfs_dir2_sf_put_ftype(mp, sfep,
 				xfs_dir2_sf_get_ftype(mp, oldsfep));
 	}
-- 
2.39.2

Re: [PATCH 10/16] xfs: optimize removing the last 8-byte inode from a shortform directory

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:20PM +0200, Christoph Hellwig wrote:
> When removing the last 8-byte inode, xfs_dir2_sf_removename calls
> xfs_dir2_sf_toino4 after removing the entry.  This is rather inefficient
> as it causes two buffer realloacations.  Instead of that pass a bool
> argument to xfs_dir2_sf_toino4 so that it can remove the entry pointed
> to by args as part of the conversion and use that to shortcut the
> process.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 41 ++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 3b6d6dda92f29f..21e04594606b89 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -34,7 +34,7 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args);
>  #define	xfs_dir2_sf_check(args)
>  #endif /* DEBUG */
>  
> -static void xfs_dir2_sf_toino4(xfs_da_args_t *args);
> +static void xfs_dir2_sf_toino4(struct xfs_da_args *args, bool remove);
>  static void xfs_dir2_sf_toino8(xfs_da_args_t *args);
>  
>  int
> @@ -935,6 +935,15 @@ xfs_dir2_sf_removename(
>  	ASSERT(dp->i_df.if_bytes == oldsize);
>  	ASSERT(oldsize >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  
> +	/*
> +	 * If this is the last 8-byte, directly convert to the 4-byte format
> +	 * and just skip the removed entry when building the new fork.
> +	 */
> +	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM && sfp->i8count == 1) {
> +		xfs_dir2_sf_toino4(args, true);
> +		return 0;
> +	}
> +
>  	/*
>  	 * Loop over the old directory entries.
>  	 * Find the one we're deleting.
> @@ -980,10 +989,8 @@ xfs_dir2_sf_removename(
>  	 * Are we changing inode number size?
>  	 */
>  	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM) {
> -		if (sfp->i8count == 1)
> -			xfs_dir2_sf_toino4(args);
> -		else
> -			sfp->i8count--;
> +		ASSERT(sfp->i8count > 1);
> +		sfp->i8count--;
>  	}
>  	xfs_dir2_sf_check(args);
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
> @@ -1087,7 +1094,7 @@ xfs_dir2_sf_replace(
>  		if (i == sfp->count) {
>  			ASSERT(args->op_flags & XFS_DA_OP_OKNOENT);
>  			if (i8elevated)
> -				xfs_dir2_sf_toino4(args);
> +				xfs_dir2_sf_toino4(args, false);
>  			return -ENOENT;
>  		}
>  	}
> @@ -1100,7 +1107,7 @@ xfs_dir2_sf_replace(
>  		 * And the old count was one, so need to convert to small.
>  		 */
>  		if (sfp->i8count == 1)
> -			xfs_dir2_sf_toino4(args);
> +			xfs_dir2_sf_toino4(args, false);
>  		else
>  			sfp->i8count--;
>  	}
> @@ -1128,7 +1135,8 @@ xfs_dir2_sf_replace(
>   */
>  static void
>  xfs_dir2_sf_toino4(
> -	xfs_da_args_t		*args)		/* operation arguments */
> +	struct xfs_da_args	*args,
> +	bool			remove)
>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
> @@ -1148,6 +1156,8 @@ xfs_dir2_sf_toino4(
>  	 * Compute the new inode size.
>  	 */
>  	newsize = oldsize - (oldsfp->count + 1) * XFS_INO64_DIFF;
> +	if (remove)
> +		newsize -= xfs_dir2_sf_entsize(mp, oldsfp, args->namelen);
>  
>  	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
>  	dp->i_df.if_bytes = newsize;
> @@ -1166,11 +1176,22 @@ xfs_dir2_sf_toino4(
>  	     i < sfp->count;
>  	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
>  		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
> +		xfs_ino_t ino = xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep);
> +
> +		/*
> +		 * Just skip over the entry that is removed if there is one.
> +		 */
> +		if (remove && args->inumber == ino) {
> +			oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep);
> +			sfp->count--;
> +			if (++i == sfp->count)
> +				break;
> +		}

What happens if a shortform directory contains two entries to the same
file?  I think @remove really means that the caller has verified that
the sf directory only contains one link @args->inumber?  If that's so,
then the comment for this function should say that.

--D

> +
>  		sfep->namelen = oldsfep->namelen;
>  		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
>  		memcpy(sfep->name, oldsfep->name, sfep->namelen);
> -		xfs_dir2_sf_put_ino(mp, sfp, sfep,
> -				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> +		xfs_dir2_sf_put_ino(mp, sfp, sfep, ino);
>  		xfs_dir2_sf_put_ftype(mp, sfep,
>  				xfs_dir2_sf_get_ftype(mp, oldsfep));
>  	}
> -- 
> 2.39.2
> 
> 

Re: [PATCH 10/16] xfs: optimize removing the last 8-byte inode from a shortform directory

[Christoph Hellwig]

On Wed, May 01, 2024 at 02:25:19PM -0700, Darrick J. Wong wrote:
> What happens if a shortform directory contains two entries to the same
> file?  I think @remove really means that the caller has verified that
> the sf directory only contains one link @args->inumber?  If that's so,
> then the comment for this function should say that.

No, I don't think there is such a validation.  But given that i8count is
1 and the entry we are removing has a 4-byte inode when we call this,
we can't by definition have two entries pointing to this inode, as then
i8count would be 2.

But I only thought about that now, and is is far to subtle (and I need
to check if the above is true for whiteouts).  I'll either clearly
document it, or change it to a name comparison.

[PATCH 11/16] xfs: add xfs_dir2_block_overhead helper

[Christoph Hellwig]

Add a common helper for the calculation of the overhead when converting
a shortform to block format directory.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_block.c |  4 ++--
 fs/xfs/libxfs/xfs_dir2_priv.h  | 12 ++++++++++++
 fs/xfs/libxfs/xfs_dir2_sf.c    | 12 ++++--------
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 378d3aefdd9ced..a19744cb43ca0c 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -1109,8 +1109,8 @@ xfs_dir2_sf_to_block(
 	/*
 	 * Compute size of block "tail" area.
 	 */
-	i = (uint)sizeof(*btp) +
-	    (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t);
+	i = xfs_dir2_block_overhead(sfp->count);
+
 	/*
 	 * The whole thing is initialized to free by the init routine.
 	 * Say we're using the leaf and tail area.
diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
index 1e4401f9ec936e..bfbc73251f275a 100644
--- a/fs/xfs/libxfs/xfs_dir2_priv.h
+++ b/fs/xfs/libxfs/xfs_dir2_priv.h
@@ -205,4 +205,16 @@ xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp,
 enum xfs_dacmp xfs_dir2_compname(struct xfs_da_args *args,
 		const unsigned char *name, int len);
 
+/*
+ * Overhead if we converted a shortform directory to block format.
+ *
+ * The extra two entries are because "." and ".." don't have real entries in
+ * the shortform format.
+ */
+static inline unsigned int xfs_dir2_block_overhead(unsigned int count)
+{
+	return (count + 2) * sizeof(struct xfs_dir2_leaf_entry) +
+		sizeof(struct xfs_dir2_block_tail);
+}
+
 #endif /* __XFS_DIR2_PRIV_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 21e04594606b89..1e1dcdf83b8f95 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -629,9 +629,7 @@ xfs_dir2_sf_addname_pick(
 	 * Calculate data bytes used excluding the new entry, if this
 	 * was a data block (block form directory).
 	 */
-	used = offset +
-	       (sfp->count + 3) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
-	       (uint)sizeof(xfs_dir2_block_tail_t);
+	used = offset + xfs_dir2_block_overhead(sfp->count + 1);
 	/*
 	 * If it won't fit in a block form then we can't insert it,
 	 * we'll go back, convert to block, then try the insert and convert
@@ -691,9 +689,7 @@ xfs_dir2_sf_check(
 	}
 	ASSERT(i8count == sfp->i8count);
 	ASSERT((char *)sfep - (char *)sfp == dp->i_disk_size);
-	ASSERT(offset +
-	       (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
-	       (uint)sizeof(xfs_dir2_block_tail_t) <= args->geo->blksize);
+	ASSERT(offset + xfs_dir2_block_overhead(sfp->count));
 }
 #endif	/* DEBUG */
 
@@ -782,8 +778,8 @@ xfs_dir2_sf_verify(
 		return __this_address;
 
 	/* Make sure this whole thing ought to be in local format. */
-	if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
-	    (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
+	if (offset + xfs_dir2_block_overhead(sfp->count) >
+	    mp->m_dir_geo->blksize)
 		return __this_address;
 
 	return NULL;
-- 
2.39.2

Re: [PATCH 11/16] xfs: add xfs_dir2_block_overhead helper

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:21PM +0200, Christoph Hellwig wrote:
> Add a common helper for the calculation of the overhead when converting
> a shortform to block format directory.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_block.c |  4 ++--
>  fs/xfs/libxfs/xfs_dir2_priv.h  | 12 ++++++++++++
>  fs/xfs/libxfs/xfs_dir2_sf.c    | 12 ++++--------
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 378d3aefdd9ced..a19744cb43ca0c 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1109,8 +1109,8 @@ xfs_dir2_sf_to_block(
>  	/*
>  	 * Compute size of block "tail" area.
>  	 */
> -	i = (uint)sizeof(*btp) +
> -	    (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t);
> +	i = xfs_dir2_block_overhead(sfp->count);
> +
>  	/*
>  	 * The whole thing is initialized to free by the init routine.
>  	 * Say we're using the leaf and tail area.
> diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> index 1e4401f9ec936e..bfbc73251f275a 100644
> --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> @@ -205,4 +205,16 @@ xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp,
>  enum xfs_dacmp xfs_dir2_compname(struct xfs_da_args *args,
>  		const unsigned char *name, int len);
>  
> +/*
> + * Overhead if we converted a shortform directory to block format.
> + *
> + * The extra two entries are because "." and ".." don't have real entries in
> + * the shortform format.
> + */
> +static inline unsigned int xfs_dir2_block_overhead(unsigned int count)
> +{
> +	return (count + 2) * sizeof(struct xfs_dir2_leaf_entry) +
> +		sizeof(struct xfs_dir2_block_tail);

I could've sworn there's a helper to compute this, but as I cannot find
it I guess I'll let that go.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +}
> +
>  #endif /* __XFS_DIR2_PRIV_H__ */
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 21e04594606b89..1e1dcdf83b8f95 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -629,9 +629,7 @@ xfs_dir2_sf_addname_pick(
>  	 * Calculate data bytes used excluding the new entry, if this
>  	 * was a data block (block form directory).
>  	 */
> -	used = offset +
> -	       (sfp->count + 3) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> -	       (uint)sizeof(xfs_dir2_block_tail_t);
> +	used = offset + xfs_dir2_block_overhead(sfp->count + 1);
>  	/*
>  	 * If it won't fit in a block form then we can't insert it,
>  	 * we'll go back, convert to block, then try the insert and convert
> @@ -691,9 +689,7 @@ xfs_dir2_sf_check(
>  	}
>  	ASSERT(i8count == sfp->i8count);
>  	ASSERT((char *)sfep - (char *)sfp == dp->i_disk_size);
> -	ASSERT(offset +
> -	       (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> -	       (uint)sizeof(xfs_dir2_block_tail_t) <= args->geo->blksize);
> +	ASSERT(offset + xfs_dir2_block_overhead(sfp->count));
>  }
>  #endif	/* DEBUG */
>  
> @@ -782,8 +778,8 @@ xfs_dir2_sf_verify(
>  		return __this_address;
>  
>  	/* Make sure this whole thing ought to be in local format. */
> -	if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> -	    (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> +	if (offset + xfs_dir2_block_overhead(sfp->count) >
> +	    mp->m_dir_geo->blksize)
>  		return __this_address;
>  
>  	return NULL;
> -- 
> 2.39.2
> 
> 

Re: [PATCH 11/16] xfs: add xfs_dir2_block_overhead helper

[Christoph Hellwig]

On Wed, May 01, 2024 at 02:27:58PM -0700, Darrick J. Wong wrote:
> I could've sworn there's a helper to compute this, but as I cannot find
> it I guess I'll let that go.

I did a fair amount of a advanced grepping (TM) and couldn't one.

[PATCH 12/16] xfs: factor out a xfs_dir2_sf_addname_common helper

[Christoph Hellwig]

Move the common code between the easy and hard cases into a common helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 48 +++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 1e1dcdf83b8f95..43e1090082b45d 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -360,6 +360,27 @@ xfs_dir2_try_block_to_sf(
 	return error;
 }
 
+static void
+xfs_dir2_sf_addname_common(
+	struct xfs_da_args	*args,
+	struct xfs_dir2_sf_entry *sfep,
+	xfs_dir2_data_aoff_t	offset,
+	bool			objchange)
+{
+	struct xfs_inode	*dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
+
+	sfep->namelen = args->namelen;
+	xfs_dir2_sf_put_offset(sfep, offset);
+	memcpy(sfep->name, args->name, sfep->namelen);
+	xfs_dir2_sf_put_ino(mp, sfp, sfep, args->inumber);
+	xfs_dir2_sf_put_ftype(mp, sfep, args->filetype);
+	sfp->count++;
+	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM && !objchange)
+		sfp->i8count++;
+}
+
 /*
  * Add a name to a shortform directory.
  * There are two algorithms, "easy" and "hard" which we decide on
@@ -476,21 +497,7 @@ xfs_dir2_sf_addname_easy(
 	 * Need to set up again due to realloc of the inode data.
 	 */
 	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + byteoff);
-	/*
-	 * Fill in the new entry.
-	 */
-	sfep->namelen = args->namelen;
-	xfs_dir2_sf_put_offset(sfep, offset);
-	memcpy(sfep->name, args->name, sfep->namelen);
-	xfs_dir2_sf_put_ino(mp, sfp, sfep, args->inumber);
-	xfs_dir2_sf_put_ftype(mp, sfep, args->filetype);
-
-	/*
-	 * Update the header and inode.
-	 */
-	sfp->count++;
-	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM)
-		sfp->i8count++;
+	xfs_dir2_sf_addname_common(args, sfep, offset, false);
 	dp->i_disk_size = new_isize;
 	xfs_dir2_sf_check(args);
 }
@@ -562,17 +569,12 @@ xfs_dir2_sf_addname_hard(
 	nbytes = (int)((char *)oldsfep - (char *)oldsfp);
 	memcpy(sfp, oldsfp, nbytes);
 	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + nbytes);
+
 	/*
 	 * Fill in the new entry, and update the header counts.
 	 */
-	sfep->namelen = args->namelen;
-	xfs_dir2_sf_put_offset(sfep, offset);
-	memcpy(sfep->name, args->name, sfep->namelen);
-	xfs_dir2_sf_put_ino(mp, sfp, sfep, args->inumber);
-	xfs_dir2_sf_put_ftype(mp, sfep, args->filetype);
-	sfp->count++;
-	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM && !objchange)
-		sfp->i8count++;
+	xfs_dir2_sf_addname_common(args, sfep, offset, objchange);
+
 	/*
 	 * If there's more left to copy, do that.
 	 */
-- 
2.39.2

Re: [PATCH 12/16] xfs: factor out a xfs_dir2_sf_addname_common helper

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:22PM +0200, Christoph Hellwig wrote:
> Move the common code between the easy and hard cases into a common helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 48 +++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 1e1dcdf83b8f95..43e1090082b45d 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -360,6 +360,27 @@ xfs_dir2_try_block_to_sf(
>  	return error;
>  }
>  
> +static void
> +xfs_dir2_sf_addname_common(
> +	struct xfs_da_args	*args,
> +	struct xfs_dir2_sf_entry *sfep,
> +	xfs_dir2_data_aoff_t	offset,
> +	bool			objchange)

Hmm.  @objchange==true in the _hard() case means that we already
converted the sf dir to ino8 format, right?  So that's why we don't need
to increment i8count?

If the answers are yes and yes, then
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> +{
> +	struct xfs_inode	*dp = args->dp;
> +	struct xfs_mount	*mp = dp->i_mount;
> +	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> +
> +	sfep->namelen = args->namelen;
> +	xfs_dir2_sf_put_offset(sfep, offset);
> +	memcpy(sfep->name, args->name, sfep->namelen);
> +	xfs_dir2_sf_put_ino(mp, sfp, sfep, args->inumber);
> +	xfs_dir2_sf_put_ftype(mp, sfep, args->filetype);
> +	sfp->count++;
> +	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM && !objchange)
> +		sfp->i8count++;
> +}
> +
>  /*
>   * Add a name to a shortform directory.
>   * There are two algorithms, "easy" and "hard" which we decide on
> @@ -476,21 +497,7 @@ xfs_dir2_sf_addname_easy(
>  	 * Need to set up again due to realloc of the inode data.
>  	 */
>  	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + byteoff);
> -	/*
> -	 * Fill in the new entry.
> -	 */
> -	sfep->namelen = args->namelen;
> -	xfs_dir2_sf_put_offset(sfep, offset);
> -	memcpy(sfep->name, args->name, sfep->namelen);
> -	xfs_dir2_sf_put_ino(mp, sfp, sfep, args->inumber);
> -	xfs_dir2_sf_put_ftype(mp, sfep, args->filetype);
> -
> -	/*
> -	 * Update the header and inode.
> -	 */
> -	sfp->count++;
> -	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM)
> -		sfp->i8count++;
> +	xfs_dir2_sf_addname_common(args, sfep, offset, false);
>  	dp->i_disk_size = new_isize;
>  	xfs_dir2_sf_check(args);
>  }
> @@ -562,17 +569,12 @@ xfs_dir2_sf_addname_hard(
>  	nbytes = (int)((char *)oldsfep - (char *)oldsfp);
>  	memcpy(sfp, oldsfp, nbytes);
>  	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + nbytes);
> +
>  	/*
>  	 * Fill in the new entry, and update the header counts.
>  	 */
> -	sfep->namelen = args->namelen;
> -	xfs_dir2_sf_put_offset(sfep, offset);
> -	memcpy(sfep->name, args->name, sfep->namelen);
> -	xfs_dir2_sf_put_ino(mp, sfp, sfep, args->inumber);
> -	xfs_dir2_sf_put_ftype(mp, sfep, args->filetype);
> -	sfp->count++;
> -	if (args->inumber > XFS_DIR2_MAX_SHORT_INUM && !objchange)
> -		sfp->i8count++;
> +	xfs_dir2_sf_addname_common(args, sfep, offset, objchange);
> +
>  	/*
>  	 * If there's more left to copy, do that.
>  	 */
> -- 
> 2.39.2
> 
> 

Re: [PATCH 12/16] xfs: factor out a xfs_dir2_sf_addname_common helper

[Christoph Hellwig]

On Wed, May 01, 2024 at 02:31:47PM -0700, Darrick J. Wong wrote:
> Hmm.  @objchange==true in the _hard() case means that we already
> converted the sf dir to ino8 format, right?  So that's why we don't need
> to increment i8count?

Yes.

It is a little weird to be honest and given the final state after the
series I wonder if i8count manipulation should just move outside the
helper.

[PATCH 13/16] xfs: move common code into xfs_dir2_sf_addname

[Christoph Hellwig]

Move updating the inode size and the call to xfs_dir2_sf_check from
xfs_dir2_sf_addname_easy and xfs_dir2_sf_addname_hard into
xfs_dir2_sf_addname.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 43e1090082b45d..a9d614dfb9e43b 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -465,6 +465,9 @@ xfs_dir2_sf_addname(
 			xfs_dir2_sf_toino8(args);
 		xfs_dir2_sf_addname_hard(args, objchange, new_isize);
 	}
+
+	dp->i_disk_size = new_isize;
+	xfs_dir2_sf_check(args);
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
 	return 0;
 }
@@ -498,8 +501,6 @@ xfs_dir2_sf_addname_easy(
 	 */
 	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + byteoff);
 	xfs_dir2_sf_addname_common(args, sfep, offset, false);
-	dp->i_disk_size = new_isize;
-	xfs_dir2_sf_check(args);
 }
 
 /*
@@ -583,8 +584,6 @@ xfs_dir2_sf_addname_hard(
 		memcpy(sfep, oldsfep, old_isize - nbytes);
 	}
 	kfree(buf);
-	dp->i_disk_size = new_isize;
-	xfs_dir2_sf_check(args);
 }
 
 /*
-- 
2.39.2

Re: [PATCH 13/16] xfs: move common code into xfs_dir2_sf_addname

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:23PM +0200, Christoph Hellwig wrote:
> Move updating the inode size and the call to xfs_dir2_sf_check from
> xfs_dir2_sf_addname_easy and xfs_dir2_sf_addname_hard into
> xfs_dir2_sf_addname.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This seems simple enough,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 43e1090082b45d..a9d614dfb9e43b 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -465,6 +465,9 @@ xfs_dir2_sf_addname(
>  			xfs_dir2_sf_toino8(args);
>  		xfs_dir2_sf_addname_hard(args, objchange, new_isize);
>  	}
> +
> +	dp->i_disk_size = new_isize;
> +	xfs_dir2_sf_check(args);
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
>  	return 0;
>  }
> @@ -498,8 +501,6 @@ xfs_dir2_sf_addname_easy(
>  	 */
>  	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + byteoff);
>  	xfs_dir2_sf_addname_common(args, sfep, offset, false);
> -	dp->i_disk_size = new_isize;
> -	xfs_dir2_sf_check(args);
>  }
>  
>  /*
> @@ -583,8 +584,6 @@ xfs_dir2_sf_addname_hard(
>  		memcpy(sfep, oldsfep, old_isize - nbytes);
>  	}
>  	kfree(buf);
> -	dp->i_disk_size = new_isize;
> -	xfs_dir2_sf_check(args);
>  }
>  
>  /*
> -- 
> 2.39.2
> 
> 

[PATCH 14/16] xfs: optimize adding the first 8-byte inode to a shortform directory

[Christoph Hellwig]

When adding a new entry to a shortform directory we have to convert the
format of the entire inode fork if the new entry is the first 8-byte
inode number.

Instead of allocating a new buffer to convert the format, and then
possible another one when doing an insert in the middle of the directory,
simply add the new entry while converting the format and avoid the
extra allocation.

For this to work, xfs_dir2_sf_addname_pick also has to return the
offset for the hard case, but this is entirely trivial.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 46 +++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index a9d614dfb9e43b..02aa176348a795 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -35,7 +35,8 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args);
 #endif /* DEBUG */
 
 static void xfs_dir2_sf_toino4(struct xfs_da_args *args, bool remove);
-static void xfs_dir2_sf_toino8(xfs_da_args_t *args);
+static void xfs_dir2_sf_toino8(struct xfs_da_args *args,
+		xfs_dir2_data_aoff_t offset);
 
 int
 xfs_dir2_sf_entsize(
@@ -450,6 +451,16 @@ xfs_dir2_sf_addname(
 	 */
 	if (args->op_flags & XFS_DA_OP_JUSTCHECK)
 		return 0;
+
+	/*
+	 * If we need convert to 8-byte inodes, piggy back adding the new entry
+	 * to the rewrite of the fork to fit the large inode number.
+	 */
+	if (objchange) {
+		xfs_dir2_sf_toino8(args, offset);
+		return 0;
+	}
+
 	/*
 	 * Do it the easy way - just add it at the end.
 	 */
@@ -461,8 +472,6 @@ xfs_dir2_sf_addname(
 	 */
 	else {
 		ASSERT(pick == 2);
-		if (objchange)
-			xfs_dir2_sf_toino8(args);
 		xfs_dir2_sf_addname_hard(args, objchange, new_isize);
 	}
 
@@ -622,6 +631,8 @@ xfs_dir2_sf_addname_pick(
 	for (i = 0; i < sfp->count; i++) {
 		if (!holefit)
 			holefit = offset + size <= xfs_dir2_sf_get_offset(sfep);
+		if (holefit)
+			*offsetp = offset;
 		offset = xfs_dir2_sf_get_offset(sfep) +
 			 xfs_dir2_data_entsize(mp, sfep->namelen);
 		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
@@ -1053,7 +1064,7 @@ xfs_dir2_sf_replace(
 		/*
 		 * Still fits, convert to 8-byte now.
 		 */
-		xfs_dir2_sf_toino8(args);
+		xfs_dir2_sf_toino8(args, 0);
 		i8elevated = 1;
 		sfp = dp->i_df.if_data;
 	} else
@@ -1205,7 +1216,8 @@ xfs_dir2_sf_toino4(
  */
 static void
 xfs_dir2_sf_toino8(
-	xfs_da_args_t		*args)		/* operation arguments */
+	struct xfs_da_args	*args,
+	xfs_dir2_data_aoff_t	new_offset)
 {
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
@@ -1213,6 +1225,7 @@ xfs_dir2_sf_toino8(
 	int			oldsize = dp->i_df.if_bytes;
 	int			i;		/* entry index */
 	int			newsize;	/* new inode size */
+	unsigned int		newent_size;
 	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
 	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
 	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
@@ -1225,6 +1238,18 @@ xfs_dir2_sf_toino8(
 	 * Compute the new inode size (nb: entry count + 1 for parent)
 	 */
 	newsize = oldsize + (oldsfp->count + 1) * XFS_INO64_DIFF;
+	if (new_offset) {
+		/*
+		 * Account for the bytes actually used.
+		 */
+		newsize += xfs_dir2_sf_entsize(mp, oldsfp, args->namelen);
+
+		/*
+		 * But for the offset calculation use the bigger data entry
+		 * format.
+		 */
+		newent_size = xfs_dir2_data_entsize(mp, args->namelen);
+	}
 
 	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
 	dp->i_df.if_bytes = newsize;
@@ -1250,6 +1275,17 @@ xfs_dir2_sf_toino8(
 				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
 		xfs_dir2_sf_put_ftype(mp, sfep,
 				xfs_dir2_sf_get_ftype(mp, oldsfep));
+
+		/*
+		 * If there is a new entry to add it once we reach the specified
+		 * offset.
+		 */
+		if (new_offset &&
+		    new_offset == xfs_dir2_sf_get_offset(sfep) + newent_size) {
+			sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
+			xfs_dir2_sf_addname_common(args, sfep, new_offset,
+					true);
+		}
 	}
 
 	kfree(oldsfp);
-- 
2.39.2

Re: [PATCH 14/16] xfs: optimize adding the first 8-byte inode to a shortform directory

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:24PM +0200, Christoph Hellwig wrote:
> When adding a new entry to a shortform directory we have to convert the
> format of the entire inode fork if the new entry is the first 8-byte
> inode number.
> 
> Instead of allocating a new buffer to convert the format, and then
> possible another one when doing an insert in the middle of the directory,
> simply add the new entry while converting the format and avoid the
> extra allocation.
> 
> For this to work, xfs_dir2_sf_addname_pick also has to return the
> offset for the hard case, but this is entirely trivial.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 46 +++++++++++++++++++++++++++++++++----
>  1 file changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index a9d614dfb9e43b..02aa176348a795 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -35,7 +35,8 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args);
>  #endif /* DEBUG */
>  
>  static void xfs_dir2_sf_toino4(struct xfs_da_args *args, bool remove);
> -static void xfs_dir2_sf_toino8(xfs_da_args_t *args);
> +static void xfs_dir2_sf_toino8(struct xfs_da_args *args,
> +		xfs_dir2_data_aoff_t offset);

I noticed a few places where we pass offset == 0 here.  That's ok as a
null value because the start of a shortform directory is always the
header, correct?

>  
>  int
>  xfs_dir2_sf_entsize(
> @@ -450,6 +451,16 @@ xfs_dir2_sf_addname(
>  	 */
>  	if (args->op_flags & XFS_DA_OP_JUSTCHECK)
>  		return 0;
> +
> +	/*
> +	 * If we need convert to 8-byte inodes, piggy back adding the new entry
> +	 * to the rewrite of the fork to fit the large inode number.
> +	 */
> +	if (objchange) {
> +		xfs_dir2_sf_toino8(args, offset);
> +		return 0;
> +	}
> +
>  	/*
>  	 * Do it the easy way - just add it at the end.
>  	 */
> @@ -461,8 +472,6 @@ xfs_dir2_sf_addname(
>  	 */
>  	else {
>  		ASSERT(pick == 2);
> -		if (objchange)
> -			xfs_dir2_sf_toino8(args);

Ok, so this isn't needed anymore because the ino8 conversion now adds
the new dirent?

>  		xfs_dir2_sf_addname_hard(args, objchange, new_isize);
>  	}
>  
> @@ -622,6 +631,8 @@ xfs_dir2_sf_addname_pick(
>  	for (i = 0; i < sfp->count; i++) {
>  		if (!holefit)
>  			holefit = offset + size <= xfs_dir2_sf_get_offset(sfep);
> +		if (holefit)
> +			*offsetp = offset;
>  		offset = xfs_dir2_sf_get_offset(sfep) +
>  			 xfs_dir2_data_entsize(mp, sfep->namelen);
>  		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
> @@ -1053,7 +1064,7 @@ xfs_dir2_sf_replace(
>  		/*
>  		 * Still fits, convert to 8-byte now.
>  		 */
> -		xfs_dir2_sf_toino8(args);
> +		xfs_dir2_sf_toino8(args, 0);

This is a replace, so we pass 0 here effectively as a null value?

>  		i8elevated = 1;
>  		sfp = dp->i_df.if_data;
>  	} else
> @@ -1205,7 +1216,8 @@ xfs_dir2_sf_toino4(
>   */
>  static void
>  xfs_dir2_sf_toino8(
> -	xfs_da_args_t		*args)		/* operation arguments */
> +	struct xfs_da_args	*args,
> +	xfs_dir2_data_aoff_t	new_offset)

Yeah, the comment for this function should note that new_offset!=0 means
to add the entry referenced in the args structure.

>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
> @@ -1213,6 +1225,7 @@ xfs_dir2_sf_toino8(
>  	int			oldsize = dp->i_df.if_bytes;
>  	int			i;		/* entry index */
>  	int			newsize;	/* new inode size */
> +	unsigned int		newent_size;
>  	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
>  	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
>  	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
> @@ -1225,6 +1238,18 @@ xfs_dir2_sf_toino8(
>  	 * Compute the new inode size (nb: entry count + 1 for parent)
>  	 */
>  	newsize = oldsize + (oldsfp->count + 1) * XFS_INO64_DIFF;
> +	if (new_offset) {
> +		/*
> +		 * Account for the bytes actually used.
> +		 */
> +		newsize += xfs_dir2_sf_entsize(mp, oldsfp, args->namelen);
> +
> +		/*
> +		 * But for the offset calculation use the bigger data entry
> +		 * format.
> +		 */
> +		newent_size = xfs_dir2_data_entsize(mp, args->namelen);

		/*
		 * Bump up the buffer size by the size of the new
		 * dirent.  Now that we've set i8count, compute the size
		 * of the new dirent.
		 */
		newsize += xfs_dir2_sf_entsize(mp, oldsfp, args->namelen);
		newent_size = xfs_dir2_data_entsize(mp, args->namelen);

> +	}
>  
>  	dp->i_df.if_data = sfp = kmalloc(newsize, GFP_KERNEL | __GFP_NOFAIL);
>  	dp->i_df.if_bytes = newsize;
> @@ -1250,6 +1275,17 @@ xfs_dir2_sf_toino8(
>  				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
>  		xfs_dir2_sf_put_ftype(mp, sfep,
>  				xfs_dir2_sf_get_ftype(mp, oldsfep));
> +
> +		/*
> +		 * If there is a new entry to add it once we reach the specified
> +		 * offset.

It took me a minute of staring at the if test logic to figure out what
we're doing here.  If, after, reformatting a directory entry, the next
entry is the offset where _pick wants us to place the new dirent, we
should jump sfep to the next entry, and then add the new entry.

Is that right?  And we can't simplify the logic to:

	if (new_offset && new_offset = xfs_dir2_sf_get_offset(sfep))

Because _pick might want us to add the entry at the end of the directory
but we haven't incremented sfp->count yet, so the loop body will not be
executed in that case.

Is it ever the case that the entry get added in the middle of a
shortform directory?

--D

> +		 */
> +		if (new_offset &&
> +		    new_offset == xfs_dir2_sf_get_offset(sfep) + newent_size) {
> +			sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
> +			xfs_dir2_sf_addname_common(args, sfep, new_offset,
> +					true);
> +		}
>  	}
>  
>  	kfree(oldsfp);
> -- 
> 2.39.2
> 
> 

Re: [PATCH 14/16] xfs: optimize adding the first 8-byte inode to a shortform directory

[Christoph Hellwig]

On Wed, May 01, 2024 at 02:50:56PM -0700, Darrick J. Wong wrote:
> I noticed a few places where we pass offset == 0 here.  That's ok as a
> null value because the start of a shortform directory is always the
> header, correct?

The start of the "physical" layout has the header, but offset is the
"logic" d_offset offset.  The start of it it reserved for (but not
actually used by) the "." and ".." entries that will occupy the space
when converted out of the short form.  Probably also needs a comment.

> Ok, so this isn't needed anymore because the ino8 conversion now adds
> the new dirent?

Yes.

> > -		xfs_dir2_sf_toino8(args);
> > +		xfs_dir2_sf_toino8(args, 0);
> 
> This is a replace, so we pass 0 here effectively as a null value?

Exactly.

> > @@ -1250,6 +1275,17 @@ xfs_dir2_sf_toino8(
> >  				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> >  		xfs_dir2_sf_put_ftype(mp, sfep,
> >  				xfs_dir2_sf_get_ftype(mp, oldsfep));
> > +
> > +		/*
> > +		 * If there is a new entry to add it once we reach the specified
> > +		 * offset.
> 
> It took me a minute of staring at the if test logic to figure out what
> we're doing here.  If, after, reformatting a directory entry, the next
> entry is the offset where _pick wants us to place the new dirent, we
> should jump sfep to the next entry, and then add the new entry.
> 
> Is that right?  And we can't simplify the logic to:
> 
> 	if (new_offset && new_offset = xfs_dir2_sf_get_offset(sfep))

== ?

> Because _pick might want us to add the entry at the end of the directory
> but we haven't incremented sfp->count yet, so the loop body will not be
> executed in that case.
> 
> Is it ever the case that the entry get added in the middle of a
> shortform directory?

Yes, that is the hard case.  There is no good reason to add it in
the middle, but we've encoded that the "logical" offset for a
shortform directly needs to fit into the physical size of a single
directory block when converted to block format in asserts and verifiers
and are stuck with it.  Otherwise we could have just always added it
at the end..

Re: [PATCH 14/16] xfs: optimize adding the first 8-byte inode to a shortform directory

[Darrick J. Wong]

On Thu, May 02, 2024 at 06:25:09AM +0200, Christoph Hellwig wrote:
> On Wed, May 01, 2024 at 02:50:56PM -0700, Darrick J. Wong wrote:
> > I noticed a few places where we pass offset == 0 here.  That's ok as a
> > null value because the start of a shortform directory is always the
> > header, correct?
> 
> The start of the "physical" layout has the header, but offset is the
> "logic" d_offset offset.  The start of it it reserved for (but not
> actually used by) the "." and ".." entries that will occupy the space
> when converted out of the short form.  Probably also needs a comment.
> 
> > Ok, so this isn't needed anymore because the ino8 conversion now adds
> > the new dirent?
> 
> Yes.
> 
> > > -		xfs_dir2_sf_toino8(args);
> > > +		xfs_dir2_sf_toino8(args, 0);
> > 
> > This is a replace, so we pass 0 here effectively as a null value?
> 
> Exactly.

ok good.

> > > @@ -1250,6 +1275,17 @@ xfs_dir2_sf_toino8(
> > >  				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> > >  		xfs_dir2_sf_put_ftype(mp, sfep,
> > >  				xfs_dir2_sf_get_ftype(mp, oldsfep));
> > > +
> > > +		/*
> > > +		 * If there is a new entry to add it once we reach the specified
> > > +		 * offset.
> > 
> > It took me a minute of staring at the if test logic to figure out what
> > we're doing here.  If, after, reformatting a directory entry, the next
> > entry is the offset where _pick wants us to place the new dirent, we
> > should jump sfep to the next entry, and then add the new entry.
> > 
> > Is that right?  And we can't simplify the logic to:
> > 
> > 	if (new_offset && new_offset = xfs_dir2_sf_get_offset(sfep))
> 
> == ?

Yes, double-equals, not single-equals.

> > Because _pick might want us to add the entry at the end of the directory
> > but we haven't incremented sfp->count yet, so the loop body will not be
> > executed in that case.
> > 
> > Is it ever the case that the entry get added in the middle of a
> > shortform directory?
> 
> Yes, that is the hard case.  There is no good reason to add it in
> the middle, but we've encoded that the "logical" offset for a
> shortform directly needs to fit into the physical size of a single
> directory block when converted to block format in asserts and verifiers
> and are stuck with it.  Otherwise we could have just always added it
> at the end..

<nod> I think the mechanics of this patch look ok, but this:

		xfs_dir2_sf_toino8(args, 0);

worries me because the reader has to know that zero is never a valid
offset for adding a dirent, vs:

#define XFS_DIR2_DATA_AOFF_NULL	((xfs_dir2_data_aoff_t)0)

		xfs_dir2_sf_toino8(args, XFS_DIR2_DATA_AOFF_NULL);

shouts that we're not trying to add anything.

--D

[PATCH 15/16] xfs: move the block format conversion out of line in xfs_dir2_sf_addname

[Christoph Hellwig]

Move the code to convert to the block format and add the entry to the end
of xfs_dir2_sf_addname instead of the current very hard to read compound
statement in the middle of the function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 02aa176348a795..0598465357cc3a 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -426,26 +426,16 @@ xfs_dir2_sf_addname(
 	}
 
 	new_isize = (int)dp->i_disk_size + incr_isize;
+
 	/*
-	 * Won't fit as shortform any more (due to size),
-	 * or the pick routine says it won't (due to offset values).
+	 * Too large to fit into the inode fork or too large offset?
 	 */
-	if (new_isize > xfs_inode_data_fork_size(dp) ||
-	    (pick =
-	     xfs_dir2_sf_addname_pick(args, objchange, &sfep, &offset)) == 0) {
-		/*
-		 * Just checking or no space reservation, it doesn't fit.
-		 */
-		if ((args->op_flags & XFS_DA_OP_JUSTCHECK) || args->total == 0)
-			return -ENOSPC;
-		/*
-		 * Convert to block form then add the name.
-		 */
-		error = xfs_dir2_sf_to_block(args);
-		if (error)
-			return error;
-		return xfs_dir2_block_addname(args);
-	}
+	if (new_isize > xfs_inode_data_fork_size(dp))
+		goto convert;
+	pick = xfs_dir2_sf_addname_pick(args, objchange, &sfep, &offset);
+	if (pick == 0)
+		goto convert;
+
 	/*
 	 * Just checking, it fits.
 	 */
@@ -479,6 +469,17 @@ xfs_dir2_sf_addname(
 	xfs_dir2_sf_check(args);
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
 	return 0;
+
+convert:
+	/*
+	 * Just checking or no space reservation, it doesn't fit.
+	 */
+	if ((args->op_flags & XFS_DA_OP_JUSTCHECK) || args->total == 0)
+		return -ENOSPC;
+	error = xfs_dir2_sf_to_block(args);
+	if (error)
+		return error;
+	return xfs_dir2_block_addname(args);
 }
 
 /*
-- 
2.39.2

Re: [PATCH 15/16] xfs: move the block format conversion out of line in xfs_dir2_sf_addname

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:25PM +0200, Christoph Hellwig wrote:
> Move the code to convert to the block format and add the entry to the end
> of xfs_dir2_sf_addname instead of the current very hard to read compound
> statement in the middle of the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yeah, that was kind of a mess...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 02aa176348a795..0598465357cc3a 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -426,26 +426,16 @@ xfs_dir2_sf_addname(
>  	}
>  
>  	new_isize = (int)dp->i_disk_size + incr_isize;
> +
>  	/*
> -	 * Won't fit as shortform any more (due to size),
> -	 * or the pick routine says it won't (due to offset values).
> +	 * Too large to fit into the inode fork or too large offset?
>  	 */
> -	if (new_isize > xfs_inode_data_fork_size(dp) ||
> -	    (pick =
> -	     xfs_dir2_sf_addname_pick(args, objchange, &sfep, &offset)) == 0) {
> -		/*
> -		 * Just checking or no space reservation, it doesn't fit.
> -		 */
> -		if ((args->op_flags & XFS_DA_OP_JUSTCHECK) || args->total == 0)
> -			return -ENOSPC;
> -		/*
> -		 * Convert to block form then add the name.
> -		 */
> -		error = xfs_dir2_sf_to_block(args);
> -		if (error)
> -			return error;
> -		return xfs_dir2_block_addname(args);
> -	}
> +	if (new_isize > xfs_inode_data_fork_size(dp))
> +		goto convert;
> +	pick = xfs_dir2_sf_addname_pick(args, objchange, &sfep, &offset);
> +	if (pick == 0)
> +		goto convert;
> +
>  	/*
>  	 * Just checking, it fits.
>  	 */
> @@ -479,6 +469,17 @@ xfs_dir2_sf_addname(
>  	xfs_dir2_sf_check(args);
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
>  	return 0;
> +
> +convert:
> +	/*
> +	 * Just checking or no space reservation, it doesn't fit.
> +	 */
> +	if ((args->op_flags & XFS_DA_OP_JUSTCHECK) || args->total == 0)
> +		return -ENOSPC;
> +	error = xfs_dir2_sf_to_block(args);
> +	if (error)
> +		return error;
> +	return xfs_dir2_block_addname(args);
>  }
>  
>  /*
> -- 
> 2.39.2
> 
> 

[PATCH 16/16] xfs: make the hard case in xfs_dir2_sf_addname less hard

[Christoph Hellwig]

xfs_dir2_sf_addname tries and easy addition first where the new entry
is added to an end and a new higher offset is allocated to it.  If an
arbitrary limit on the offset is trigger it goes down the "hard" path
and instead looks for a hole in the offset space, which then also
implies inserting the new entry in the middle as the entries are sorted
by the d_offset offset.

The code to add an entry the hard way is way to complicated and
inefficient as it duplicates the linear search of the directory to
find the entry, full frees the old inode fork data and reallocates
it just to copy data into it from a temporary buffer.  Rewrite all
this to use the offset from the initial scan, do the usual idata
realloc and then just move the entries past the insertion point out
of the way in place using memmove.  This also happens to allow sharing
the code entirely with the easy case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 319 ++++++++++--------------------------
 1 file changed, 89 insertions(+), 230 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 0598465357cc3a..4ba93835dd847f 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -16,18 +16,6 @@
 #include "xfs_dir2_priv.h"
 #include "xfs_trace.h"
 
-/*
- * Prototypes for internal functions.
- */
-static void xfs_dir2_sf_addname_easy(xfs_da_args_t *args,
-				     xfs_dir2_sf_entry_t *sfep,
-				     xfs_dir2_data_aoff_t offset,
-				     int new_isize);
-static void xfs_dir2_sf_addname_hard(xfs_da_args_t *args, int objchange,
-				     int new_isize);
-static int xfs_dir2_sf_addname_pick(xfs_da_args_t *args, int objchange,
-				    xfs_dir2_sf_entry_t **sfepp,
-				    xfs_dir2_data_aoff_t *offsetp);
 #ifdef DEBUG
 static void xfs_dir2_sf_check(xfs_da_args_t *args);
 #else
@@ -361,6 +349,61 @@ xfs_dir2_try_block_to_sf(
 	return error;
 }
 
+static xfs_dir2_data_aoff_t
+xfs_dir2_sf_addname_pick_offset(
+	struct xfs_da_args	*args,
+	unsigned int		*byteoffp)
+{
+	struct xfs_inode	*dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
+	xfs_dir2_data_aoff_t	offset = args->geo->data_first_offset;
+	struct xfs_dir2_sf_entry *sfep = xfs_dir2_sf_firstentry(sfp);
+	unsigned int		data_size =
+		xfs_dir2_data_entsize(mp, args->namelen);
+	unsigned int		data_overhead =
+		xfs_dir2_block_overhead(sfp->count + 1);
+	xfs_dir2_data_aoff_t	hole_offset = 0;
+	unsigned int		byteoff = 0;
+	unsigned int		i;
+
+	/*
+	 * Scan the directory to find the last offset and find a suitable
+	 * hole that we can use if needed.
+	 */
+	for (i = 0; i < sfp->count; i++) {
+		if (!hole_offset &&
+		    offset + data_size <= xfs_dir2_sf_get_offset(sfep)) {
+			hole_offset = offset;
+			byteoff = (void *)sfep - dp->i_df.if_data;
+		}
+		offset = xfs_dir2_sf_get_offset(sfep) +
+			xfs_dir2_data_entsize(mp, sfep->namelen);
+		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
+	}
+
+	/*
+	 * If just appending the entry would make the offset too large, use the
+	 * first hole that fits it if there is one or else give up and convert
+	 * to block format.
+	 *
+	 * Note that "too large" here is a completely arbitrary limit.  The
+	 * offset is logical concept not related to the physical byteoff and
+	 * there is no fundamental underlying limit to it.  But it has been
+	 * encoded in asserts and verifiers for a long time so we have to
+	 * respect it.
+	 */
+	if (offset + data_size + data_overhead > args->geo->blksize) {
+		if (!hole_offset)
+			return 0;
+		*byteoffp = byteoff;
+		return hole_offset;
+	}
+
+	*byteoffp = dp->i_disk_size;
+	return offset;
+}
+
 static void
 xfs_dir2_sf_addname_common(
 	struct xfs_da_args	*args,
@@ -384,23 +427,29 @@ xfs_dir2_sf_addname_common(
 
 /*
  * Add a name to a shortform directory.
- * There are two algorithms, "easy" and "hard" which we decide on
- * before changing anything.
- * Convert to block form if necessary, if the new entry won't fit.
+ *
+ * Shortform directories are always tighty packed, and we simply allocate
+ * a new higher offset and add the entry at the end.
+ *
+ * While we could search for a hole in the offset space there really isn't
+ * much of a a point in doing so and complicating the algorithm.
+ *
+ * Convert to block form if the new entry won't fit.
  */
-int						/* error */
+int
 xfs_dir2_sf_addname(
-	xfs_da_args_t		*args)		/* operation arguments */
+	struct xfs_da_args	*args)
 {
 	struct xfs_inode	*dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
-	int			error;		/* error return value */
-	int			incr_isize;	/* total change in size */
-	int			new_isize;	/* size after adding name */
-	int			objchange;	/* changing to 8-byte inodes */
-	xfs_dir2_data_aoff_t	offset = 0;	/* offset for new entry */
-	int			pick;		/* which algorithm to use */
-	xfs_dir2_sf_entry_t	*sfep = NULL;	/* shortform entry */
+	struct xfs_dir2_sf_entry *sfep;
+	xfs_dir2_data_aoff_t	offset;
+	unsigned int		entsize;
+	unsigned int		new_isize;
+	unsigned int		byteoff;
+	bool			objchange = false;
+	int			error;
 
 	trace_xfs_dir2_sf_addname(args);
 
@@ -408,11 +457,12 @@ xfs_dir2_sf_addname(
 	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
 	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
 	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
+
 	/*
-	 * Compute entry (and change in) size.
+	 * Compute entry size and new inode size.
 	 */
-	incr_isize = xfs_dir2_sf_entsize(dp->i_mount, sfp, args->namelen);
-	objchange = 0;
+	entsize = xfs_dir2_sf_entsize(mp, sfp, args->namelen);
+	new_isize = dp->i_disk_size + entsize;
 
 	/*
 	 * Do we have to change to 8 byte inodes?
@@ -421,19 +471,17 @@ xfs_dir2_sf_addname(
 		/*
 		 * Yes, adjust the inode size.  old count + (parent + new)
 		 */
-		incr_isize += (sfp->count + 2) * XFS_INO64_DIFF;
-		objchange = 1;
+		new_isize += (sfp->count + 2) * XFS_INO64_DIFF;
+		objchange = true;
 	}
 
-	new_isize = (int)dp->i_disk_size + incr_isize;
-
 	/*
 	 * Too large to fit into the inode fork or too large offset?
 	 */
 	if (new_isize > xfs_inode_data_fork_size(dp))
 		goto convert;
-	pick = xfs_dir2_sf_addname_pick(args, objchange, &sfep, &offset);
-	if (pick == 0)
+	offset = xfs_dir2_sf_addname_pick_offset(args, &byteoff);
+	if (!offset)
 		goto convert;
 
 	/*
@@ -451,20 +499,17 @@ xfs_dir2_sf_addname(
 		return 0;
 	}
 
+	sfp = xfs_idata_realloc(dp, entsize, XFS_DATA_FORK);
+	sfep = dp->i_df.if_data + byteoff;
+
 	/*
-	 * Do it the easy way - just add it at the end.
-	 */
-	if (pick == 1)
-		xfs_dir2_sf_addname_easy(args, sfep, offset, new_isize);
-	/*
-	 * Do it the hard way - look for a place to insert the new entry.
-	 * Convert to 8 byte inode numbers first if necessary.
+	 * If we're inserting in the middle, move the tail out of the way first.
 	 */
-	else {
-		ASSERT(pick == 2);
-		xfs_dir2_sf_addname_hard(args, objchange, new_isize);
+	if (byteoff < dp->i_disk_size) {
+		memmove(sfep, (void *)sfep + entsize,
+			dp->i_disk_size - byteoff);
 	}
-
+	xfs_dir2_sf_addname_common(args, sfep, offset, objchange);
 	dp->i_disk_size = new_isize;
 	xfs_dir2_sf_check(args);
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
@@ -482,192 +527,6 @@ xfs_dir2_sf_addname(
 	return xfs_dir2_block_addname(args);
 }
 
-/*
- * Add the new entry the "easy" way.
- * This is copying the old directory and adding the new entry at the end.
- * Since it's sorted by "offset" we need room after the last offset
- * that's already there, and then room to convert to a block directory.
- * This is already checked by the pick routine.
- */
-static void
-xfs_dir2_sf_addname_easy(
-	xfs_da_args_t		*args,		/* operation arguments */
-	xfs_dir2_sf_entry_t	*sfep,		/* pointer to new entry */
-	xfs_dir2_data_aoff_t	offset,		/* offset to use for new ent */
-	int			new_isize)	/* new directory size */
-{
-	struct xfs_inode	*dp = args->dp;
-	struct xfs_mount	*mp = dp->i_mount;
-	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
-	int			byteoff = (int)((char *)sfep - (char *)sfp);
-
-	/*
-	 * Grow the in-inode space.
-	 */
-	sfp = xfs_idata_realloc(dp, xfs_dir2_sf_entsize(mp, sfp, args->namelen),
-			  XFS_DATA_FORK);
-	/*
-	 * Need to set up again due to realloc of the inode data.
-	 */
-	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + byteoff);
-	xfs_dir2_sf_addname_common(args, sfep, offset, false);
-}
-
-/*
- * Add the new entry the "hard" way.
- * The caller has already converted to 8 byte inode numbers if necessary,
- * in which case we need to leave the i8count at 1.
- * Find a hole that the new entry will fit into, and copy
- * the first part of the entries, the new entry, and the last part of
- * the entries.
- */
-/* ARGSUSED */
-static void
-xfs_dir2_sf_addname_hard(
-	xfs_da_args_t		*args,		/* operation arguments */
-	int			objchange,	/* changing inode number size */
-	int			new_isize)	/* new directory size */
-{
-	struct xfs_inode	*dp = args->dp;
-	struct xfs_mount	*mp = dp->i_mount;
-	int			add_datasize;	/* data size need for new ent */
-	char			*buf;		/* buffer for old */
-	int			eof;		/* reached end of old dir */
-	int			nbytes;		/* temp for byte copies */
-	xfs_dir2_data_aoff_t	new_offset;	/* next offset value */
-	xfs_dir2_data_aoff_t	offset;		/* current offset value */
-	int			old_isize;	/* previous size */
-	xfs_dir2_sf_entry_t	*oldsfep;	/* entry in original dir */
-	xfs_dir2_sf_hdr_t	*oldsfp;	/* original shortform dir */
-	xfs_dir2_sf_entry_t	*sfep;		/* entry in new dir */
-	xfs_dir2_sf_hdr_t	*sfp;		/* new shortform dir */
-
-	/*
-	 * Copy the old directory to the stack buffer.
-	 */
-	old_isize = (int)dp->i_disk_size;
-	buf = kmalloc(old_isize, GFP_KERNEL | __GFP_NOFAIL);
-	oldsfp = (xfs_dir2_sf_hdr_t *)buf;
-	memcpy(oldsfp, dp->i_df.if_data, old_isize);
-	/*
-	 * Loop over the old directory finding the place we're going
-	 * to insert the new entry.
-	 * If it's going to end up at the end then oldsfep will point there.
-	 */
-	for (offset = args->geo->data_first_offset,
-	      oldsfep = xfs_dir2_sf_firstentry(oldsfp),
-	      add_datasize = xfs_dir2_data_entsize(mp, args->namelen),
-	      eof = (char *)oldsfep == &buf[old_isize];
-	     !eof;
-	     offset = new_offset + xfs_dir2_data_entsize(mp, oldsfep->namelen),
-	      oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep),
-	      eof = (char *)oldsfep == &buf[old_isize]) {
-		new_offset = xfs_dir2_sf_get_offset(oldsfep);
-		if (offset + add_datasize <= new_offset)
-			break;
-	}
-	/*
-	 * Get rid of the old directory, then allocate space for
-	 * the new one.  We do this so xfs_idata_realloc won't copy
-	 * the data.
-	 */
-	xfs_idata_realloc(dp, -old_isize, XFS_DATA_FORK);
-	sfp = xfs_idata_realloc(dp, new_isize, XFS_DATA_FORK);
-
-	/*
-	 * Copy the first part of the directory, including the header.
-	 */
-	nbytes = (int)((char *)oldsfep - (char *)oldsfp);
-	memcpy(sfp, oldsfp, nbytes);
-	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + nbytes);
-
-	/*
-	 * Fill in the new entry, and update the header counts.
-	 */
-	xfs_dir2_sf_addname_common(args, sfep, offset, objchange);
-
-	/*
-	 * If there's more left to copy, do that.
-	 */
-	if (!eof) {
-		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
-		memcpy(sfep, oldsfep, old_isize - nbytes);
-	}
-	kfree(buf);
-}
-
-/*
- * Decide if the new entry will fit at all.
- * If it will fit, pick between adding the new entry to the end (easy)
- * or somewhere else (hard).
- * Return 0 (won't fit), 1 (easy), 2 (hard).
- */
-/*ARGSUSED*/
-static int					/* pick result */
-xfs_dir2_sf_addname_pick(
-	xfs_da_args_t		*args,		/* operation arguments */
-	int			objchange,	/* inode # size changes */
-	xfs_dir2_sf_entry_t	**sfepp,	/* out(1): new entry ptr */
-	xfs_dir2_data_aoff_t	*offsetp)	/* out(1): new offset */
-{
-	struct xfs_inode	*dp = args->dp;
-	struct xfs_mount	*mp = dp->i_mount;
-	int			holefit;	/* found hole it will fit in */
-	int			i;		/* entry number */
-	xfs_dir2_data_aoff_t	offset;		/* data block offset */
-	xfs_dir2_sf_entry_t	*sfep;		/* shortform entry */
-	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
-	int			size;		/* entry's data size */
-	int			used;		/* data bytes used */
-
-	size = xfs_dir2_data_entsize(mp, args->namelen);
-	offset = args->geo->data_first_offset;
-	sfep = xfs_dir2_sf_firstentry(sfp);
-	holefit = 0;
-	/*
-	 * Loop over sf entries.
-	 * Keep track of data offset and whether we've seen a place
-	 * to insert the new entry.
-	 */
-	for (i = 0; i < sfp->count; i++) {
-		if (!holefit)
-			holefit = offset + size <= xfs_dir2_sf_get_offset(sfep);
-		if (holefit)
-			*offsetp = offset;
-		offset = xfs_dir2_sf_get_offset(sfep) +
-			 xfs_dir2_data_entsize(mp, sfep->namelen);
-		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
-	}
-	/*
-	 * Calculate data bytes used excluding the new entry, if this
-	 * was a data block (block form directory).
-	 */
-	used = offset + xfs_dir2_block_overhead(sfp->count + 1);
-	/*
-	 * If it won't fit in a block form then we can't insert it,
-	 * we'll go back, convert to block, then try the insert and convert
-	 * to leaf.
-	 */
-	if (used + (holefit ? 0 : size) > args->geo->blksize)
-		return 0;
-	/*
-	 * If changing the inode number size, do it the hard way.
-	 */
-	if (objchange)
-		return 2;
-	/*
-	 * If it won't fit at the end then do it the hard way (use the hole).
-	 */
-	if (used + size > args->geo->blksize)
-		return 2;
-	/*
-	 * Do it the easy way.
-	 */
-	*sfepp = sfep;
-	*offsetp = offset;
-	return 1;
-}
-
 #ifdef DEBUG
 /*
  * Check consistency of shortform directory, assert if bad.
-- 
2.39.2

Re: [PATCH 16/16] xfs: make the hard case in xfs_dir2_sf_addname less hard

[Darrick J. Wong]

On Tue, Apr 30, 2024 at 02:49:26PM +0200, Christoph Hellwig wrote:
> xfs_dir2_sf_addname tries and easy addition first where the new entry
> is added to an end and a new higher offset is allocated to it.  If an
> arbitrary limit on the offset is trigger it goes down the "hard" path
> and instead looks for a hole in the offset space, which then also
> implies inserting the new entry in the middle as the entries are sorted
> by the d_offset offset.

/me smacks forehead.  Ahh, right.  This is how _pick can end up telling
us to insert a dirent in the middle of the shortform dirent.  The bytes
of the dirent are packed together, but the d_offset "address space" can
be sparse.  So if a directory contains:

u.sfdir2.list[0].namelen = 15
u.sfdir2.list[0].offset = 0x30
u.sfdir2.list[0].name = ”frame000000.tst”
u.sfdir2.list[0].inumber.i4 = 25165953

u.sfdir2.list[1].namelen = 15
u.sfdir2.list[1].offset = 0x90
u.sfdir2.list[1].name = ”frame000001.tst”
u.sfdir2.list[1].inumber.i4 = 25165954

Then the _pick function can decide that we should insert the dirent
at "offset" 0x50, which involves memmove'ing u.sfdir2.list[1] upwards,
and then writing a new dirent between the two:

u.sfdir2.list[0].namelen = 15
u.sfdir2.list[0].offset = 0x30
u.sfdir2.list[0].name = ”frame000000.tst”
u.sfdir2.list[0].inumber.i4 = 25165953

u.sfdir2.list[1].namelen = 15
u.sfdir2.list[1].offset = 0x50
u.sfdir2.list[1].name = ”mugwumpquux.tst”
u.sfdir2.list[1].inumber.i4 = 25165955

u.sfdir2.list[2].namelen = 15
u.sfdir2.list[2].offset = 0x90
u.sfdir2.list[2].name = ”frame000001.tst”
u.sfdir2.list[2].inumber.i4 = 25165954

and this is really what the "hard" case is doing.

> The code to add an entry the hard way is way to complicated and
> inefficient as it duplicates the linear search of the directory to
> find the entry, full frees the old inode fork data and reallocates
> it just to copy data into it from a temporary buffer.  Rewrite all
> this to use the offset from the initial scan, do the usual idata
> realloc and then just move the entries past the insertion point out
> of the way in place using memmove.  This also happens to allow sharing
> the code entirely with the easy case.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

If my understanding of that is correct, then
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_sf.c | 319 ++++++++++--------------------------
>  1 file changed, 89 insertions(+), 230 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 0598465357cc3a..4ba93835dd847f 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -16,18 +16,6 @@
>  #include "xfs_dir2_priv.h"
>  #include "xfs_trace.h"
>  
> -/*
> - * Prototypes for internal functions.
> - */
> -static void xfs_dir2_sf_addname_easy(xfs_da_args_t *args,
> -				     xfs_dir2_sf_entry_t *sfep,
> -				     xfs_dir2_data_aoff_t offset,
> -				     int new_isize);
> -static void xfs_dir2_sf_addname_hard(xfs_da_args_t *args, int objchange,
> -				     int new_isize);
> -static int xfs_dir2_sf_addname_pick(xfs_da_args_t *args, int objchange,
> -				    xfs_dir2_sf_entry_t **sfepp,
> -				    xfs_dir2_data_aoff_t *offsetp);
>  #ifdef DEBUG
>  static void xfs_dir2_sf_check(xfs_da_args_t *args);
>  #else
> @@ -361,6 +349,61 @@ xfs_dir2_try_block_to_sf(
>  	return error;
>  }
>  
> +static xfs_dir2_data_aoff_t
> +xfs_dir2_sf_addname_pick_offset(
> +	struct xfs_da_args	*args,
> +	unsigned int		*byteoffp)
> +{
> +	struct xfs_inode	*dp = args->dp;
> +	struct xfs_mount	*mp = dp->i_mount;
> +	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> +	xfs_dir2_data_aoff_t	offset = args->geo->data_first_offset;
> +	struct xfs_dir2_sf_entry *sfep = xfs_dir2_sf_firstentry(sfp);
> +	unsigned int		data_size =
> +		xfs_dir2_data_entsize(mp, args->namelen);
> +	unsigned int		data_overhead =
> +		xfs_dir2_block_overhead(sfp->count + 1);
> +	xfs_dir2_data_aoff_t	hole_offset = 0;
> +	unsigned int		byteoff = 0;
> +	unsigned int		i;
> +
> +	/*
> +	 * Scan the directory to find the last offset and find a suitable
> +	 * hole that we can use if needed.
> +	 */
> +	for (i = 0; i < sfp->count; i++) {
> +		if (!hole_offset &&
> +		    offset + data_size <= xfs_dir2_sf_get_offset(sfep)) {
> +			hole_offset = offset;
> +			byteoff = (void *)sfep - dp->i_df.if_data;
> +		}
> +		offset = xfs_dir2_sf_get_offset(sfep) +
> +			xfs_dir2_data_entsize(mp, sfep->namelen);
> +		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
> +	}
> +
> +	/*
> +	 * If just appending the entry would make the offset too large, use the
> +	 * first hole that fits it if there is one or else give up and convert
> +	 * to block format.
> +	 *
> +	 * Note that "too large" here is a completely arbitrary limit.  The
> +	 * offset is logical concept not related to the physical byteoff and
> +	 * there is no fundamental underlying limit to it.  But it has been
> +	 * encoded in asserts and verifiers for a long time so we have to
> +	 * respect it.
> +	 */
> +	if (offset + data_size + data_overhead > args->geo->blksize) {
> +		if (!hole_offset)
> +			return 0;
> +		*byteoffp = byteoff;
> +		return hole_offset;
> +	}
> +
> +	*byteoffp = dp->i_disk_size;
> +	return offset;
> +}
> +
>  static void
>  xfs_dir2_sf_addname_common(
>  	struct xfs_da_args	*args,
> @@ -384,23 +427,29 @@ xfs_dir2_sf_addname_common(
>  
>  /*
>   * Add a name to a shortform directory.
> - * There are two algorithms, "easy" and "hard" which we decide on
> - * before changing anything.
> - * Convert to block form if necessary, if the new entry won't fit.
> + *
> + * Shortform directories are always tighty packed, and we simply allocate
> + * a new higher offset and add the entry at the end.
> + *
> + * While we could search for a hole in the offset space there really isn't
> + * much of a a point in doing so and complicating the algorithm.
> + *
> + * Convert to block form if the new entry won't fit.
>   */
> -int						/* error */
> +int
>  xfs_dir2_sf_addname(
> -	xfs_da_args_t		*args)		/* operation arguments */
> +	struct xfs_da_args	*args)
>  {
>  	struct xfs_inode	*dp = args->dp;
> +	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> -	int			error;		/* error return value */
> -	int			incr_isize;	/* total change in size */
> -	int			new_isize;	/* size after adding name */
> -	int			objchange;	/* changing to 8-byte inodes */
> -	xfs_dir2_data_aoff_t	offset = 0;	/* offset for new entry */
> -	int			pick;		/* which algorithm to use */
> -	xfs_dir2_sf_entry_t	*sfep = NULL;	/* shortform entry */
> +	struct xfs_dir2_sf_entry *sfep;
> +	xfs_dir2_data_aoff_t	offset;
> +	unsigned int		entsize;
> +	unsigned int		new_isize;
> +	unsigned int		byteoff;
> +	bool			objchange = false;
> +	int			error;
>  
>  	trace_xfs_dir2_sf_addname(args);
>  
> @@ -408,11 +457,12 @@ xfs_dir2_sf_addname(
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
> +
>  	/*
> -	 * Compute entry (and change in) size.
> +	 * Compute entry size and new inode size.
>  	 */
> -	incr_isize = xfs_dir2_sf_entsize(dp->i_mount, sfp, args->namelen);
> -	objchange = 0;
> +	entsize = xfs_dir2_sf_entsize(mp, sfp, args->namelen);
> +	new_isize = dp->i_disk_size + entsize;
>  
>  	/*
>  	 * Do we have to change to 8 byte inodes?
> @@ -421,19 +471,17 @@ xfs_dir2_sf_addname(
>  		/*
>  		 * Yes, adjust the inode size.  old count + (parent + new)
>  		 */
> -		incr_isize += (sfp->count + 2) * XFS_INO64_DIFF;
> -		objchange = 1;
> +		new_isize += (sfp->count + 2) * XFS_INO64_DIFF;
> +		objchange = true;
>  	}
>  
> -	new_isize = (int)dp->i_disk_size + incr_isize;
> -
>  	/*
>  	 * Too large to fit into the inode fork or too large offset?
>  	 */
>  	if (new_isize > xfs_inode_data_fork_size(dp))
>  		goto convert;
> -	pick = xfs_dir2_sf_addname_pick(args, objchange, &sfep, &offset);
> -	if (pick == 0)
> +	offset = xfs_dir2_sf_addname_pick_offset(args, &byteoff);
> +	if (!offset)
>  		goto convert;
>  
>  	/*
> @@ -451,20 +499,17 @@ xfs_dir2_sf_addname(
>  		return 0;
>  	}
>  
> +	sfp = xfs_idata_realloc(dp, entsize, XFS_DATA_FORK);
> +	sfep = dp->i_df.if_data + byteoff;
> +
>  	/*
> -	 * Do it the easy way - just add it at the end.
> -	 */
> -	if (pick == 1)
> -		xfs_dir2_sf_addname_easy(args, sfep, offset, new_isize);
> -	/*
> -	 * Do it the hard way - look for a place to insert the new entry.
> -	 * Convert to 8 byte inode numbers first if necessary.
> +	 * If we're inserting in the middle, move the tail out of the way first.
>  	 */
> -	else {
> -		ASSERT(pick == 2);
> -		xfs_dir2_sf_addname_hard(args, objchange, new_isize);
> +	if (byteoff < dp->i_disk_size) {
> +		memmove(sfep, (void *)sfep + entsize,
> +			dp->i_disk_size - byteoff);
>  	}
> -
> +	xfs_dir2_sf_addname_common(args, sfep, offset, objchange);
>  	dp->i_disk_size = new_isize;
>  	xfs_dir2_sf_check(args);
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_DDATA);
> @@ -482,192 +527,6 @@ xfs_dir2_sf_addname(
>  	return xfs_dir2_block_addname(args);
>  }
>  
> -/*
> - * Add the new entry the "easy" way.
> - * This is copying the old directory and adding the new entry at the end.
> - * Since it's sorted by "offset" we need room after the last offset
> - * that's already there, and then room to convert to a block directory.
> - * This is already checked by the pick routine.
> - */
> -static void
> -xfs_dir2_sf_addname_easy(
> -	xfs_da_args_t		*args,		/* operation arguments */
> -	xfs_dir2_sf_entry_t	*sfep,		/* pointer to new entry */
> -	xfs_dir2_data_aoff_t	offset,		/* offset to use for new ent */
> -	int			new_isize)	/* new directory size */
> -{
> -	struct xfs_inode	*dp = args->dp;
> -	struct xfs_mount	*mp = dp->i_mount;
> -	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> -	int			byteoff = (int)((char *)sfep - (char *)sfp);
> -
> -	/*
> -	 * Grow the in-inode space.
> -	 */
> -	sfp = xfs_idata_realloc(dp, xfs_dir2_sf_entsize(mp, sfp, args->namelen),
> -			  XFS_DATA_FORK);
> -	/*
> -	 * Need to set up again due to realloc of the inode data.
> -	 */
> -	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + byteoff);
> -	xfs_dir2_sf_addname_common(args, sfep, offset, false);
> -}
> -
> -/*
> - * Add the new entry the "hard" way.
> - * The caller has already converted to 8 byte inode numbers if necessary,
> - * in which case we need to leave the i8count at 1.
> - * Find a hole that the new entry will fit into, and copy
> - * the first part of the entries, the new entry, and the last part of
> - * the entries.
> - */
> -/* ARGSUSED */
> -static void
> -xfs_dir2_sf_addname_hard(
> -	xfs_da_args_t		*args,		/* operation arguments */
> -	int			objchange,	/* changing inode number size */
> -	int			new_isize)	/* new directory size */
> -{
> -	struct xfs_inode	*dp = args->dp;
> -	struct xfs_mount	*mp = dp->i_mount;
> -	int			add_datasize;	/* data size need for new ent */
> -	char			*buf;		/* buffer for old */
> -	int			eof;		/* reached end of old dir */
> -	int			nbytes;		/* temp for byte copies */
> -	xfs_dir2_data_aoff_t	new_offset;	/* next offset value */
> -	xfs_dir2_data_aoff_t	offset;		/* current offset value */
> -	int			old_isize;	/* previous size */
> -	xfs_dir2_sf_entry_t	*oldsfep;	/* entry in original dir */
> -	xfs_dir2_sf_hdr_t	*oldsfp;	/* original shortform dir */
> -	xfs_dir2_sf_entry_t	*sfep;		/* entry in new dir */
> -	xfs_dir2_sf_hdr_t	*sfp;		/* new shortform dir */
> -
> -	/*
> -	 * Copy the old directory to the stack buffer.
> -	 */
> -	old_isize = (int)dp->i_disk_size;
> -	buf = kmalloc(old_isize, GFP_KERNEL | __GFP_NOFAIL);
> -	oldsfp = (xfs_dir2_sf_hdr_t *)buf;
> -	memcpy(oldsfp, dp->i_df.if_data, old_isize);
> -	/*
> -	 * Loop over the old directory finding the place we're going
> -	 * to insert the new entry.
> -	 * If it's going to end up at the end then oldsfep will point there.
> -	 */
> -	for (offset = args->geo->data_first_offset,
> -	      oldsfep = xfs_dir2_sf_firstentry(oldsfp),
> -	      add_datasize = xfs_dir2_data_entsize(mp, args->namelen),
> -	      eof = (char *)oldsfep == &buf[old_isize];
> -	     !eof;
> -	     offset = new_offset + xfs_dir2_data_entsize(mp, oldsfep->namelen),
> -	      oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep),
> -	      eof = (char *)oldsfep == &buf[old_isize]) {
> -		new_offset = xfs_dir2_sf_get_offset(oldsfep);
> -		if (offset + add_datasize <= new_offset)
> -			break;
> -	}
> -	/*
> -	 * Get rid of the old directory, then allocate space for
> -	 * the new one.  We do this so xfs_idata_realloc won't copy
> -	 * the data.
> -	 */
> -	xfs_idata_realloc(dp, -old_isize, XFS_DATA_FORK);
> -	sfp = xfs_idata_realloc(dp, new_isize, XFS_DATA_FORK);
> -
> -	/*
> -	 * Copy the first part of the directory, including the header.
> -	 */
> -	nbytes = (int)((char *)oldsfep - (char *)oldsfp);
> -	memcpy(sfp, oldsfp, nbytes);
> -	sfep = (xfs_dir2_sf_entry_t *)((char *)sfp + nbytes);
> -
> -	/*
> -	 * Fill in the new entry, and update the header counts.
> -	 */
> -	xfs_dir2_sf_addname_common(args, sfep, offset, objchange);
> -
> -	/*
> -	 * If there's more left to copy, do that.
> -	 */
> -	if (!eof) {
> -		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
> -		memcpy(sfep, oldsfep, old_isize - nbytes);
> -	}
> -	kfree(buf);
> -}
> -
> -/*
> - * Decide if the new entry will fit at all.
> - * If it will fit, pick between adding the new entry to the end (easy)
> - * or somewhere else (hard).
> - * Return 0 (won't fit), 1 (easy), 2 (hard).
> - */
> -/*ARGSUSED*/
> -static int					/* pick result */
> -xfs_dir2_sf_addname_pick(
> -	xfs_da_args_t		*args,		/* operation arguments */
> -	int			objchange,	/* inode # size changes */
> -	xfs_dir2_sf_entry_t	**sfepp,	/* out(1): new entry ptr */
> -	xfs_dir2_data_aoff_t	*offsetp)	/* out(1): new offset */
> -{
> -	struct xfs_inode	*dp = args->dp;
> -	struct xfs_mount	*mp = dp->i_mount;
> -	int			holefit;	/* found hole it will fit in */
> -	int			i;		/* entry number */
> -	xfs_dir2_data_aoff_t	offset;		/* data block offset */
> -	xfs_dir2_sf_entry_t	*sfep;		/* shortform entry */
> -	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> -	int			size;		/* entry's data size */
> -	int			used;		/* data bytes used */
> -
> -	size = xfs_dir2_data_entsize(mp, args->namelen);
> -	offset = args->geo->data_first_offset;
> -	sfep = xfs_dir2_sf_firstentry(sfp);
> -	holefit = 0;
> -	/*
> -	 * Loop over sf entries.
> -	 * Keep track of data offset and whether we've seen a place
> -	 * to insert the new entry.
> -	 */
> -	for (i = 0; i < sfp->count; i++) {
> -		if (!holefit)
> -			holefit = offset + size <= xfs_dir2_sf_get_offset(sfep);
> -		if (holefit)
> -			*offsetp = offset;
> -		offset = xfs_dir2_sf_get_offset(sfep) +
> -			 xfs_dir2_data_entsize(mp, sfep->namelen);
> -		sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep);
> -	}
> -	/*
> -	 * Calculate data bytes used excluding the new entry, if this
> -	 * was a data block (block form directory).
> -	 */
> -	used = offset + xfs_dir2_block_overhead(sfp->count + 1);
> -	/*
> -	 * If it won't fit in a block form then we can't insert it,
> -	 * we'll go back, convert to block, then try the insert and convert
> -	 * to leaf.
> -	 */
> -	if (used + (holefit ? 0 : size) > args->geo->blksize)
> -		return 0;
> -	/*
> -	 * If changing the inode number size, do it the hard way.
> -	 */
> -	if (objchange)
> -		return 2;
> -	/*
> -	 * If it won't fit at the end then do it the hard way (use the hole).
> -	 */
> -	if (used + size > args->geo->blksize)
> -		return 2;
> -	/*
> -	 * Do it the easy way.
> -	 */
> -	*sfepp = sfep;
> -	*offsetp = offset;
> -	return 1;
> -}
> -
>  #ifdef DEBUG
>  /*
>   * Check consistency of shortform directory, assert if bad.
> -- 
> 2.39.2
> 
> 

Re: [PATCH 16/16] xfs: make the hard case in xfs_dir2_sf_addname less hard

[kernel test robot]

Hello,

kernel test robot noticed "dmesg.WARNING:at_fs/xfs/xfs_message.c:#asswarn[xfs]" on:

commit: cbc81dd4e3e501eb8888bf412f7d4fdd8c416927 ("[PATCH 16/16] xfs: make the hard case in xfs_dir2_sf_addname less hard")
url: https://github.com/intel-lab-lkp/linux/commits/Christoph-Hellwig/xfs-remove-an-extra-buffer-allocation-in-xfs_attr_shortform_to_leaf/20240430-214950
base: https://git.kernel.org/cgit/fs/xfs/xfs-linux.git for-next
patch link: https://lore.kernel.org/all/20240430124926.1775355-17-hch@lst.de/
patch subject: [PATCH 16/16] xfs: make the hard case in xfs_dir2_sf_addname less hard

in testcase: filebench
version: filebench-x86_64-22620e6-1_20240224
with following parameters:

	disk: 1HDD
	fs: xfs
	test: fileserver.f
	cpufreq_governor: performance

compiler: gcc-13
test machine: 128 threads 2 sockets Intel(R) Xeon(R) Platinum 8358 CPU @ 2.60GHz (Ice Lake) with 128G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <yujie.liu@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202405101352.bcf759f-lkp@intel.com


[   41.583616][ T4699] XFS: Assertion failed: xfs_dir2_sf_lookup(args) == -ENOENT, file: fs/xfs/libxfs/xfs_dir2_sf.c, line: 456
[   41.595114][ T4699] ------------[ cut here ]------------
[ 41.600675][ T4699] WARNING: CPU: 66 PID: 4699 at fs/xfs/xfs_message.c:89 asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[   41.609938][ T4699] Modules linked in: xfs device_dax(+) nd_pmem nd_btt dax_pmem intel_rapl_msr intel_rapl_common btrfs x86_pkg_temp_thermal intel_powerclamp blake2b_generic xor coretemp raid6_pq libcrc32c sd_mod t10_pi crc64_rocksoft_generic crc64_rocksoft crc64 kvm_intel sg kvm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel sha512_ssse3 rapl ahci ipmi_ssif ast libahci intel_cstate binfmt_misc acpi_ipmi drm_shmem_helper mei_me ipmi_si i2c_i801 ioatdma dax_hmem intel_uncore libata drm_kms_helper mei i2c_smbus ipmi_devintf intel_pch_thermal nfit wmi dca ipmi_msghandler libnvdimm acpi_pad acpi_power_meter joydev drm loop fuse dm_mod ip_tables
[   41.669439][ T4699] CPU: 66 PID: 4699 Comm: filebench Not tainted 6.9.0-rc4-00238-gcbc81dd4e3e5 #1
[ 41.678673][ T4699] RIP: 0010:asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 41.684114][ T4699] Code: 90 90 66 0f 1f 00 0f 1f 44 00 00 49 89 d0 41 89 c9 48 c7 c2 18 cd 45 c1 48 89 f1 48 89 fe 48 c7 c7 c8 e6 44 c1 e8 18 fd ff ff <0f> 0b c3 cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90
All code
========
   0:   90                      nop
   1:   90                      nop
   2:   66 0f 1f 00             nopw   (%rax)
   6:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
   b:   49 89 d0                mov    %rdx,%r8
   e:   41 89 c9                mov    %ecx,%r9d
  11:   48 c7 c2 18 cd 45 c1    mov    $0xffffffffc145cd18,%rdx
  18:   48 89 f1                mov    %rsi,%rcx
  1b:   48 89 fe                mov    %rdi,%rsi
  1e:   48 c7 c7 c8 e6 44 c1    mov    $0xffffffffc144e6c8,%rdi
  25:   e8 18 fd ff ff          call   0xfffffffffffffd42
  2a:*  0f 0b                   ud2             <-- trapping instruction
  2c:   c3                      ret
  2d:   cc                      int3
  2e:   cc                      int3
  2f:   cc                      int3
  30:   cc                      int3
  31:   90                      nop
  32:   90                      nop
  33:   90                      nop
  34:   90                      nop
  35:   90                      nop
  36:   90                      nop
  37:   90                      nop
  38:   90                      nop
  39:   90                      nop
  3a:   90                      nop
  3b:   90                      nop
  3c:   90                      nop
  3d:   90                      nop
  3e:   90                      nop
  3f:   90                      nop

Code starting with the faulting instruction
===========================================
   0:   0f 0b                   ud2
   2:   c3                      ret
   3:   cc                      int3
   4:   cc                      int3
   5:   cc                      int3
   6:   cc                      int3
   7:   90                      nop
   8:   90                      nop
   9:   90                      nop
   a:   90                      nop
   b:   90                      nop
   c:   90                      nop
   d:   90                      nop
   e:   90                      nop
   f:   90                      nop
  10:   90                      nop
  11:   90                      nop
  12:   90                      nop
  13:   90                      nop
  14:   90                      nop
  15:   90                      nop
[   41.704055][ T4699] RSP: 0018:ffa0000020c4faa0 EFLAGS: 00010246
[   41.710227][ T4699] RAX: 0000000000000000 RBX: ff11000105112300 RCX: 000000007fffffff
[   41.718304][ T4699] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffc144e6c8
[   41.726378][ T4699] RBP: ff1100010067c000 R08: 0000000000000000 R09: 000000000000000a
[   41.734449][ T4699] R10: 000000000000000a R11: 0fffffffffffffff R12: ffa0000020c4fc28
[   41.742526][ T4699] R13: ff1100013d163200 R14: ff11000107e5f000 R15: 0000000000000023
[   41.750622][ T4699] FS:  00007fffd14006c0(0000) GS:ff1100103fa80000(0000) knlGS:0000000000000000
[   41.759686][ T4699] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   41.766378][ T4699] CR2: 00007ffff7d50000 CR3: 0000000159594005 CR4: 0000000000771ef0
[   41.774460][ T4699] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   41.782532][ T4699] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   41.790619][ T4699] PKRU: 55555554
[   41.794261][ T4699] Call Trace:
[   41.797669][ T4699]  <TASK>
[ 41.800737][ T4699] ? __warn (kernel/panic.c:694)
[ 41.804898][ T4699] ? asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 41.809761][ T4699] ? report_bug (lib/bug.c:180 lib/bug.c:219)
[ 41.814357][ T4699] ? handle_bug (arch/x86/kernel/traps.c:239)
[ 41.818819][ T4699] ? exc_invalid_op (arch/x86/kernel/traps.c:260 (discriminator 1))
[ 41.823593][ T4699] ? asm_exc_invalid_op (arch/x86/include/asm/idtentry.h:621)
[ 41.828743][ T4699] ? asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 41.833541][ T4699] xfs_dir2_sf_addname (fs/xfs/libxfs/xfs_dir2_sf.c:457 (discriminator 1)) xfs
[ 41.839390][ T4699] xfs_dir_createname (fs/xfs/libxfs/xfs_dir2.c:357) xfs
[ 41.845125][ T4699] xfs_create (fs/xfs/xfs_inode.c:1116) xfs
[ 41.850168][ T4699] ? generic_permission (fs/namei.c:346 (discriminator 1) fs/namei.c:407 (discriminator 1))
[ 41.855344][ T4699] xfs_generic_create (fs/xfs/xfs_iops.c:202 (discriminator 1)) xfs
[ 41.861071][ T4699] ? generic_permission (fs/namei.c:346 (discriminator 1) fs/namei.c:407 (discriminator 1))
[ 41.866237][ T4699] lookup_open+0x4c8/0x570
[ 41.871311][ T4699] open_last_lookups (fs/namei.c:3567 (discriminator 1))
[ 41.876301][ T4699] path_openat (fs/namei.c:3796)
[ 41.880726][ T4699] do_filp_open (fs/namei.c:3826)
[ 41.885194][ T4699] do_sys_openat2 (fs/open.c:1406)
[ 41.889761][ T4699] __x64_sys_openat (fs/open.c:1432)
[ 41.894487][ T4699] do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1))
[ 41.899034][ T4699] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
[   41.904966][ T4699] RIP: 0033:0x7ffff7df0f80
[ 41.909418][ T4699] Code: 48 89 44 24 20 75 93 44 89 54 24 0c e8 39 d8 f8 ff 44 8b 54 24 0c 89 da 48 89 ee 41 89 c0 bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 38 44 89 c7 89 44 24 0c e8 8c d8 f8 ff 8b 44
All code
========
   0:   48 89 44 24 20          mov    %rax,0x20(%rsp)
   5:   75 93                   jne    0xffffffffffffff9a
   7:   44 89 54 24 0c          mov    %r10d,0xc(%rsp)
   c:   e8 39 d8 f8 ff          call   0xfffffffffff8d84a
  11:   44 8b 54 24 0c          mov    0xc(%rsp),%r10d
  16:   89 da                   mov    %ebx,%edx
  18:   48 89 ee                mov    %rbp,%rsi
  1b:   41 89 c0                mov    %eax,%r8d
  1e:   bf 9c ff ff ff          mov    $0xffffff9c,%edi
  23:   b8 01 01 00 00          mov    $0x101,%eax
  28:   0f 05                   syscall
  2a:*  48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax         <-- trapping instruction
  30:   77 38                   ja     0x6a
  32:   44 89 c7                mov    %r8d,%edi
  35:   89 44 24 0c             mov    %eax,0xc(%rsp)
  39:   e8 8c d8 f8 ff          call   0xfffffffffff8d8ca
  3e:   8b                      .byte 0x8b
  3f:   44                      rex.R

Code starting with the faulting instruction
===========================================
   0:   48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
   6:   77 38                   ja     0x40
   8:   44 89 c7                mov    %r8d,%edi
   b:   89 44 24 0c             mov    %eax,0xc(%rsp)
   f:   e8 8c d8 f8 ff          call   0xfffffffffff8d8a0
  14:   8b                      .byte 0x8b
  15:   44                      rex.R
[   41.929230][ T4699] RSP: 002b:00007fffd13fdcd0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
[   41.937705][ T4699] RAX: ffffffffffffffda RBX: 0000000000000042 RCX: 00007ffff7df0f80
[   41.945760][ T4699] RDX: 0000000000000042 RSI: 00007fffd13fde00 RDI: 00000000ffffff9c
[   41.953766][ T4699] RBP: 00007fffd13fde00 R08: 0000000000000000 R09: 00007ffeb4000b70
[   41.961774][ T4699] R10: 00000000000001b6 R11: 0000000000000293 R12: 0000000000000000
[   41.969773][ T4699] R13: 00007fffef5b6148 R14: 00007fffd13fee00 R15: 0000000000000040
[   41.977770][ T4699]  </TASK>
[   41.980813][ T4699] ---[ end trace 0000000000000000 ]---
[   42.196053][ T4696] XFS: Assertion failed: error != -ENOENT, file: fs/xfs/xfs_inode.c, line: 2827
[   42.205189][ T4696] ------------[ cut here ]------------
[ 42.210734][ T4696] WARNING: CPU: 28 PID: 4696 at fs/xfs/xfs_message.c:89 asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[   42.219964][ T4696] Modules linked in: xfs device_dax(+) nd_pmem nd_btt dax_pmem intel_rapl_msr intel_rapl_common btrfs x86_pkg_temp_thermal intel_powerclamp blake2b_generic xor coretemp raid6_pq libcrc32c sd_mod t10_pi crc64_rocksoft_generic crc64_rocksoft crc64 kvm_intel sg kvm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel sha512_ssse3 rapl ahci ipmi_ssif ast libahci intel_cstate binfmt_misc acpi_ipmi drm_shmem_helper mei_me ipmi_si i2c_i801 ioatdma dax_hmem intel_uncore libata drm_kms_helper mei i2c_smbus ipmi_devintf intel_pch_thermal nfit wmi dca ipmi_msghandler libnvdimm acpi_pad acpi_power_meter joydev drm loop fuse dm_mod ip_tables
[   42.279151][ T4696] CPU: 28 PID: 4696 Comm: filebench Tainted: G        W          6.9.0-rc4-00238-gcbc81dd4e3e5 #1
[ 42.289767][ T4696] RIP: 0010:asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 42.295150][ T4696] Code: 90 90 66 0f 1f 00 0f 1f 44 00 00 49 89 d0 41 89 c9 48 c7 c2 18 cd 45 c1 48 89 f1 48 89 fe 48 c7 c7 c8 e6 44 c1 e8 18 fd ff ff <0f> 0b c3 cc cc cc cc 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90
All code
========
   0:   90                      nop
   1:   90                      nop
   2:   66 0f 1f 00             nopw   (%rax)
   6:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
   b:   49 89 d0                mov    %rdx,%r8
   e:   41 89 c9                mov    %ecx,%r9d
  11:   48 c7 c2 18 cd 45 c1    mov    $0xffffffffc145cd18,%rdx
  18:   48 89 f1                mov    %rsi,%rcx
  1b:   48 89 fe                mov    %rdi,%rsi
  1e:   48 c7 c7 c8 e6 44 c1    mov    $0xffffffffc144e6c8,%rdi
  25:   e8 18 fd ff ff          call   0xfffffffffffffd42
  2a:*  0f 0b                   ud2             <-- trapping instruction
  2c:   c3                      ret
  2d:   cc                      int3
  2e:   cc                      int3
  2f:   cc                      int3
  30:   cc                      int3
  31:   90                      nop
  32:   90                      nop
  33:   90                      nop
  34:   90                      nop
  35:   90                      nop
  36:   90                      nop
  37:   90                      nop
  38:   90                      nop
  39:   90                      nop
  3a:   90                      nop
  3b:   90                      nop
  3c:   90                      nop
  3d:   90                      nop
  3e:   90                      nop
  3f:   90                      nop

Code starting with the faulting instruction
===========================================
   0:   0f 0b                   ud2
   2:   c3                      ret
   3:   cc                      int3
   4:   cc                      int3
   5:   cc                      int3
   6:   cc                      int3
   7:   90                      nop
   8:   90                      nop
   9:   90                      nop
   a:   90                      nop
   b:   90                      nop
   c:   90                      nop
   d:   90                      nop
   e:   90                      nop
   f:   90                      nop
  10:   90                      nop
  11:   90                      nop
  12:   90                      nop
  13:   90                      nop
  14:   90                      nop
  15:   90                      nop
[   42.314977][ T4696] RSP: 0018:ffa0000020c37db8 EFLAGS: 00010246
[   42.321096][ T4696] RAX: 0000000000000000 RBX: ff1100017eccc400 RCX: 000000007fffffff
[   42.329126][ T4696] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffc144e6c8
[   42.337154][ T4696] RBP: ffa0000020c37e40 R08: 0000000000000000 R09: 000000000000000a
[   42.345178][ T4696] R10: 000000000000000a R11: 0fffffffffffffff R12: ff11000107e5f000
[   42.353204][ T4696] R13: 0000000000008000 R14: 0000000000000000 R15: ff1100010067c000
[   42.361240][ T4696] FS:  00007fffd32006c0(0000) GS:ff1100103f900000(0000) knlGS:0000000000000000
[   42.370230][ T4696] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   42.376878][ T4696] CR2: 0000555555579cf0 CR3: 0000000159594005 CR4: 0000000000771ef0
[   42.384913][ T4696] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   42.392951][ T4696] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   42.400990][ T4696] PKRU: 55555554
[   42.404617][ T4696] Call Trace:
[   42.407980][ T4696]  <TASK>
[ 42.410992][ T4696] ? __warn (kernel/panic.c:694)
[ 42.415133][ T4696] ? asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 42.419927][ T4696] ? report_bug (lib/bug.c:180 lib/bug.c:219)
[ 42.424498][ T4696] ? handle_bug (arch/x86/kernel/traps.c:239)
[ 42.428892][ T4696] ? exc_invalid_op (arch/x86/kernel/traps.c:260 (discriminator 1))
[ 42.433649][ T4696] ? asm_exc_invalid_op (arch/x86/include/asm/idtentry.h:621)
[ 42.438775][ T4696] ? asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 42.443560][ T4696] ? asswarn (fs/xfs/xfs_message.c:89 (discriminator 1)) xfs
[ 42.448341][ T4696] xfs_remove (fs/xfs/xfs_inode.c:2827 (discriminator 1)) xfs
[ 42.453379][ T4696] xfs_vn_unlink (fs/xfs/xfs_iops.c:404) xfs
[ 42.458499][ T4696] vfs_unlink (fs/namei.c:4335)
[ 42.462895][ T4696] do_unlinkat (fs/namei.c:4399 (discriminator 1))
[ 42.467367][ T4696] __x64_sys_unlink (fs/namei.c:4445)
[ 42.472097][ T4696] do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1))
[ 42.476661][ T4696] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
[   42.482607][ T4696] RIP: 0033:0x7ffff7df2a07
[ 42.487068][ T4696] Code: f0 ff ff 73 01 c3 48 8b 0d f6 83 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 b8 57 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 83 0d 00 f7 d8 64 89 01 48
All code
========
   0:   f0 ff                   lock (bad)
   2:   ff 73 01                push   0x1(%rbx)
   5:   c3                      ret
   6:   48 8b 0d f6 83 0d 00    mov    0xd83f6(%rip),%rcx        # 0xd8403
   d:   f7 d8                   neg    %eax
   f:   64 89 01                mov    %eax,%fs:(%rcx)
  12:   48 83 c8 ff             or     $0xffffffffffffffff,%rax
  16:   c3                      ret
  17:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
  1e:   00 00 00
  21:   66 90                   xchg   %ax,%ax
  23:   b8 57 00 00 00          mov    $0x57,%eax
  28:   0f 05                   syscall
  2a:*  48 3d 01 f0 ff ff       cmp    $0xfffffffffffff001,%rax         <-- trapping instruction
  30:   73 01                   jae    0x33
  32:   c3                      ret
  33:   48 8b 0d c9 83 0d 00    mov    0xd83c9(%rip),%rcx        # 0xd8403
  3a:   f7 d8                   neg    %eax
  3c:   64 89 01                mov    %eax,%fs:(%rcx)
  3f:   48                      rex.W

Code starting with the faulting instruction
===========================================
   0:   48 3d 01 f0 ff ff       cmp    $0xfffffffffffff001,%rax
   6:   73 01                   jae    0x9
   8:   c3                      ret
   9:   48 8b 0d c9 83 0d 00    mov    0xd83c9(%rip),%rcx        # 0xd83d9
  10:   f7 d8                   neg    %eax
  12:   64 89 01                mov    %eax,%fs:(%rcx)
  15:   48                      rex.W
[   42.506970][ T4696] RSP: 002b:00007fffd31fee38 EFLAGS: 00000206 ORIG_RAX: 0000000000000057
[   42.515426][ T4696] RAX: ffffffffffffffda RBX: 00007fffef5b5c08 RCX: 00007ffff7df2a07
[   42.523454][ T4696] RDX: 0000000000000000 RSI: 000000006638299e RDI: 00007fffd31fee60
[   42.531478][ T4696] RBP: 00007fffd31fee60 R08: 0000000000000007 R09: 00007ffed4000b70
[   42.539501][ T4696] R10: 00007fffd31fee00 R11: 0000000000000206 R12: 00007ffed4000b70
[   42.547525][ T4696] R13: 00007ffff55e3ae0 R14: 00007ffff5940c08 R15: 00007fffd2a00000
[   42.555540][ T4696]  </TASK>
[   42.558619][ T4696] ---[ end trace 0000000000000000 ]---
[ 42.564118][ T4696] XFS (sdd1): Internal error xfs_trans_cancel at line 1112 of file fs/xfs/xfs_trans.c. Caller xfs_remove (fs/xfs/xfs_inode.c:2865) xfs
[   42.577304][ T4696] CPU: 28 PID: 4696 Comm: filebench Tainted: G        W          6.9.0-rc4-00238-gcbc81dd4e3e5 #1
[   42.587946][ T4696] Call Trace:
[   42.591293][ T4696]  <TASK>
[ 42.594294][ T4696] dump_stack_lvl (lib/dump_stack.c:117)
[ 42.598846][ T4696] xfs_trans_cancel (fs/xfs/xfs_trans.c:1113) xfs
[ 42.604396][ T4696] xfs_remove (fs/xfs/xfs_inode.c:2865) xfs
[ 42.609419][ T4696] xfs_vn_unlink (fs/xfs/xfs_iops.c:404) xfs
[ 42.614520][ T4696] vfs_unlink (fs/namei.c:4335)
[ 42.618888][ T4696] do_unlinkat (fs/namei.c:4399 (discriminator 1))
[ 42.623340][ T4696] __x64_sys_unlink (fs/namei.c:4445)
[ 42.628051][ T4696] do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1))
[ 42.632585][ T4696] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
[   42.638504][ T4696] RIP: 0033:0x7ffff7df2a07
[ 42.642945][ T4696] Code: f0 ff ff 73 01 c3 48 8b 0d f6 83 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 b8 57 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 83 0d 00 f7 d8 64 89 01 48
All code
========
   0:   f0 ff                   lock (bad)
   2:   ff 73 01                push   0x1(%rbx)
   5:   c3                      ret
   6:   48 8b 0d f6 83 0d 00    mov    0xd83f6(%rip),%rcx        # 0xd8403
   d:   f7 d8                   neg    %eax
   f:   64 89 01                mov    %eax,%fs:(%rcx)
  12:   48 83 c8 ff             or     $0xffffffffffffffff,%rax
  16:   c3                      ret
  17:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
  1e:   00 00 00
  21:   66 90                   xchg   %ax,%ax
  23:   b8 57 00 00 00          mov    $0x57,%eax
  28:   0f 05                   syscall
  2a:*  48 3d 01 f0 ff ff       cmp    $0xfffffffffffff001,%rax         <-- trapping instruction
  30:   73 01                   jae    0x33
  32:   c3                      ret
  33:   48 8b 0d c9 83 0d 00    mov    0xd83c9(%rip),%rcx        # 0xd8403
  3a:   f7 d8                   neg    %eax
  3c:   64 89 01                mov    %eax,%fs:(%rcx)
  3f:   48                      rex.W

Code starting with the faulting instruction
===========================================
   0:   48 3d 01 f0 ff ff       cmp    $0xfffffffffffff001,%rax
   6:   73 01                   jae    0x9
   8:   c3                      ret
   9:   48 8b 0d c9 83 0d 00    mov    0xd83c9(%rip),%rcx        # 0xd83d9
  10:   f7 d8                   neg    %eax
  12:   64 89 01                mov    %eax,%fs:(%rcx)
  15:   48                      rex.W
[   42.662759][ T4696] RSP: 002b:00007fffd31fee38 EFLAGS: 00000206 ORIG_RAX: 0000000000000057
[   42.671206][ T4696] RAX: ffffffffffffffda RBX: 00007fffef5b5c08 RCX: 00007ffff7df2a07
[   42.679223][ T4696] RDX: 0000000000000000 RSI: 000000006638299e RDI: 00007fffd31fee60
[   42.687242][ T4696] RBP: 00007fffd31fee60 R08: 0000000000000007 R09: 00007ffed4000b70
[   42.695260][ T4696] R10: 00007fffd31fee00 R11: 0000000000000206 R12: 00007ffed4000b70
[   42.703276][ T4696] R13: 00007ffff55e3ae0 R14: 00007ffff5940c08 R15: 00007fffd2a00000
[   42.711295][ T4696]  </TASK>


The kernel config is available at:
https://download.01.org/0day-ci/archive/20240510/202405101352.bcf759f-lkp@intel.com


-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki