Re: [Cluster-devel] [PATCH 3/3] gfs2: Recover statfs info in journal head

2020-08-19 Thread Bob Peterson
- Original Message -
> Apply the outstanding statfs changes in the journal head to the
> master statfs file. Zero out the local statfs file for good measure.
> 
> Signed-off-by: Abhi Das 
> ---
>  fs/gfs2/lops.c |   2 +-
>  fs/gfs2/lops.h |   1 +
>  fs/gfs2/recovery.c | 121 +
>  3 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 53d2dbf6605e..061747b959c8 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -831,7 +831,7 @@ static int buf_lo_scan_elements(struct gfs2_jdesc *jd,
> u32 start,
>   *
>   */
>  
> -static void gfs2_meta_sync(struct gfs2_glock *gl)
> +void gfs2_meta_sync(struct gfs2_glock *gl)
>  {
>   struct address_space *mapping = gfs2_glock2aspace(gl);
>   struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
> diff --git a/fs/gfs2/lops.h b/fs/gfs2/lops.h
> index 9c5e4e491e03..4a3d8aecdf82 100644
> --- a/fs/gfs2/lops.h
> +++ b/fs/gfs2/lops.h
> @@ -27,6 +27,7 @@ extern void gfs2_log_submit_bio(struct bio **biop, int
> opf);
>  extern void gfs2_pin(struct gfs2_sbd *sdp, struct buffer_head *bh);
>  extern int gfs2_find_jhead(struct gfs2_jdesc *jd,
>  struct gfs2_log_header_host *head, bool keep_cache);
> +extern void gfs2_meta_sync(struct gfs2_glock *gl);
>  
>  static inline unsigned int buf_limit(struct gfs2_sbd *sdp)
>  {
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index a8bb17e355b8..428a0aad49c6 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -296,6 +296,126 @@ static void gfs2_recovery_done(struct gfs2_sbd *sdp,
> unsigned int jid,
>   sdp->sd_lockstruct.ls_ops->lm_recovery_result(sdp, jid, 
> message);
>  }
>  
> +static int lookup_statfs_inodes(struct gfs2_jdesc *jd, struct inode
> **master,
> + struct inode **local)
> +{
> + int error = 0;
> + char buf[30];
> + struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> + struct inode *md = d_inode(sdp->sd_master_dir), *pn;
> +
> + *master = gfs2_lookup_simple(md, "statfs");
> + if (IS_ERR(*master)) {
> + error = PTR_ERR(*master);
> + fs_err(sdp, "Can't read in statfs inode: %d\n", error);
> + goto out;
> + }
> + pn = gfs2_lookup_simple(md, "per_node");
> + if (IS_ERR(pn)) {
> + error = PTR_ERR(pn);
> + fs_err(sdp, "Can't find per_node directory: %d\n", error);
> + goto put_m_ip;
> + }
> + sprintf(buf, "statfs_change%u", jd->jd_jid);
> + *local = gfs2_lookup_simple(pn, buf);
> + if (IS_ERR(*local)) {
> + error = PTR_ERR(*local);
> + fs_err(sdp, "Can't find local \"sc\" file for jid:%u: %d\n",
> +jd->jd_jid, error);
> + }
> + iput(pn);
> + if (!error)
> + return error;
> +put_m_ip:
> + iput(*master);
> +out:
> + return error;
> +}
> +
> +static int update_statfs_inode(struct gfs2_jdesc *jd, struct gfs2_inode *ip,
> +struct gfs2_log_header_host *head)
> +{
> + /*
> +  * If head is NULL, ip points to a local statfs file.
> +  * zero out the statfs data in the inode pointed to by ip.
> +  */
> + struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> + struct gfs2_statfs_change_host sc;
> + struct gfs2_holder gh;
> + struct buffer_head *bh;
> + int error = 0;
> +
> + error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_NOCACHE, );
> + if (error)
> + goto out;
> +
> + error = gfs2_meta_inode_buffer(ip, );
> + if (error)
> + goto out_unlock;
> +
> + spin_lock(>sd_statfs_spin);
> + if (head) {
> + gfs2_statfs_change_in(, bh->b_data + sizeof(struct 
> gfs2_dinode));
> + sc.sc_total += head->lh_local_total;
> + sc.sc_free += head->lh_local_free;
> + sc.sc_dinodes += head->lh_local_dinodes;
> + gfs2_statfs_change_out(, bh->b_data + sizeof(struct 
> gfs2_dinode));
> + fs_info(sdp, "jid=%u: Updated master statfs Total:%lld, "
> + "Free:%lld, Dinodes:%lld after change "
> + "[%+lld,%+lld,%+lld]\n", jd->jd_jid, sc.sc_total,
> + sc.sc_free, sc.sc_dinodes, head->lh_local_total,
> + head->lh_local_free, head->lh_local_dinodes);
> + } else {
> + memset(bh->b_data + sizeof(struct gfs2_dinode), 0,
> +sizeof(struct gfs2_statfs_change));
> + if (jd->jd_jid == sdp->sd_lockstruct.ls_jid) { /* This node's 
> journal */
> + sdp->sd_statfs_local.sc_total = 0;
> + sdp->sd_statfs_local.sc_free = 0;
> + sdp->sd_statfs_local.sc_dinodes = 0;
> + }
> + }
> + spin_unlock(>sd_statfs_spin);
> + mark_buffer_dirty(bh);
> + brelse(bh);
> + gfs2_meta_sync(ip->i_gl);
> +
> +out_unlock:
> 

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

2020-08-19 Thread Bob Peterson
- 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 
> ---
>  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