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

[Steven Whitehouse]

Hi,

On 20/08/2020 12:04, Abhijith Das wrote:
>
>
> On Wed, Aug 19, 2020 at 12:07 PM Bob Peterson <rpeterso at redhat.com 
> <mailto:rpeterso at 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 at redhat.com <mailto:adas at 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

You need to think about correctness at recovery time. It may be faster 
to not write the data into the journal for the local statfs file, but 
how will that affect recovery depending on whether that recovery is 
performed by either a newer or older kernel? Being backwards compatible 
might be more important in this case, so worth looking at carefully,

Steve.


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