Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
- Original Message - > From: "Steven Whitehouse" > To: "Andreas Gruenbacher" > Cc: "Bob Peterson" , "cluster-devel" > , "Abhijith Das" > > 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 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 > >>> --- > >>>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 += 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
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 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 --- 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 += 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.
Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
On 9 August 2018 at 11:35, Steven Whitehouse 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 >> --- >> 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 += 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
Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
- 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
Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
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 --- 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 += 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.
Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
- Original Message - > From: "Bob Peterson" > To: "cluster-devel" > 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 Hi, Looks good. ACK. Cheers! --Abhi Reviewed-by: Abhi Das > --- > 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 += length; > + if (start >= jd->jd_blocks) > + start -= jd->jd_blocks; > > brelse(bh); > } > >
Re: [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor
On 8 August 2018 at 20: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 Reviewed-by: Andreas Gruenbacher > --- > 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 += length; > + if (start >= jd->jd_blocks) > + start -= jd->jd_blocks; > > brelse(bh); > } >
