[PATCH 0/4] xfs: more code refactoring

[PATCH 0/4] xfs: more code refactoring

[Eric Sandeen]

combine some directory & rt code which was cut&paste
long ago...

 libxfs/xfs_dir2.c     |   67 +++---------------
 libxfs/xfs_dir2.h     |    2 
 libxfs/xfs_rtbitmap.c |   45 ++++++++++--
 xfs_file.c            |  178 ++++++++++++++------------------------------------
 xfs_inode.c           |   24 ++++--
 xfs_log_recover.c     |   83 ++++++++---------------
 xfs_rtalloc.c         |   55 ---------------
 xfs_rtalloc.h         |    4 +
 xfs_symlink.c         |    8 +-
 9 files changed, 158 insertions(+), 308 deletions(-)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/4] xfs: check resblks before calling xfs_dir_canenter

[Eric Sandeen]

Move the resblks test out of the xfs_dir_canenter,
and into the caller.

This makes a little more sense on the face of it;
xfs_dir_canenter immediately returns if resblks !=0;
and given some of the comments preceding the calls:

 * Check for ability to enter directory entry, if no space reserved.

even more so.

It also facilitates the next patch.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 6cef221..ea84e1c 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -535,22 +535,17 @@ out_free:
 
 /*
  * See if this entry can be added to the directory without allocating space.
- * First checks that the caller couldn't reserve enough space (resblks = 0).
  */
 int
 xfs_dir_canenter(
 	xfs_trans_t	*tp,
 	xfs_inode_t	*dp,
-	struct xfs_name	*name,		/* name of entry to add */
-	uint		resblks)
+	struct xfs_name	*name)		/* name of entry to add */
 {
 	struct xfs_da_args *args;
 	int		rval;
 	int		v;		/* type-checking value */
 
-	if (resblks)
-		return 0;
-
 	ASSERT(S_ISDIR(dp->i_d.di_mode));
 
 	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index c8e86b0..4dff261 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -136,7 +136,7 @@ extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
 				xfs_fsblock_t *first,
 				struct xfs_bmap_free *flist, xfs_extlen_t tot);
 extern int xfs_dir_canenter(struct xfs_trans *tp, struct xfs_inode *dp,
-				struct xfs_name *name, uint resblks);
+				struct xfs_name *name);
 
 /*
  * Direct call from the bmap code, bypassing the generic directory layer.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..c92cb48 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1153,9 +1153,11 @@ xfs_create(
 	if (error)
 		goto out_trans_cancel;
 
-	error = xfs_dir_canenter(tp, dp, name, resblks);
-	if (error)
-		goto out_trans_cancel;
+	if (!resblks) {
+		error = xfs_dir_canenter(tp, dp, name);
+		if (error)
+			goto out_trans_cancel;
+	}
 
 	/*
 	 * A newly created regular or special file just has one directory
@@ -1421,9 +1423,11 @@ xfs_link(
 		goto error_return;
 	}
 
-	error = xfs_dir_canenter(tp, tdp, target_name, resblks);
-	if (error)
-		goto error_return;
+	if (!resblks) {
+		error = xfs_dir_canenter(tp, tdp, target_name);
+		if (error)
+			goto error_return;
+	}
 
 	xfs_bmap_init(&free_list, &first_block);
 
@@ -2759,9 +2763,11 @@ xfs_rename(
 		 * If there's no space reservation, check the entry will
 		 * fit before actually inserting it.
 		 */
-		error = xfs_dir_canenter(tp, target_dp, target_name, spaceres);
-		if (error)
-			goto error_return;
+		if (!spaceres) {
+			error = xfs_dir_canenter(tp, target_dp, target_name);
+			if (error)
+				goto error_return;
+		}
 		/*
 		 * If target does not exist and the rename crosses
 		 * directories, adjust the target directory link count
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 6a944a2..02ae62a 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -269,9 +269,11 @@ xfs_symlink(
 	/*
 	 * Check for ability to enter directory entry, if no space reserved.
 	 */
-	error = xfs_dir_canenter(tp, dp, link_name, resblks);
-	if (error)
-		goto error_return;
+	if (!resblks) {
+		error = xfs_dir_canenter(tp, dp, link_name);
+		if (error)
+			goto error_return;
+	}
 	/*
 	 * Initialize the bmap freelist prior to calling either
 	 * bmapi or the directory create code.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] xfs: check resblks before calling xfs_dir_canenter

[Brian Foster]

On Thu, Aug 21, 2014 at 07:55:21PM -0500, Eric Sandeen wrote:
> Move the resblks test out of the xfs_dir_canenter,
> and into the caller.
> 
> This makes a little more sense on the face of it;
> xfs_dir_canenter immediately returns if resblks !=0;
> and given some of the comments preceding the calls:
> 
>  * Check for ability to enter directory entry, if no space reserved.
> 
> even more so.
> 
> It also facilitates the next patch.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 6cef221..ea84e1c 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -535,22 +535,17 @@ out_free:
>  
>  /*
>   * See if this entry can be added to the directory without allocating space.
> - * First checks that the caller couldn't reserve enough space (resblks = 0).
>   */
>  int
>  xfs_dir_canenter(
>  	xfs_trans_t	*tp,
>  	xfs_inode_t	*dp,
> -	struct xfs_name	*name,		/* name of entry to add */
> -	uint		resblks)
> +	struct xfs_name	*name)		/* name of entry to add */
>  {
>  	struct xfs_da_args *args;
>  	int		rval;
>  	int		v;		/* type-checking value */
>  
> -	if (resblks)
> -		return 0;
> -
>  	ASSERT(S_ISDIR(dp->i_d.di_mode));
>  
>  	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index c8e86b0..4dff261 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -136,7 +136,7 @@ extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
>  				xfs_fsblock_t *first,
>  				struct xfs_bmap_free *flist, xfs_extlen_t tot);
>  extern int xfs_dir_canenter(struct xfs_trans *tp, struct xfs_inode *dp,
> -				struct xfs_name *name, uint resblks);
> +				struct xfs_name *name);
>  
>  /*
>   * Direct call from the bmap code, bypassing the generic directory layer.
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index fea3c92..c92cb48 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1153,9 +1153,11 @@ xfs_create(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_dir_canenter(tp, dp, name, resblks);
> -	if (error)
> -		goto out_trans_cancel;
> +	if (!resblks) {
> +		error = xfs_dir_canenter(tp, dp, name);
> +		if (error)
> +			goto out_trans_cancel;
> +	}
>  
>  	/*
>  	 * A newly created regular or special file just has one directory
> @@ -1421,9 +1423,11 @@ xfs_link(
>  		goto error_return;
>  	}
>  
> -	error = xfs_dir_canenter(tp, tdp, target_name, resblks);
> -	if (error)
> -		goto error_return;
> +	if (!resblks) {
> +		error = xfs_dir_canenter(tp, tdp, target_name);
> +		if (error)
> +			goto error_return;
> +	}
>  
>  	xfs_bmap_init(&free_list, &first_block);
>  
> @@ -2759,9 +2763,11 @@ xfs_rename(
>  		 * If there's no space reservation, check the entry will
>  		 * fit before actually inserting it.
>  		 */
> -		error = xfs_dir_canenter(tp, target_dp, target_name, spaceres);
> -		if (error)
> -			goto error_return;
> +		if (!spaceres) {
> +			error = xfs_dir_canenter(tp, target_dp, target_name);
> +			if (error)
> +				goto error_return;
> +		}
>  		/*
>  		 * If target does not exist and the rename crosses
>  		 * directories, adjust the target directory link count
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 6a944a2..02ae62a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -269,9 +269,11 @@ xfs_symlink(
>  	/*
>  	 * Check for ability to enter directory entry, if no space reserved.
>  	 */
> -	error = xfs_dir_canenter(tp, dp, link_name, resblks);
> -	if (error)
> -		goto error_return;
> +	if (!resblks) {
> +		error = xfs_dir_canenter(tp, dp, link_name);
> +		if (error)
> +			goto error_return;
> +	}
>  	/*
>  	 * Initialize the bmap freelist prior to calling either
>  	 * bmapi or the directory create code.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] xfs: check resblks before calling xfs_dir_canenter

[Christoph Hellwig]

Looks good (although not useful on it's own, but there's more followups)

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4] xfs: combine xfs_dir_canenter into xfs_dir_createname

[Eric Sandeen]

xfs_dir_canenter and xfs_dir_createname are
almost identical.

Fold the former into the latter, with a helpful
wrapper for the former.  If createname is called without
an inode number, it now only checks for space, and does
not actually add the entry.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index ea84e1c..fdd391f 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -237,7 +237,8 @@ xfs_dir_init(
 }
 
 /*
-  Enter a name in a directory.
+ * Enter a name in a directory.
+ * If inum is 0, only test for available space.
  */
 int
 xfs_dir_createname(
@@ -254,10 +255,12 @@ xfs_dir_createname(
 	int			v;		/* type-checking value */
 
 	ASSERT(S_ISDIR(dp->i_d.di_mode));
-	rval = xfs_dir_ino_validate(tp->t_mountp, inum);
-	if (rval)
-		return rval;
-	XFS_STATS_INC(xs_dir_create);
+	if (inum) {
+		rval = xfs_dir_ino_validate(tp->t_mountp, inum);
+		if (rval)
+			return rval;
+		XFS_STATS_INC(xs_dir_create);
+	}
 
 	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
 	if (!args)
@@ -276,6 +279,8 @@ xfs_dir_createname(
 	args->whichfork = XFS_DATA_FORK;
 	args->trans = tp;
 	args->op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
+	if (!inum)
+		args->op_flags |= XFS_DA_OP_JUSTCHECK;
 
 	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
 		rval = xfs_dir2_sf_addname(args);
@@ -542,50 +547,7 @@ xfs_dir_canenter(
 	xfs_inode_t	*dp,
 	struct xfs_name	*name)		/* name of entry to add */
 {
-	struct xfs_da_args *args;
-	int		rval;
-	int		v;		/* type-checking value */
-
-	ASSERT(S_ISDIR(dp->i_d.di_mode));
-
-	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
-	if (!args)
-		return -ENOMEM;
-
-	args->geo = dp->i_mount->m_dir_geo;
-	args->name = name->name;
-	args->namelen = name->len;
-	args->filetype = name->type;
-	args->hashval = dp->i_mount->m_dirnameops->hashname(name);
-	args->dp = dp;
-	args->whichfork = XFS_DATA_FORK;
-	args->trans = tp;
-	args->op_flags = XFS_DA_OP_JUSTCHECK | XFS_DA_OP_ADDNAME |
-							XFS_DA_OP_OKNOENT;
-
-	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
-		rval = xfs_dir2_sf_addname(args);
-		goto out_free;
-	}
-
-	rval = xfs_dir2_isblock(args, &v);
-	if (rval)
-		goto out_free;
-	if (v) {
-		rval = xfs_dir2_block_addname(args);
-		goto out_free;
-	}
-
-	rval = xfs_dir2_isleaf(args, &v);
-	if (rval)
-		goto out_free;
-	if (v)
-		rval = xfs_dir2_leaf_addname(args);
-	else
-		rval = xfs_dir2_node_addname(args);
-out_free:
-	kmem_free(args);
-	return rval;
+	return xfs_dir_createname(tp, dp, name, 0, NULL, NULL, 0);
 }
 
 /*


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs: combine xfs_dir_canenter into xfs_dir_createname

[Brian Foster]

On Thu, Aug 21, 2014 at 07:58:43PM -0500, Eric Sandeen wrote:
> xfs_dir_canenter and xfs_dir_createname are
> almost identical.
> 
> Fold the former into the latter, with a helpful
> wrapper for the former.  If createname is called without
> an inode number, it now only checks for space, and does
> not actually add the entry.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index ea84e1c..fdd391f 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -237,7 +237,8 @@ xfs_dir_init(
>  }
>  
>  /*
> -  Enter a name in a directory.
> + * Enter a name in a directory.
> + * If inum is 0, only test for available space.
>   */
>  int
>  xfs_dir_createname(
> @@ -254,10 +255,12 @@ xfs_dir_createname(
>  	int			v;		/* type-checking value */
>  
>  	ASSERT(S_ISDIR(dp->i_d.di_mode));
> -	rval = xfs_dir_ino_validate(tp->t_mountp, inum);
> -	if (rval)
> -		return rval;
> -	XFS_STATS_INC(xs_dir_create);
> +	if (inum) {
> +		rval = xfs_dir_ino_validate(tp->t_mountp, inum);
> +		if (rval)
> +			return rval;
> +		XFS_STATS_INC(xs_dir_create);
> +	}
>  
>  	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
>  	if (!args)
> @@ -276,6 +279,8 @@ xfs_dir_createname(
>  	args->whichfork = XFS_DATA_FORK;
>  	args->trans = tp;
>  	args->op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> +	if (!inum)
> +		args->op_flags |= XFS_DA_OP_JUSTCHECK;
>  
>  	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
>  		rval = xfs_dir2_sf_addname(args);
> @@ -542,50 +547,7 @@ xfs_dir_canenter(
>  	xfs_inode_t	*dp,
>  	struct xfs_name	*name)		/* name of entry to add */
>  {
> -	struct xfs_da_args *args;
> -	int		rval;
> -	int		v;		/* type-checking value */
> -
> -	ASSERT(S_ISDIR(dp->i_d.di_mode));
> -
> -	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> -	if (!args)
> -		return -ENOMEM;
> -
> -	args->geo = dp->i_mount->m_dir_geo;
> -	args->name = name->name;
> -	args->namelen = name->len;
> -	args->filetype = name->type;
> -	args->hashval = dp->i_mount->m_dirnameops->hashname(name);
> -	args->dp = dp;
> -	args->whichfork = XFS_DATA_FORK;
> -	args->trans = tp;
> -	args->op_flags = XFS_DA_OP_JUSTCHECK | XFS_DA_OP_ADDNAME |
> -							XFS_DA_OP_OKNOENT;
> -
> -	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> -		rval = xfs_dir2_sf_addname(args);
> -		goto out_free;
> -	}
> -
> -	rval = xfs_dir2_isblock(args, &v);
> -	if (rval)
> -		goto out_free;
> -	if (v) {
> -		rval = xfs_dir2_block_addname(args);
> -		goto out_free;
> -	}
> -
> -	rval = xfs_dir2_isleaf(args, &v);
> -	if (rval)
> -		goto out_free;
> -	if (v)
> -		rval = xfs_dir2_leaf_addname(args);
> -	else
> -		rval = xfs_dir2_node_addname(args);
> -out_free:
> -	kmem_free(args);
> -	return rval;
> +	return xfs_dir_createname(tp, dp, name, 0, NULL, NULL, 0);
>  }
>  
>  /*
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs: combine xfs_dir_canenter into xfs_dir_createname

[Christoph Hellwig]

On Thu, Aug 21, 2014 at 07:58:43PM -0500, Eric Sandeen wrote:
> xfs_dir_canenter and xfs_dir_createname are
> almost identical.
> 
> Fold the former into the latter, with a helpful
> wrapper for the former.  If createname is called without
> an inode number, it now only checks for space, and does
> not actually add the entry.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

The code changes looks good to me, but..

>  /*
> -  Enter a name in a directory.
> + * Enter a name in a directory.
> + * If inum is 0, only test for available space.
>   */
>  int
>  xfs_dir_createname(

Given how confusing the xfs_dir_createname function name is now this
probably needs a more detailed description mentioning the checking as
first class behavior.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4 V2] xfs: combine xfs_dir_canenter into xfs_dir_createname

[Eric Sandeen]

xfs_dir_canenter and xfs_dir_createname are
almost identical.

Fold the former into the latter, with a helpful
wrapper for the former.  If createname is called without
an inode number, it now only checks for space, and does
not actually add the entry.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: slightly more verbose comment

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index ea84e1c..fdd391f 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -237,7 +237,8 @@ xfs_dir_init(
 }
 
 /*
-  Enter a name in a directory.
+ * Enter a name in a directory, or check for available space.
+ * If inum is 0, only the available space test is performed.
  */
 int
 xfs_dir_createname(
@@ -254,10 +255,12 @@ xfs_dir_createname(
 	int			v;		/* type-checking value */
 
 	ASSERT(S_ISDIR(dp->i_d.di_mode));
-	rval = xfs_dir_ino_validate(tp->t_mountp, inum);
-	if (rval)
-		return rval;
-	XFS_STATS_INC(xs_dir_create);
+	if (inum) {
+		rval = xfs_dir_ino_validate(tp->t_mountp, inum);
+		if (rval)
+			return rval;
+		XFS_STATS_INC(xs_dir_create);
+	}
 
 	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
 	if (!args)
@@ -276,6 +279,8 @@ xfs_dir_createname(
 	args->whichfork = XFS_DATA_FORK;
 	args->trans = tp;
 	args->op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
+	if (!inum)
+		args->op_flags |= XFS_DA_OP_JUSTCHECK;
 
 	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
 		rval = xfs_dir2_sf_addname(args);
@@ -542,50 +547,7 @@ xfs_dir_canenter(
 	xfs_inode_t	*dp,
 	struct xfs_name	*name)		/* name of entry to add */
 {
-	struct xfs_da_args *args;
-	int		rval;
-	int		v;		/* type-checking value */
-
-	ASSERT(S_ISDIR(dp->i_d.di_mode));
-
-	args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
-	if (!args)
-		return -ENOMEM;
-
-	args->geo = dp->i_mount->m_dir_geo;
-	args->name = name->name;
-	args->namelen = name->len;
-	args->filetype = name->type;
-	args->hashval = dp->i_mount->m_dirnameops->hashname(name);
-	args->dp = dp;
-	args->whichfork = XFS_DATA_FORK;
-	args->trans = tp;
-	args->op_flags = XFS_DA_OP_JUSTCHECK | XFS_DA_OP_ADDNAME |
-							XFS_DA_OP_OKNOENT;
-
-	if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
-		rval = xfs_dir2_sf_addname(args);
-		goto out_free;
-	}
-
-	rval = xfs_dir2_isblock(args, &v);
-	if (rval)
-		goto out_free;
-	if (v) {
-		rval = xfs_dir2_block_addname(args);
-		goto out_free;
-	}
-
-	rval = xfs_dir2_isleaf(args, &v);
-	if (rval)
-		goto out_free;
-	if (v)
-		rval = xfs_dir2_leaf_addname(args);
-	else
-		rval = xfs_dir2_node_addname(args);
-out_free:
-	kmem_free(args);
-	return rval;
+	return xfs_dir_createname(tp, dp, name, 0, NULL, NULL, 0);
 }
 
 /*



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/4] xfs: combine xfs_rtmodify_summary and xfs_rtget_summary

[Eric Sandeen]

xfs_rtmodify_summary and xfs_rtget_summary are
almost identical; fold them into
xfs_rtmodify_summary_int(), with wrappers for
each of the original calls.

The _int function modifies if a delta is passed,
and returns a summary pointer if *sum is passed.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index f4dd697..50e3b93 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -424,20 +424,24 @@ xfs_rtfind_forw(
 }
 
 /*
- * Read and modify the summary information for a given extent size,
+ * Read and/or modify the summary information for a given extent size,
  * bitmap block combination.
  * Keeps track of a current summary block, so we don't keep reading
  * it from the buffer cache.
+ *
+ * Summary information is returned in *sum if specified.
+ * If no delta is specified, returns summary only.
  */
 int
-xfs_rtmodify_summary(
-	xfs_mount_t	*mp,		/* file system mount point */
+xfs_rtmodify_summary_int(
+	xfs_mount_t	*mp,		/* file system mount structure */
 	xfs_trans_t	*tp,		/* transaction pointer */
 	int		log,		/* log2 of extent size */
 	xfs_rtblock_t	bbno,		/* bitmap block number */
 	int		delta,		/* change to make to summary info */
 	xfs_buf_t	**rbpp,		/* in/out: summary block buffer */
-	xfs_fsblock_t	*rsb)		/* in/out: summary block number */
+	xfs_fsblock_t	*rsb,		/* in/out: summary block number */
+	xfs_suminfo_t	*sum)		/* out: summary info for this block */
 {
 	xfs_buf_t	*bp;		/* buffer for the summary block */
 	int		error;		/* error value */
@@ -480,15 +484,40 @@ xfs_rtmodify_summary(
 		}
 	}
 	/*
-	 * Point to the summary information, modify and log it.
+	 * Point to the summary information, modify/log it, and/or copy it out.
 	 */
 	sp = XFS_SUMPTR(mp, bp, so);
-	*sp += delta;
-	xfs_trans_log_buf(tp, bp, (uint)((char *)sp - (char *)bp->b_addr),
-		(uint)((char *)sp - (char *)bp->b_addr + sizeof(*sp) - 1));
+	if (delta) {
+		uint first = (uint)((char *)sp - (char *)bp->b_addr);
+
+		*sp += delta;
+		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
+	}
+	if (sum) {
+		/*
+		 * Drop the buffer if we're not asked to remember it.
+		 */
+		if (!rbpp)
+			xfs_trans_brelse(tp, bp);
+		*sum = *sp;
+	}
 	return 0;
 }
 
+int
+xfs_rtmodify_summary(
+	xfs_mount_t	*mp,		/* file system mount structure */
+	xfs_trans_t	*tp,		/* transaction pointer */
+	int		log,		/* log2 of extent size */
+	xfs_rtblock_t	bbno,		/* bitmap block number */
+	int		delta,		/* change to make to summary info */
+	xfs_buf_t	**rbpp,		/* in/out: summary block buffer */
+	xfs_fsblock_t	*rsb)		/* in/out: summary block number */
+{
+	return xfs_rtmodify_summary_int(mp, tp, log, bbno,
+					delta, rbpp, rsb, NULL);
+}
+
 /*
  * Set the given range of bitmap bits to the given value.
  * Do whatever I/O and logging is required.
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 909e143..d1160cc 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -46,7 +46,7 @@
  * Keeps track of a current summary block, so we don't keep reading
  * it from the buffer cache.
  */
-STATIC int				/* error */
+int
 xfs_rtget_summary(
 	xfs_mount_t	*mp,		/* file system mount structure */
 	xfs_trans_t	*tp,		/* transaction pointer */
@@ -56,60 +56,9 @@ xfs_rtget_summary(
 	xfs_fsblock_t	*rsb,		/* in/out: summary block number */
 	xfs_suminfo_t	*sum)		/* out: summary info for this block */
 {
-	xfs_buf_t	*bp;		/* buffer for summary block */
-	int		error;		/* error value */
-	xfs_fsblock_t	sb;		/* summary fsblock */
-	int		so;		/* index into the summary file */
-	xfs_suminfo_t	*sp;		/* pointer to returned data */
-
-	/*
-	 * Compute entry number in the summary file.
-	 */
-	so = XFS_SUMOFFS(mp, log, bbno);
-	/*
-	 * Compute the block number in the summary file.
-	 */
-	sb = XFS_SUMOFFSTOBLOCK(mp, so);
-	/*
-	 * If we have an old buffer, and the block number matches, use that.
-	 */
-	if (rbpp && *rbpp && *rsb == sb)
-		bp = *rbpp;
-	/*
-	 * Otherwise we have to get the buffer.
-	 */
-	else {
-		/*
-		 * If there was an old one, get rid of it first.
-		 */
-		if (rbpp && *rbpp)
-			xfs_trans_brelse(tp, *rbpp);
-		error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
-		if (error) {
-			return error;
-		}
-		/*
-		 * Remember this buffer and block for the next call.
-		 */
-		if (rbpp) {
-			*rbpp = bp;
-			*rsb = sb;
-		}
-	}
-	/*
-	 * Point to the summary information & copy it out.
-	 */
-	sp = XFS_SUMPTR(mp, bp, so);
-	*sum = *sp;
-	/*
-	 * Drop the buffer if we're not asked to remember it.
-	 */
-	if (!rbpp)
-		xfs_trans_brelse(tp, bp);
-	return 0;
+	return xfs_rtmodify_summary_int(mp, tp, log, bbno, 0, rbpp, rsb, sum);
 }
 
-
 /*
  * Return whether there are any free extents in the size range given
  * by low and high, for the bitmap block bbno.
diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
index c642795..76c0a4a 100644
--- a/fs/xfs/xfs_rtalloc.h
+++ b/fs/xfs/xfs_rtalloc.h
@@ -111,6 +111,10 @@ int xfs_rtfind_forw(struct xfs_mount *mp, struct xfs_trans *tp,
 		    xfs_rtblock_t *rtblock);
 int xfs_rtmodify_range(struct xfs_mount *mp, struct xfs_trans *tp,
 		       xfs_rtblock_t start, xfs_extlen_t len, int val);
+int xfs_rtmodify_summary_int(struct xfs_mount *mp, struct xfs_trans *tp,
+			     int log, xfs_rtblock_t bbno, int delta,
+			     xfs_buf_t **rbpp, xfs_fsblock_t *rsb,
+			     xfs_suminfo_t *sum);
 int xfs_rtmodify_summary(struct xfs_mount *mp, struct xfs_trans *tp, int log,
 			 xfs_rtblock_t bbno, int delta, xfs_buf_t **rbpp,
 			 xfs_fsblock_t *rsb);


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs: combine xfs_rtmodify_summary and xfs_rtget_summary

[Brian Foster]

On Thu, Aug 21, 2014 at 08:00:45PM -0500, Eric Sandeen wrote:
> xfs_rtmodify_summary and xfs_rtget_summary are
> almost identical; fold them into
> xfs_rtmodify_summary_int(), with wrappers for
> each of the original calls.
> 
> The _int function modifies if a delta is passed,
> and returns a summary pointer if *sum is passed.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index f4dd697..50e3b93 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -424,20 +424,24 @@ xfs_rtfind_forw(
>  }
>  
>  /*
> - * Read and modify the summary information for a given extent size,
> + * Read and/or modify the summary information for a given extent size,
>   * bitmap block combination.
>   * Keeps track of a current summary block, so we don't keep reading
>   * it from the buffer cache.
> + *
> + * Summary information is returned in *sum if specified.
> + * If no delta is specified, returns summary only.
>   */
>  int
> -xfs_rtmodify_summary(
> -	xfs_mount_t	*mp,		/* file system mount point */
> +xfs_rtmodify_summary_int(
> +	xfs_mount_t	*mp,		/* file system mount structure */
>  	xfs_trans_t	*tp,		/* transaction pointer */
>  	int		log,		/* log2 of extent size */
>  	xfs_rtblock_t	bbno,		/* bitmap block number */
>  	int		delta,		/* change to make to summary info */
>  	xfs_buf_t	**rbpp,		/* in/out: summary block buffer */
> -	xfs_fsblock_t	*rsb)		/* in/out: summary block number */
> +	xfs_fsblock_t	*rsb,		/* in/out: summary block number */
> +	xfs_suminfo_t	*sum)		/* out: summary info for this block */
>  {
>  	xfs_buf_t	*bp;		/* buffer for the summary block */
>  	int		error;		/* error value */
> @@ -480,15 +484,40 @@ xfs_rtmodify_summary(
>  		}
>  	}
>  	/*
> -	 * Point to the summary information, modify and log it.
> +	 * Point to the summary information, modify/log it, and/or copy it out.
>  	 */
>  	sp = XFS_SUMPTR(mp, bp, so);
> -	*sp += delta;
> -	xfs_trans_log_buf(tp, bp, (uint)((char *)sp - (char *)bp->b_addr),
> -		(uint)((char *)sp - (char *)bp->b_addr + sizeof(*sp) - 1));
> +	if (delta) {
> +		uint first = (uint)((char *)sp - (char *)bp->b_addr);
> +
> +		*sp += delta;
> +		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
> +	}
> +	if (sum) {
> +		/*
> +		 * Drop the buffer if we're not asked to remember it.
> +		 */
> +		if (!rbpp)
> +			xfs_trans_brelse(tp, bp);

This introduces some potentially weird circumstances (e.g., acquire,
log, release of a buffer), but I think it's resolved by the next patch.

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +		*sum = *sp;
> +	}
>  	return 0;
>  }
>  
> +int
> +xfs_rtmodify_summary(
> +	xfs_mount_t	*mp,		/* file system mount structure */
> +	xfs_trans_t	*tp,		/* transaction pointer */
> +	int		log,		/* log2 of extent size */
> +	xfs_rtblock_t	bbno,		/* bitmap block number */
> +	int		delta,		/* change to make to summary info */
> +	xfs_buf_t	**rbpp,		/* in/out: summary block buffer */
> +	xfs_fsblock_t	*rsb)		/* in/out: summary block number */
> +{
> +	return xfs_rtmodify_summary_int(mp, tp, log, bbno,
> +					delta, rbpp, rsb, NULL);
> +}
> +
>  /*
>   * Set the given range of bitmap bits to the given value.
>   * Do whatever I/O and logging is required.
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 909e143..d1160cc 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -46,7 +46,7 @@
>   * Keeps track of a current summary block, so we don't keep reading
>   * it from the buffer cache.
>   */
> -STATIC int				/* error */
> +int
>  xfs_rtget_summary(
>  	xfs_mount_t	*mp,		/* file system mount structure */
>  	xfs_trans_t	*tp,		/* transaction pointer */
> @@ -56,60 +56,9 @@ xfs_rtget_summary(
>  	xfs_fsblock_t	*rsb,		/* in/out: summary block number */
>  	xfs_suminfo_t	*sum)		/* out: summary info for this block */
>  {
> -	xfs_buf_t	*bp;		/* buffer for summary block */
> -	int		error;		/* error value */
> -	xfs_fsblock_t	sb;		/* summary fsblock */
> -	int		so;		/* index into the summary file */
> -	xfs_suminfo_t	*sp;		/* pointer to returned data */
> -
> -	/*
> -	 * Compute entry number in the summary file.
> -	 */
> -	so = XFS_SUMOFFS(mp, log, bbno);
> -	/*
> -	 * Compute the block number in the summary file.
> -	 */
> -	sb = XFS_SUMOFFSTOBLOCK(mp, so);
> -	/*
> -	 * If we have an old buffer, and the block number matches, use that.
> -	 */
> -	if (rbpp && *rbpp && *rsb == sb)
> -		bp = *rbpp;
> -	/*
> -	 * Otherwise we have to get the buffer.
> -	 */
> -	else {
> -		/*
> -		 * If there was an old one, get rid of it first.
> -		 */
> -		if (rbpp && *rbpp)
> -			xfs_trans_brelse(tp, *rbpp);
> -		error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
> -		if (error) {
> -			return error;
> -		}
> -		/*
> -		 * Remember this buffer and block for the next call.
> -		 */
> -		if (rbpp) {
> -			*rbpp = bp;
> -			*rsb = sb;
> -		}
> -	}
> -	/*
> -	 * Point to the summary information & copy it out.
> -	 */
> -	sp = XFS_SUMPTR(mp, bp, so);
> -	*sum = *sp;
> -	/*
> -	 * Drop the buffer if we're not asked to remember it.
> -	 */
> -	if (!rbpp)
> -		xfs_trans_brelse(tp, bp);
> -	return 0;
> +	return xfs_rtmodify_summary_int(mp, tp, log, bbno, 0, rbpp, rsb, sum);
>  }
>  
> -
>  /*
>   * Return whether there are any free extents in the size range given
>   * by low and high, for the bitmap block bbno.
> diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> index c642795..76c0a4a 100644
> --- a/fs/xfs/xfs_rtalloc.h
> +++ b/fs/xfs/xfs_rtalloc.h
> @@ -111,6 +111,10 @@ int xfs_rtfind_forw(struct xfs_mount *mp, struct xfs_trans *tp,
>  		    xfs_rtblock_t *rtblock);
>  int xfs_rtmodify_range(struct xfs_mount *mp, struct xfs_trans *tp,
>  		       xfs_rtblock_t start, xfs_extlen_t len, int val);
> +int xfs_rtmodify_summary_int(struct xfs_mount *mp, struct xfs_trans *tp,
> +			     int log, xfs_rtblock_t bbno, int delta,
> +			     xfs_buf_t **rbpp, xfs_fsblock_t *rsb,
> +			     xfs_suminfo_t *sum);
>  int xfs_rtmodify_summary(struct xfs_mount *mp, struct xfs_trans *tp, int log,
>  			 xfs_rtblock_t bbno, int delta, xfs_buf_t **rbpp,
>  			 xfs_fsblock_t *rsb);
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs: combine xfs_rtmodify_summary and xfs_rtget_summary

[Eric Sandeen]

On 8/22/14, 8:19 AM, Brian Foster wrote:
> On Thu, Aug 21, 2014 at 08:00:45PM -0500, Eric Sandeen wrote:

...

>> @@ -480,15 +484,40 @@ xfs_rtmodify_summary(
>>  		}
>>  	}
>>  	/*
>> -	 * Point to the summary information, modify and log it.
>> +	 * Point to the summary information, modify/log it, and/or copy it out.
>>  	 */
>>  	sp = XFS_SUMPTR(mp, bp, so);
>> -	*sp += delta;
>> -	xfs_trans_log_buf(tp, bp, (uint)((char *)sp - (char *)bp->b_addr),
>> -		(uint)((char *)sp - (char *)bp->b_addr + sizeof(*sp) - 1));
>> +	if (delta) {
>> +		uint first = (uint)((char *)sp - (char *)bp->b_addr);
>> +
>> +		*sp += delta;
>> +		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
>> +	}
>> +	if (sum) {
>> +		/*
>> +		 * Drop the buffer if we're not asked to remember it.
>> +		 */
>> +		if (!rbpp)
>> +			xfs_trans_brelse(tp, bp);
> 
> This introduces some potentially weird circumstances (e.g., acquire,
> log, release of a buffer), but I think it's resolved by the next patch.
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

does it introduce it, or just highlight it?  I thought it was weird too,
but I think it existed before; that's what prompted me to go looking at
callers and drop the rbpp checks, FWIW.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs: combine xfs_rtmodify_summary and xfs_rtget_summary

[Eric Sandeen]

On 8/22/14, 10:01 AM, Eric Sandeen wrote:
> On 8/22/14, 8:19 AM, Brian Foster wrote:
>> On Thu, Aug 21, 2014 at 08:00:45PM -0500, Eric Sandeen wrote:
> 
> ...
> 
>>> @@ -480,15 +484,40 @@ xfs_rtmodify_summary(
>>>  		}
>>>  	}
>>>  	/*
>>> -	 * Point to the summary information, modify and log it.
>>> +	 * Point to the summary information, modify/log it, and/or copy it out.
>>>  	 */
>>>  	sp = XFS_SUMPTR(mp, bp, so);
>>> -	*sp += delta;
>>> -	xfs_trans_log_buf(tp, bp, (uint)((char *)sp - (char *)bp->b_addr),
>>> -		(uint)((char *)sp - (char *)bp->b_addr + sizeof(*sp) - 1));
>>> +	if (delta) {
>>> +		uint first = (uint)((char *)sp - (char *)bp->b_addr);
>>> +
>>> +		*sp += delta;
>>> +		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
>>> +	}
>>> +	if (sum) {
>>> +		/*
>>> +		 * Drop the buffer if we're not asked to remember it.
>>> +		 */
>>> +		if (!rbpp)
>>> +			xfs_trans_brelse(tp, bp);
>>
>> This introduces some potentially weird circumstances (e.g., acquire,
>> log, release of a buffer), but I think it's resolved by the next patch.
>>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> does it introduce it, or just highlight it?  I thought it was weird too,
> but I think it existed before; that's what prompted me to go looking at
> callers and drop the rbpp checks, FWIW.

Oh, right, if delta & sum,there's a problem if (!rpbb).  And I did introduce
that as a new issue.  But the code at that point never sees (!rbpp) so I think
it's safe.  I could respin patches 3 & 4, to do them in the opposite order,
if desired.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/4] xfs: remove rbpp check from xfs_rtmodify_summary_int

[Eric Sandeen]

rbpp is always passed into xfs_rtmodify_summary
and xfs_rtget_summary, so there is no need to
test for it in xfs_rtmodify_summary_int.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 50e3b93..7c818f1 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -460,7 +460,7 @@ xfs_rtmodify_summary_int(
 	/*
 	 * If we have an old buffer, and the block number matches, use that.
 	 */
-	if (rbpp && *rbpp && *rsb == sb)
+	if (*rbpp && *rsb == sb)
 		bp = *rbpp;
 	/*
 	 * Otherwise we have to get the buffer.
@@ -469,7 +469,7 @@ xfs_rtmodify_summary_int(
 		/*
 		 * If there was an old one, get rid of it first.
 		 */
-		if (rbpp && *rbpp)
+		if (*rbpp)
 			xfs_trans_brelse(tp, *rbpp);
 		error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
 		if (error) {
@@ -478,10 +478,8 @@ xfs_rtmodify_summary_int(
 		/*
 		 * Remember this buffer and block for the next call.
 		 */
-		if (rbpp) {
-			*rbpp = bp;
-			*rsb = sb;
-		}
+		*rbpp = bp;
+		*rsb = sb;
 	}
 	/*
 	 * Point to the summary information, modify/log it, and/or copy it out.
@@ -493,14 +491,8 @@ xfs_rtmodify_summary_int(
 		*sp += delta;
 		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
 	}
-	if (sum) {
-		/*
-		 * Drop the buffer if we're not asked to remember it.
-		 */
-		if (!rbpp)
-			xfs_trans_brelse(tp, bp);
+	if (sum)
 		*sum = *sp;
-	}
 	return 0;
 }
 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] xfs: remove rbpp check from xfs_rtmodify_summary_int

[Brian Foster]

On Thu, Aug 21, 2014 at 08:03:19PM -0500, Eric Sandeen wrote:
> rbpp is always passed into xfs_rtmodify_summary
> and xfs_rtget_summary, so there is no need to
> test for it in xfs_rtmodify_summary_int.
> 

Looks fine, but this is also called through a variety of twisty paths.
Could we add a top-level error check or assert?

Brian

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 50e3b93..7c818f1 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -460,7 +460,7 @@ xfs_rtmodify_summary_int(
>  	/*
>  	 * If we have an old buffer, and the block number matches, use that.
>  	 */
> -	if (rbpp && *rbpp && *rsb == sb)
> +	if (*rbpp && *rsb == sb)
>  		bp = *rbpp;
>  	/*
>  	 * Otherwise we have to get the buffer.
> @@ -469,7 +469,7 @@ xfs_rtmodify_summary_int(
>  		/*
>  		 * If there was an old one, get rid of it first.
>  		 */
> -		if (rbpp && *rbpp)
> +		if (*rbpp)
>  			xfs_trans_brelse(tp, *rbpp);
>  		error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
>  		if (error) {
> @@ -478,10 +478,8 @@ xfs_rtmodify_summary_int(
>  		/*
>  		 * Remember this buffer and block for the next call.
>  		 */
> -		if (rbpp) {
> -			*rbpp = bp;
> -			*rsb = sb;
> -		}
> +		*rbpp = bp;
> +		*rsb = sb;
>  	}
>  	/*
>  	 * Point to the summary information, modify/log it, and/or copy it out.
> @@ -493,14 +491,8 @@ xfs_rtmodify_summary_int(
>  		*sp += delta;
>  		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
>  	}
> -	if (sum) {
> -		/*
> -		 * Drop the buffer if we're not asked to remember it.
> -		 */
> -		if (!rbpp)
> -			xfs_trans_brelse(tp, bp);
> +	if (sum)
>  		*sum = *sp;
> -	}
>  	return 0;
>  }
>  
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] xfs: remove rbpp check from xfs_rtmodify_summary_int

[Eric Sandeen]

On 8/22/14, 8:20 AM, Brian Foster wrote:
> On Thu, Aug 21, 2014 at 08:03:19PM -0500, Eric Sandeen wrote:
>> rbpp is always passed into xfs_rtmodify_summary
>> and xfs_rtget_summary, so there is no need to
>> test for it in xfs_rtmodify_summary_int.
>>
> 
> Looks fine, but this is also called through a variety of twisty paths.
> Could we add a top-level error check or assert?

We could, though it'll oops pretty fast if we don't send rbpp anyway.
An ASSERT might be decent for documentation, though...

-Eric

> Brian
> 
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
>> index 50e3b93..7c818f1 100644
>> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
>> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
>> @@ -460,7 +460,7 @@ xfs_rtmodify_summary_int(
>>  	/*
>>  	 * If we have an old buffer, and the block number matches, use that.
>>  	 */
>> -	if (rbpp && *rbpp && *rsb == sb)
>> +	if (*rbpp && *rsb == sb)
>>  		bp = *rbpp;
>>  	/*
>>  	 * Otherwise we have to get the buffer.
>> @@ -469,7 +469,7 @@ xfs_rtmodify_summary_int(
>>  		/*
>>  		 * If there was an old one, get rid of it first.
>>  		 */
>> -		if (rbpp && *rbpp)
>> +		if (*rbpp)
>>  			xfs_trans_brelse(tp, *rbpp);
>>  		error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
>>  		if (error) {
>> @@ -478,10 +478,8 @@ xfs_rtmodify_summary_int(
>>  		/*
>>  		 * Remember this buffer and block for the next call.
>>  		 */
>> -		if (rbpp) {
>> -			*rbpp = bp;
>> -			*rsb = sb;
>> -		}
>> +		*rbpp = bp;
>> +		*rsb = sb;
>>  	}
>>  	/*
>>  	 * Point to the summary information, modify/log it, and/or copy it out.
>> @@ -493,14 +491,8 @@ xfs_rtmodify_summary_int(
>>  		*sp += delta;
>>  		xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
>>  	}
>> -	if (sum) {
>> -		/*
>> -		 * Drop the buffer if we're not asked to remember it.
>> -		 */
>> -		if (!rbpp)
>> -			xfs_trans_brelse(tp, bp);
>> +	if (sum)
>>  		*sum = *sp;
>> -	}
>>  	return 0;
>>  }
>>  
>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs