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

[Abhijith Das <adas@redhat.com>]

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