[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[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 <rpeterso@redhat.com>
---
 fs/gfs2/recovery.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 0f501f938d1c..6c6b19263b82 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -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);
 	}

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Andreas Gruenbacher]

On 8 August 2018 at 20:52, Bob Peterson <rpeterso@redhat.com> wrote:
> 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 <rpeterso@redhat.com>

Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

> ---
>  fs/gfs2/recovery.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 0f501f938d1c..6c6b19263b82 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -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);
>         }
>

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Abhijith Das]

----- Original Message -----
> From: "Bob Peterson" <rpeterso@redhat.com>
> To: "cluster-devel" <cluster-devel@redhat.com>
> Sent: Wednesday, August 8, 2018 1:52:03 PM
> Subject: [Cluster-devel] [GFS2 PATCH] 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 <rpeterso@redhat.com>

Hi,

Looks good. ACK.

Cheers!
--Abhi

Reviewed-by: Abhi Das <adas@redhat.com>

> ---
>  fs/gfs2/recovery.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 0f501f938d1c..6c6b19263b82 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -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);
>  	}
> 
> 

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Steven Whitehouse]

Hi,


On 08/08/18 19:52, Bob Peterson wrote:
> 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 <rpeterso@redhat.com>
> ---
>   fs/gfs2/recovery.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 0f501f938d1c..6c6b19263b82 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -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,

Steve.

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Bob Peterson]

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

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Andreas Gruenbacher]

On 9 August 2018 at 11:35, Steven Whitehouse <swhiteho@redhat.com> wrote:
> Hi,
>
>
>
> On 08/08/18 19:52, Bob Peterson wrote:
>>
>> 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 <rpeterso@redhat.com>
>> ---
>>   fs/gfs2/recovery.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
>> index 0f501f938d1c..6c6b19263b82 100644
>> --- a/fs/gfs2/recovery.c
>> +++ b/fs/gfs2/recovery.c
>> @@ -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,

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

Andreas

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Steven Whitehouse]

Hi,


On 10/08/18 13:13, Andreas Gruenbacher wrote:
> On 9 August 2018 at 11:35, Steven Whitehouse <swhiteho@redhat.com> wrote:
>> Hi,
>>
>>
>>
>> On 08/08/18 19:52, Bob Peterson wrote:
>>> 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 <rpeterso@redhat.com>
>>> ---
>>>    fs/gfs2/recovery.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
>>> index 0f501f938d1c..6c6b19263b82 100644
>>> --- a/fs/gfs2/recovery.c
>>> +++ b/fs/gfs2/recovery.c
>>> @@ -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,
> 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.

[Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor

[Abhijith Das]

----- Original Message -----
> From: "Steven Whitehouse" <swhiteho@redhat.com>
> To: "Andreas Gruenbacher" <agruenba@redhat.com>
> Cc: "Bob Peterson" <rpeterso@redhat.com>, "cluster-devel" <cluster-devel@redhat.com>, "Abhijith Das"
> <adas@redhat.com>
> Sent: Friday, August 10, 2018 7:21:28 AM
> Subject: Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
> 
> Hi,
> 
> 
> On 10/08/18 13:13, Andreas Gruenbacher wrote:
> > On 9 August 2018 at 11:35, Steven Whitehouse <swhiteho@redhat.com> wrote:
> >> Hi,
> >>
> >>
> >>
> >> On 08/08/18 19:52, Bob Peterson wrote:
> >>> 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 <rpeterso@redhat.com>
> >>> ---
> >>>    fs/gfs2/recovery.c | 5 +++--
> >>>    1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> >>> index 0f501f938d1c..6c6b19263b82 100644
> >>> --- a/fs/gfs2/recovery.c
> >>> +++ b/fs/gfs2/recovery.c
> >>> @@ -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,
> > 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