[Cluster-devel] [PATCH 1/3] gfs2: Don't write updates to local statfs file

[Abhijith Das <adas@redhat.com>]

On Wed, Aug 19, 2020 at 12:07 PM Bob Peterson <rpeterso@redhat.com> wrote:

> ----- Original Message -----
> > We store the local statfs info in the journal header now so
> > there's no need to write to the local statfs file anymore.
> >
> > Signed-off-by: Abhi Das <adas@redhat.com>
> > ---
> >  fs/gfs2/lops.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> > index cb2a11b458c6..53d2dbf6605e 100644
> > --- a/fs/gfs2/lops.c
> > +++ b/fs/gfs2/lops.c
> > @@ -104,7 +104,15 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct
> > buffer_head *bh,
> >       BUG_ON(!buffer_pinned(bh));
> >
> >       lock_buffer(bh);
> > -     mark_buffer_dirty(bh);
> > +     /*
> > +      * We want to eliminate the local statfs file eventually.
> > +      * But, for now, we're simply not going to update it by
> > +      * never marking its buffers dirty
> > +      */
> > +     if (!(bd->bd_gl->gl_name.ln_type == LM_TYPE_INODE &&
> > +           bd->bd_gl->gl_object == GFS2_I(sdp->sd_sc_inode)))
> > +             mark_buffer_dirty(bh);
> > +
> >       clear_buffer_pinned(bh);
> >
> >       if (buffer_is_rgrp(bd))
> > --
> > 2.20.1
>
> Hi,
>
> This seems dangerous to me. It can only get to gfs2_unpin by trying to
> commit buffers for a transaction. If the buffers aren't marked dirty,
> that means transactions will be queued to the ail1 list that won't be
> fully written. So what happens to them? Do they eventually get freed?
>
> I'm also concerned about a potential impact to performance, since
> gfs2_unpin gets called with every metadata buffer that's used.
> The additional if checks may not costs us much time-wise, but it's a
> pretty hot function.
>
> Can't we accomplish the same thing by making function update_statfs()
> never add the buffers to the transaction in the first place?
> IOW, by just removing the line:
>         gfs2_trans_add_meta(m_ip->i_gl, m_bh);
> That way we don't need to worry about its buffer getting pinned,
> unpinned and queued to the ail.
>
> Regards,
>
> Bob Peterson
>
>
Fair point. I'll post an updated version of this patch that  doesn't queue
the buffer in the first place.

Cheers!
--Abhi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20200820/7031ceb0/attachment.htm>