[PATCH 1/4 linux-next] xfs: use swap() in xfs_dir2_leafn_rebalance()

[PATCH 1/4 linux-next] xfs: use swap() in xfs_dir2_leafn_rebalance()

[Fabian Frederick]

Use kernel.h macro definition.
Also remove assignment in if condition (checkpatch error)

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/xfs/libxfs/xfs_dir2_node.c | 8 ++------

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/xfs/libxfs/xfs_dir2_node.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index 41b80d3..84442a6 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -967,13 +967,10 @@ xfs_dir2_leafn_rebalance(
 	/*
 	 * If the block order is wrong, swap the arguments.
 	 */
-	if ((swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp))) {
-		xfs_da_state_blk_t	*tmp;	/* temp for block swap */
+	swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp);
+	if (swap)
+		swap(blk1, blk2);
 
-		tmp = blk1;
-		blk1 = blk2;
-		blk2 = tmp;
-	}
 	leaf1 = blk1->bp->b_addr;
 	leaf2 = blk2->bp->b_addr;
 	dp->d_ops->leaf_hdr_from_disk(&hdr1, leaf1);
-- 
2.4.0

[PATCH 2/4 linux-next] xfs: use swap() in xfs_lock_two_inodes()

[Fabian Frederick]

Use kernel.h macro definition.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/xfs/xfs_inode.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index d6ebc85..d4f3a09 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -500,7 +500,6 @@ xfs_lock_two_inodes(
 	xfs_inode_t		*ip1,
 	uint			lock_mode)
 {
-	xfs_inode_t		*temp;
 	int			attempts = 0;
 	xfs_log_item_t		*lp;
 
@@ -512,11 +511,8 @@ xfs_lock_two_inodes(
 
 	ASSERT(ip0->i_ino != ip1->i_ino);
 
-	if (ip0->i_ino > ip1->i_ino) {
-		temp = ip0;
-		ip0 = ip1;
-		ip1 = temp;
-	}
+	if (ip0->i_ino > ip1->i_ino)
+		swap(ip0, ip1);
 
  again:
 	xfs_ilock(ip0, xfs_lock_inumorder(lock_mode, 0));
-- 
2.4.0

[PATCH 3/4 linux-next] xfs: use swap() in xfs_attr3_leaf_rebalance()

[Fabian Frederick]

Use kernel.h macro definition.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 04e79d5..a346455 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1481,17 +1481,10 @@ xfs_attr3_leaf_rebalance(
 	 */
 	swap = 0;
 	if (xfs_attr3_leaf_order(blk1->bp, &ichdr1, blk2->bp, &ichdr2)) {
-		struct xfs_da_state_blk	*tmp_blk;
-		struct xfs_attr3_icleaf_hdr tmp_ichdr;
-
-		tmp_blk = blk1;
-		blk1 = blk2;
-		blk2 = tmp_blk;
+		swap(blk1, blk2);
 
 		/* struct copies to swap them rather than reconverting */
-		tmp_ichdr = ichdr1;
-		ichdr1 = ichdr2;
-		ichdr2 = tmp_ichdr;
+		swap(ichdr1, ichdr2);
 
 		leaf1 = blk1->bp->b_addr;
 		leaf2 = blk2->bp->b_addr;
-- 
2.4.0

[PATCH 4/4 linux-next] xfs: use swap() in xfs_da3_node_rebalance()

[Fabian Frederick]

Use kernel.h macro definition.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/xfs/libxfs/xfs_da_btree.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 2385f8c..f0101a8 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -734,7 +734,6 @@ xfs_da3_node_rebalance(
 {
 	struct xfs_da_intnode	*node1;
 	struct xfs_da_intnode	*node2;
-	struct xfs_da_intnode	*tmpnode;
 	struct xfs_da_node_entry *btree1;
 	struct xfs_da_node_entry *btree2;
 	struct xfs_da_node_entry *btree_s;
@@ -764,9 +763,7 @@ xfs_da3_node_rebalance(
 	    ((be32_to_cpu(btree2[0].hashval) < be32_to_cpu(btree1[0].hashval)) ||
 	     (be32_to_cpu(btree2[nodehdr2.count - 1].hashval) <
 			be32_to_cpu(btree1[nodehdr1.count - 1].hashval)))) {
-		tmpnode = node1;
-		node1 = node2;
-		node2 = tmpnode;
+		swap(node1, node2);
 		dp->d_ops->node_hdr_from_disk(&nodehdr1, node1);
 		dp->d_ops->node_hdr_from_disk(&nodehdr2, node2);
 		btree1 = dp->d_ops->node_tree_p(node1);
-- 
2.4.0

Re: [PATCH 1/4 linux-next] xfs: use swap() in xfs_dir2_leafn_rebalance()

[Al Viro]

On Mon, May 18, 2015 at 07:13:48PM +0200, Fabian Frederick wrote:
>  	 * If the block order is wrong, swap the arguments.
>  	 */
> -	if ((swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp))) {
> -		xfs_da_state_blk_t	*tmp;	/* temp for block swap */
> +	swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp);
> +	if (swap)
> +		swap(blk1, blk2);

Egads...  Have you even read what you'd written?  Yes, sure, preprocessor
will do the right thing, but it's a very noticable annoyance for somebody
reading that.  Rename the bleeding flag, please.

Re: [PATCH 1/4 linux-next] xfs: use swap() in xfs_dir2_leafn_rebalance()

[Fabian Frederick]

> On 18 May 2015 at 19:19 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>
> On Mon, May 18, 2015 at 07:13:48PM +0200, Fabian Frederick wrote:
> >      * If the block order is wrong, swap the arguments.
> >      */
> > -   if ((swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp))) {
> > -           xfs_da_state_blk_t      *tmp;   /* temp for block swap */
> > +   swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp);
> > +   if (swap)
> > +           swap(blk1, blk2);
>
> Egads...  Have you even read what you'd written?  Yes, sure, preprocessor
> will do the right thing, but it's a very noticable annoyance for somebody
> reading that.  Rename the bleeding flag, please.

I wanted to focus on the swap() update in this small patchset (some other things
should be done in there like have xfs_dir2_leafn_order() return bool) but I can
rename it in something like need_swap. Do I need to resend the 4 patches Dave ?

Regards,
Fabian

Re: [PATCH 1/4 linux-next] xfs: use swap() in xfs_dir2_leafn_rebalance()

[Dave Chinner]

On Mon, May 18, 2015 at 08:06:13PM +0200, Fabian Frederick wrote:
> 
> 
> > On 18 May 2015 at 19:19 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >
> >
> > On Mon, May 18, 2015 at 07:13:48PM +0200, Fabian Frederick wrote:
> > >      * If the block order is wrong, swap the arguments.
> > >      */
> > > -   if ((swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp))) {
> > > -           xfs_da_state_blk_t      *tmp;   /* temp for block swap */
> > > +   swap = xfs_dir2_leafn_order(dp, blk1->bp, blk2->bp);
> > > +   if (swap)
> > > +           swap(blk1, blk2);
> >
> > Egads...  Have you even read what you'd written?  Yes, sure, preprocessor
> > will do the right thing, but it's a very noticable annoyance for somebody
> > reading that.  Rename the bleeding flag, please.
> 
> I wanted to focus on the swap() update in this small patchset (some other things
> should be done in there like have xfs_dir2_leafn_order() return bool) but I can
> rename it in something like need_swap. Do I need to resend the 4 patches Dave ?

4 patches is 3 patches too many for noise like this.  Anyway, two of
the patches have the same local "swap" variable problem; the context
is "swap order" not "need swap".

FWIW, I am not a fan of changing the code for no actual gain - the
compiled code is identical, there are no stack savings, and now I
have to look at an extra file to work out what the code does. If
you're changing the code and this is prep work for a large series,
then by all means clean the code up. But otherwise, changes like
this just mean work that other developers have in progress need
rebasing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com