Skip to contentSkip to conversation

GFS2: Simplify iterative add loop in foreach_descriptor

PATCH New Bob Peterson 4 participants 8 messages Aug 8, 2018 – Aug 10, 2018 1 message not in the archive GFS2 file system

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bob Peterson

author
#1 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>
---
 fs/gfs2/recovery.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +3 -2
index 0f501f938d1c..6c6b19263b82 100644
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd, unsigned int start,
 			return error;
 		}
 
-		while (length--)
-			gfs2_replay_incr_blk(jd, &start);
+		start += length;
+		if (start >= jd->jd_blocks)
+			start -= jd->jd_blocks;
 
 		brelse(bh);
 	}
maintainer
#2 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

Quoted 8 lines · Bob Peterson

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>
Reviewed-by Andreas Gruenbacher <[email protected]>
Quoted 21 lines
---
 fs/gfs2/recovery.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +3 -2
index 0f501f938d1c..6c6b19263b82 100644
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd, unsigned int start,
                        return error;
                }
 
-               while (length--)
-                       gfs2_replay_incr_blk(jd, &start);
+               start += length;
+               if (start >= jd->jd_blocks)
+                       start -= jd->jd_blocks;
 
                brelse(bh);
        }
#3 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

In reply to Bob Peterson

----- Original Message -----

Quoted 5 lines

From: "Bob Peterson" <[email protected]>
To: "cluster-devel" <[email protected]>
Sent: Wednesday, August 8, 2018 1:52:03 PM
Subject: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

Quoted 8 lines

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>

Hi,

Looks good. ACK.

Cheers!
--Abhi

Reviewed-by Abhi Das <[email protected]>
Quoted 23 lines
---
 fs/gfs2/recovery.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +3 -2
index 0f501f938d1c..6c6b19263b82 100644
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd,
 unsigned int start,
 			return error;
 		}
 
-		while (length--)
-			gfs2_replay_incr_blk(jd, &start);
+		start += length;
+		if (start >= jd->jd_blocks)
+			start -= jd->jd_blocks;
 
 		brelse(bh);

}

#4 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

In reply to Bob Peterson

Hi,

Quoted 19 lines · Bob Peterson

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>
---
  fs/gfs2/recovery.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +0 -0
index 0f501f938d1c..6c6b19263b82 100644
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd, unsigned int start,
  			return error;

fs/gfs2/recovery.c static int foreach_descriptor(struct gfs2_jdesc *jd, unsigned int start, · line 362

Quoted 10 lines
fs/gfs2/recovery.c +3 -2
  		}
 
-		while (length--)
-			gfs2_replay_incr_blk(jd, &start);
+		start += length;
+		if (start >= jd->jd_blocks)
+			start -= jd->jd_blocks;
 
  		brelse(bh);
  	}

Now you've hidden the increment of the replay block. Please don't open code this, but just add an argument to gfs2_replay_incr_blk() such that you can tell it how many blocks to increment, rather than just assuming a single block as it does at the moment. Otherwise this can easily get missed when someone looks at the code in future, and expects gfs2_replay_incr_blk to be the only thing that changes the position during recovery,

Steve.

author
#5 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

----- Original Message -----

Quoted 9 lines

Now you've hidden the increment of the replay block. Please don't open code this, but just add an argument to gfs2_replay_incr_blk() such that you can tell it how many blocks to increment, rather than just assuming a single block as it does at the moment. Otherwise this can easily get missed when someone looks at the code in future, and expects gfs2_replay_incr_blk to be the only thing that changes the position during recovery,

Steve.

Hi,

Since we're so close to the merge window, I'll just remove it from the tree and do this afterward.

Bob Peterson

maintainer
#6 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

In reply to Steven Whitehouse
Quoted 33 lines · Steven Whitehouse

Hi,

Quoted 28 lines · Bob Peterson

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>
---
  fs/gfs2/recovery.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +3 -1
index 0f501f938d1c..6c6b19263b82 100644
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd,
 unsigned int start,
                        return error;
                }
  -             while (length--)
-                       gfs2_replay_incr_blk(jd, &start);
+               start += length;
+               if (start >= jd->jd_blocks)
+                       start -= jd->jd_blocks;
                brelse(bh);
        }
Quoted 8 lines
Quoted 1 line

Now you've hidden the increment of the replay block. Please don't open code this, but just add an argument to gfs2_replay_incr_blk() such that you can tell it how many blocks to increment, rather than just assuming a single block as it does at the moment. Otherwise this can easily get missed when someone looks at the code in future, and expects gfs2_replay_incr_blk to be the only thing that changes the position during recovery,

If we really want to encapsulate "add modulo jd->jd_blocks", it's also open-coded in find_good_lh and jhead_scan.

Andreas

#7 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

Hi,

Quoted 40 lines · Andreas Gruenbacher
Quoted 31 lines · Steven Whitehouse

Hi,

Quoted 26 lines · Bob Peterson

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>
---
   fs/gfs2/recovery.c | 5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +3 -1
index 0f501f938d1c..6c6b19263b82 100644
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd,
 unsigned int start,
                         return error;
                 }
   -             while (length--)
-                       gfs2_replay_incr_blk(jd, &start);
+               start += length;
+               if (start >= jd->jd_blocks)
+                       start -= jd->jd_blocks;
                 brelse(bh);
Quoted 8 lines

fs/gfs2/recovery.c static int foreach_descriptor(struct gfs2_jdesc *jd, · line 362

Quoted 2 lines
fs/gfs2/recovery.c +0 -0
         }

Now you've hidden the increment of the replay block. Please don't open code this, but just add an argument to gfs2_replay_incr_blk() such that you can tell it how many blocks to increment, rather than just assuming a single block as it does at the moment. Otherwise this can easily get missed when someone looks at the code in future, and expects gfs2_replay_incr_blk to be the only thing that changes the position during recovery,

Quoted 4 lines

If we really want to encapsulate "add modulo jd->jd_blocks", it's also open-coded in find_good_lh and jhead_scan.

Andreas

I wonder if those will go away with Abhi's patch set in due course?

Steve.

#8 lore raw

PATCH 1/1 GFS2: Simplify iterative add loop in foreach_descriptor

----- Original Message -----

Quoted 53 lines

From: "Steven Whitehouse" <[email protected]>
To: "Andreas Gruenbacher" <[email protected]>
Cc: "Bob Peterson" <[email protected]>, "cluster-devel" <[email protected]>, "Abhijith Das" <[email protected]>
Sent: Friday, August 10, 2018 7:21:28 AM
Subject: Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

Hi,

Quoted 42 lines · Andreas Gruenbacher
Quoted 41 lines · Steven Whitehouse

Hi,

Quoted 18 lines · Bob Peterson

Hi,

Before this patch, function foreach_descriptor repeatedly called function gfs2_replay_incr_blk which just incremented the value while decrementing another, and checked for wrap. This is a waste of time. This patch just adds the value and adjusts it if a wrap occurred.

Signed-off-by Bob Peterson <[email protected]>
---
   fs/gfs2/recovery.c | 5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)
fs/gfs2/recovery.c +0 -0
index 0f501f938d1c..6c6b19263b82 100644

fs/gfs2/recovery.c static int foreach_descriptor(struct gfs2_jdesc *jd, · line 362

Quoted 10 lines
fs/gfs2/recovery.c +3 -1
@@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd,
 unsigned int start,
                         return error;
                 }
   -             while (length--)
-                       gfs2_replay_incr_blk(jd, &start);
+               start += length;
+               if (start >= jd->jd_blocks)
+                       start -= jd->jd_blocks;
                 brelse(bh);
         }

Now you've hidden the increment of the replay block. Please don't open code
this, but just add an argument to gfs2_replay_incr_blk() such that you can tell it how many blocks to increment, rather than just assuming a single block as it does at the moment. Otherwise this can easily get missed when someone looks at the code in future, and expects gfs2_replay_incr_blk to be
the only thing that changes the position during recovery,

Quoted 10 lines
Quoted 4 lines

If we really want to encapsulate "add modulo jd->jd_blocks", it's also open-coded in find_good_lh and jhead_scan.

Andreas

I wonder if those will go away with Abhi's patch set in due course?

Steve.

Yeah... find_good_lh and jhead_scan will go away with my patch set.

--Abhi