From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bob Peterson <rpeterso@redhat.com>
Date: Wed, 19 Aug 2020 13:07:26 -0400 (EDT)
Subject: [Cluster-devel] [PATCH 1/3] gfs2: Don't write updates to
	local	statfs file
In-Reply-To: <20200813200114.5665-2-adas@redhat.com>
References: <20200813200114.5665-1-adas@redhat.com>
	<20200813200114.5665-2-adas@redhat.com>
Message-ID: <165825613.12780098.1597856846775.JavaMail.zimbra@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

----- 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