On Tue, Dec 22, 2020 at 3:19 PM Bob Peterson <[email protected]> wrote:
>
> Hi,
>
> Comments inline.
>
> ----- Original Message -----
> > Wake up log waiters in gfs2_log_release when log space has actually become
> > available. This is a much better place for the wakeup than gfs2_logd.
> >
> > Check if enough log space is immeditely available before anything else. If
> > there isn't, use io_wait_event to wait instead of open-coding it.
> >
> > Signed-off-by: Andreas Gruenbacher <[email protected]>
> > ---
> > fs/gfs2/log.c | 54 ++++++++++++++++++++++-----------------------------
> > 1 file changed, 23 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> > index 95ad444bd3dc..7a65823ad1f3 100644
> > --- a/fs/gfs2/log.c
> > +++ b/fs/gfs2/log.c
> > @@ -420,6 +420,8 @@ void gfs2_log_release(struct gfs2_sbd *sdp, unsigned int
> > blks)
> > trace_gfs2_log_blocks(sdp, blks);
> > gfs2_assert_withdraw(sdp, atomic_read(&sdp->sd_log_blks_free) <=
> > sdp->sd_jdesc->jd_blocks);
> > + if (atomic_read(&sdp->sd_log_blks_needed))
> > + wake_up(&sdp->sd_log_waitq);
> > }
> >
> > /**
> > @@ -444,36 +446,33 @@ void gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned
> > int blks)
> > {
> > unsigned reserved_blks = 7 * (4096 / sdp->sd_vfs->s_blocksize);
> > unsigned wanted = blks + reserved_blks;
> > - DEFINE_WAIT(wait);
> > - int did_wait = 0;
> > unsigned int free_blocks;
> >
> > - atomic_add(blks, &sdp->sd_log_blks_needed);
> > -retry:
> > free_blocks = atomic_read(&sdp->sd_log_blks_free);
> > - if (unlikely(free_blocks <= wanted)) {
> > - do {
> > - prepare_to_wait_exclusive(&sdp->sd_log_waitq, &wait,
> > - TASK_UNINTERRUPTIBLE);
> > + while (free_blocks >= wanted) {
> > + if (atomic_try_cmpxchg(&sdp->sd_log_blks_free, &free_blocks,
> > + free_blocks - blks))
> > + return;
>
> This would be a good place to have cond_resched() or schedule() or something,
> no?
No, this is just an atomic_sub_if_positive(); see
atomic_dec_if_positive in atomic-fallback.h.
> > + }
> > +
> > + atomic_add(blks, &sdp->sd_log_blks_needed);
> > + for (;;) {
> > + if (current != sdp->sd_logd_process)
> > wake_up(&sdp->sd_logd_waitq);
> > - did_wait = 1;
> > - if (atomic_read(&sdp->sd_log_blks_free) <= wanted)
> > - io_schedule();
> > - free_blocks = atomic_read(&sdp->sd_log_blks_free);
> > - } while(free_blocks <= wanted);
> > - finish_wait(&sdp->sd_log_waitq, &wait);
> > + io_wait_event(sdp->sd_log_waitq,
> > + (free_blocks = atomic_read(&sdp->sd_log_blks_free),
> > + free_blocks >= wanted));
> > + do {
> > + if (atomic_try_cmpxchg(&sdp->sd_log_blks_free,
> > + &free_blocks,
> > + free_blocks - blks))
> > + goto reserved;
>
> Same here. We need to test these patches with a minimal number of cpus.
>
> > + } while (free_blocks >= wanted);
> > }
> > - if (atomic_cmpxchg(&sdp->sd_log_blks_free, free_blocks,
> > - free_blocks - blks) != free_blocks)
> > - goto retry;
> > - atomic_sub(blks, &sdp->sd_log_blks_needed);
> > - trace_gfs2_log_blocks(sdp, -blks);
> >
> > - /*
> > - * If we waited, then so might others, wake them up _after_ we get
> > - * our share of the log.
> > - */
> > - if (unlikely(did_wait))
> > +reserved:
> > + trace_gfs2_log_blocks(sdp, -blks);
> > + if (atomic_sub_return(blks, &sdp->sd_log_blks_needed))
> > wake_up(&sdp->sd_log_waitq);
> > }
> >
> > @@ -1190,7 +1189,6 @@ int gfs2_logd(void *data)
> > struct gfs2_sbd *sdp = data;
> > unsigned long t = 1;
> > DEFINE_WAIT(wait);
> > - bool did_flush;
> >
> > while (!kthread_should_stop()) {
> >
> > @@ -1209,12 +1207,10 @@ int gfs2_logd(void *data)
> > continue;
> > }
> >
> > - did_flush = false;
> > if (gfs2_jrnl_flush_reqd(sdp) || t == 0) {
> > gfs2_ail1_empty(sdp, 0);
> > gfs2_log_flush(sdp, NULL, GFS2_LOG_HEAD_FLUSH_NORMAL |
> > GFS2_LFC_LOGD_JFLUSH_REQD);
> > - did_flush = true;
> > }
> >
> > if (gfs2_ail_flush_reqd(sdp)) {
> > @@ -1223,12 +1219,8 @@ int gfs2_logd(void *data)
> > gfs2_ail1_empty(sdp, 0);
> > gfs2_log_flush(sdp, NULL, GFS2_LOG_HEAD_FLUSH_NORMAL |
> > GFS2_LFC_LOGD_AIL_FLUSH_REQD);
> > - 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;
> >
> > try_to_freeze();
> > --
> > 2.26.2
> >
> >
Thanks,
Andreas
