From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Mon, 3 Jul 2017 10:32:09 +0100 Subject: [Cluster-devel] [GFS2 PATCH] GFS2: Rework gfs2_logd daemon In-Reply-To: <995075348.27804274.1498841138568.JavaMail.zimbra@redhat.com> References: <995075348.27804274.1498841138568.JavaMail.zimbra@redhat.com> Message-ID: <2d222be3-0aec-8bfc-8139-e10ad31ebe34@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi, On 30/06/17 17:45, Bob Peterson wrote: > Hi, > > This patch reorganizes some of the hokey logic in gfs2_logd. > It also tries to only fetch the logd_secs tunable only once per > second to avoid too many spinlock conflicts. > > Signed-off-by: Bob Peterson Again, please explain what the benefit is. The tunable should only every be accessed by gfs2_logd and by a userland process that is updating the tunable. If there is contention here, something is very wrong. The patch appears to remove the check for whether a journal or ail flush is required before gfs2_logd sleeps, so this appears that it would likely make performance worse, Steve. > --- > diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c > index 32aa1f0..3f18db4 100644 > --- a/fs/gfs2/log.c > +++ b/fs/gfs2/log.c > @@ -915,15 +915,12 @@ int gfs2_logd(void *data) > struct gfs2_sbd *sdp = data; > unsigned long t = 1; > DEFINE_WAIT(wait); > - bool did_flush; > + unsigned long tune_time = 0; > > while (!kthread_should_stop()) { > - > - did_flush = false; > if (gfs2_jrnl_flush_reqd(sdp) || t == 0) { > gfs2_ail1_empty(sdp); > gfs2_log_flush(sdp, NULL, NORMAL_FLUSH); > - did_flush = true; > } > > if (gfs2_ail_flush_reqd(sdp)) { > @@ -931,26 +928,22 @@ int gfs2_logd(void *data) > gfs2_ail1_wait(sdp); > gfs2_ail1_empty(sdp); > gfs2_log_flush(sdp, NULL, NORMAL_FLUSH); > - did_flush = true; > } > > - if (!gfs2_ail_flush_reqd(sdp) || did_flush) > - wake_up(&sdp->sd_log_waitq); > - > - t = gfs2_tune_get(sdp, gt_logd_secs) * HZ; > + wake_up(&sdp->sd_log_waitq); > > try_to_freeze(); > > - do { > - prepare_to_wait(&sdp->sd_logd_waitq, &wait, > - TASK_INTERRUPTIBLE); > - if (!gfs2_ail_flush_reqd(sdp) && > - !gfs2_jrnl_flush_reqd(sdp) && > - !kthread_should_stop()) > - t = schedule_timeout(t); > - } while(t && !gfs2_ail_flush_reqd(sdp) && > - !gfs2_jrnl_flush_reqd(sdp) && > - !kthread_should_stop()); > + if (kthread_should_stop()) > + break; > + > + if (time_after(jiffies, tune_time + HZ)) { > + t = gfs2_tune_get(sdp, gt_logd_secs) * HZ; > + tune_time = jiffies; > + } > + prepare_to_wait(&sdp->sd_logd_waitq, &wait, > + TASK_INTERRUPTIBLE); > + t = schedule_timeout(t); > finish_wait(&sdp->sd_logd_waitq, &wait); > } >