[Cluster-devel] [PATCH 6/8] gfs2: instrumentation wrt log_flush stuck

[Andreas Gruenbacher]

On Fri, Jun 5, 2020 at 4:49 PM Bob Peterson <rpeterso at redhat.com> wrote:
> Hi Andreas,
>
> ----- Original Message -----
> (snip)
> > > @@ -970,7 +969,16 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct
> > > gfs2_glock *gl, u32 flags)
> > >
> > >         if (!(flags & GFS2_LOG_HEAD_FLUSH_NORMAL)) {
> > >                 if (!sdp->sd_log_idle) {
> > > +                       unsigned long start = jiffies;
> > > +
> > >                         for (;;) {
> > > +                               if (time_after(jiffies, start + (HZ *
> > > 600))) {
> >
> > This should probably have some rate limiting as well, for example:
>
> Seems unnecessary. If the log flush gets stuck, the message will be printed
> once, and at most every 10 minutes.

No, after ten minutes, the message will actually be printed for each
iteration of the loop. That's exactly why I was suggesting the rate
limiting.

> >                                         start = jiffies;
> >
> > I'm not sure what provides similar rate limiting in gfs2_ail1_flush.
> >
> > > +                                       fs_err(sdp, "Error: In
> > > gfs2_log_flush "
> > > +                                              "for 10 minutes! t=%d\n",
> > > +                                              current->journal_info ? 1 :
> > > 0);
> >
> > Please don't break the format string up like that; this makes grepping
> > harder.
>
> How do you propose I break present it without going over 80 character?
> I could #define it as a constant, or put it into a separate function
> that has less indentation, I suppose.

There *is* no hard 80 character limit. The checkpatch warnings
shouldn't push us into making our code worse. Also note that since
commit bdc48fa11e, checkpatch will only warn about lines longer than
100 characters.

Thanks,
Andreas