On Wed, Aug 19, 2020 at 12:07 PM Bob Peterson <[email protected]> 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 <[email protected]> > > --- > > 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
