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

[Steven Whitehouse <swhiteho@redhat.com>]

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.