* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor [not found] <17883672.769837.1533754253446.JavaMail.zimbra@redhat.com> @ 2018-08-08 18:52 ` Bob Peterson 2018-08-08 19:21 ` Andreas Gruenbacher ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Bob Peterson @ 2018-08-08 18:52 UTC (permalink / raw) To: cluster-devel.redhat.com 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); } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-08 18:52 ` [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor Bob Peterson @ 2018-08-08 19:21 ` Andreas Gruenbacher 2018-08-08 21:59 ` Abhijith Das 2018-08-09 9:35 ` Steven Whitehouse 2 siblings, 0 replies; 8+ messages in thread From: Andreas Gruenbacher @ 2018-08-08 19:21 UTC (permalink / raw) To: cluster-devel.redhat.com 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); > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-08 18:52 ` [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor Bob Peterson 2018-08-08 19:21 ` Andreas Gruenbacher @ 2018-08-08 21:59 ` Abhijith Das 2018-08-09 9:35 ` Steven Whitehouse 2 siblings, 0 replies; 8+ messages in thread From: Abhijith Das @ 2018-08-08 21:59 UTC (permalink / raw) To: cluster-devel.redhat.com ----- 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); > } > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-08 18:52 ` [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor Bob Peterson 2018-08-08 19:21 ` Andreas Gruenbacher 2018-08-08 21:59 ` Abhijith Das @ 2018-08-09 9:35 ` Steven Whitehouse 2018-08-09 12:15 ` Bob Peterson 2018-08-10 12:13 ` Andreas Gruenbacher 2 siblings, 2 replies; 8+ messages in thread From: Steven Whitehouse @ 2018-08-09 9:35 UTC (permalink / raw) To: cluster-devel.redhat.com 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-09 9:35 ` Steven Whitehouse @ 2018-08-09 12:15 ` Bob Peterson 2018-08-10 12:13 ` Andreas Gruenbacher 1 sibling, 0 replies; 8+ messages in thread From: Bob Peterson @ 2018-08-09 12:15 UTC (permalink / raw) To: cluster-devel.redhat.com ----- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-09 9:35 ` Steven Whitehouse 2018-08-09 12:15 ` Bob Peterson @ 2018-08-10 12:13 ` Andreas Gruenbacher 2018-08-10 12:21 ` Steven Whitehouse 1 sibling, 1 reply; 8+ messages in thread From: Andreas Gruenbacher @ 2018-08-10 12:13 UTC (permalink / raw) To: cluster-devel.redhat.com 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-10 12:13 ` Andreas Gruenbacher @ 2018-08-10 12:21 ` Steven Whitehouse 2018-08-10 14:26 ` Abhijith Das 0 siblings, 1 reply; 8+ messages in thread From: Steven Whitehouse @ 2018-08-10 12:21 UTC (permalink / raw) To: cluster-devel.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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor 2018-08-10 12:21 ` Steven Whitehouse @ 2018-08-10 14:26 ` Abhijith Das 0 siblings, 0 replies; 8+ messages in thread From: Abhijith Das @ 2018-08-10 14:26 UTC (permalink / raw) To: cluster-devel.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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-10 14:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <17883672.769837.1533754253446.JavaMail.zimbra@redhat.com>
2018-08-08 18:52 ` [Cluster-devel] [GFS2 PATCH] GFS2: Simplify iterative add loop in foreach_descriptor Bob Peterson
2018-08-08 19:21 ` Andreas Gruenbacher
2018-08-08 21:59 ` Abhijith Das
2018-08-09 9:35 ` Steven Whitehouse
2018-08-09 12:15 ` Bob Peterson
2018-08-10 12:13 ` Andreas Gruenbacher
2018-08-10 12:21 ` Steven Whitehouse
2018-08-10 14:26 ` Abhijith Das
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).