[Cluster-devel] [PATCH 0/3] local statfs improvements

[Cluster-devel] [PATCH 0/3] local statfs improvements

[Abhi Das]

With this patchset, we don't write to the local statfs file
anymore. The local statfs data is written into the journal
and synced to the master statfs file during a log flush or
during recovery.

Abhi Das (3):
  gfs2: Don't write updates to local statfs file
  gfs2: Add fields for statfs info in struct gfs2_log_header_host
  gfs2: Recover statfs info in journal head

 fs/gfs2/incore.h   |   4 ++
 fs/gfs2/lops.c     |  12 ++++-
 fs/gfs2/lops.h     |   1 +
 fs/gfs2/recovery.c | 125 +++++++++++++++++++++++++++++++++++++++++++++
 fs/gfs2/super.c    |   2 +-
 fs/gfs2/super.h    |   2 +
 6 files changed, 143 insertions(+), 3 deletions(-)

-- 
2.20.1

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

[Abhi Das]

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

[Cluster-devel] [PATCH 2/3] gfs2: Add fields for statfs info in struct gfs2_log_header_host

[Abhi Das]

And read these in __get_log_header() from the log header.
Also make gfs2_statfs_change_out() non-static so it can be used
outside of super.c

Signed-off-by: Abhi Das <adas@redhat.com>
---
 fs/gfs2/incore.h   | 4 ++++
 fs/gfs2/recovery.c | 4 ++++
 fs/gfs2/super.c    | 2 +-
 fs/gfs2/super.h    | 2 ++
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index ca2ec02436ec..9fc12206a3ad 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -41,6 +41,10 @@ struct gfs2_log_header_host {
 	u32 lh_flags;		/* GFS2_LOG_HEAD_... */
 	u32 lh_tail;		/* Block number of log tail */
 	u32 lh_blkno;
+
+	s64 lh_local_total;
+	s64 lh_local_free;
+	s64 lh_local_dinodes;
 };
 
 /*
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 390ea79d682c..a8bb17e355b8 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -144,6 +144,10 @@ int __get_log_header(struct gfs2_sbd *sdp, const struct gfs2_log_header *lh,
 	head->lh_tail = be32_to_cpu(lh->lh_tail);
 	head->lh_blkno = be32_to_cpu(lh->lh_blkno);
 
+	head->lh_local_total = be64_to_cpu(lh->lh_local_total);
+	head->lh_local_free = be64_to_cpu(lh->lh_local_free);
+	head->lh_local_dinodes = be64_to_cpu(lh->lh_local_dinodes);
+
 	return 0;
 }
 /**
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 9f4d9e7be839..4c51d30d65c5 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -224,7 +224,7 @@ void gfs2_statfs_change_in(struct gfs2_statfs_change_host *sc, const void *buf)
 	sc->sc_dinodes = be64_to_cpu(str->sc_dinodes);
 }
 
-static void gfs2_statfs_change_out(const struct gfs2_statfs_change_host *sc, void *buf)
+void gfs2_statfs_change_out(const struct gfs2_statfs_change_host *sc, void *buf)
 {
 	struct gfs2_statfs_change *str = buf;
 
diff --git a/fs/gfs2/super.h b/fs/gfs2/super.h
index 51900554ed81..ed4f5cb29074 100644
--- a/fs/gfs2/super.h
+++ b/fs/gfs2/super.h
@@ -37,6 +37,8 @@ extern void gfs2_statfs_change(struct gfs2_sbd *sdp, s64 total, s64 free,
 			       s64 dinodes);
 extern void gfs2_statfs_change_in(struct gfs2_statfs_change_host *sc,
 				  const void *buf);
+extern void gfs2_statfs_change_out(const struct gfs2_statfs_change_host *sc,
+				   void *buf);
 extern void update_statfs(struct gfs2_sbd *sdp, struct buffer_head *m_bh,
 			  struct buffer_head *l_bh);
 extern int gfs2_statfs_sync(struct super_block *sb, int type);
-- 
2.20.1

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

[Abhi Das]

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 <adas@redhat.com>
---
 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, &gh);
+	if (error)
+		goto out;
+
+	error = gfs2_meta_inode_buffer(ip, &bh);
+	if (error)
+		goto out_unlock;
+
+	spin_lock(&sdp->sd_statfs_spin);
+	if (head) {
+		gfs2_statfs_change_in(&sc, 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(&sc, 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(&sdp->sd_statfs_spin);
+	mark_buffer_dirty(bh);
+	brelse(bh);
+	gfs2_meta_sync(ip->i_gl);
+
+out_unlock:
+	gfs2_glock_dq_uninit(&gh);
+out:
+	return error;
+}
+
+static void recover_local_statfs(struct gfs2_jdesc *jd,
+				 struct gfs2_log_header_host *head)
+{
+	struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
+	struct inode *master = NULL, *local = NULL;
+	int error;
+
+	if (!head->lh_local_total && !head->lh_local_free
+	    && !head->lh_local_dinodes) /* No change */
+		goto out;
+
+	error = lookup_statfs_inodes(jd, &master, &local);
+	if (error)
+		goto out;
+	error = update_statfs_inode(jd, GFS2_I(master), head);
+	if (error)
+		goto iput_inodes;
+	error = update_statfs_inode(jd, GFS2_I(local), NULL);
+	if (error)
+		goto iput_inodes;
+	if (jd->jd_jid == sdp->sd_lockstruct.ls_jid)
+		memset(&sdp->sd_statfs_local, 0,
+		       sizeof(struct gfs2_statfs_change_host));
+
+iput_inodes:
+	iput(master);
+	iput(local);
+out:
+	return;
+}
+
 void gfs2_recover_func(struct work_struct *work)
 {
 	struct gfs2_jdesc *jd = container_of(work, struct gfs2_jdesc, jd_work);
@@ -415,6 +535,7 @@ void gfs2_recover_func(struct work_struct *work)
 				goto fail_gunlock_thaw;
 		}
 
+		recover_local_statfs(jd, &head);
 		clean_journal(jd, &head);
 		up_read(&sdp->sd_log_flush_lock);
 
-- 
2.20.1

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

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

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

[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 <adas@redhat.com>
> ---
>  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, &gh);
> +	if (error)
> +		goto out;
> +
> +	error = gfs2_meta_inode_buffer(ip, &bh);
> +	if (error)
> +		goto out_unlock;
> +
> +	spin_lock(&sdp->sd_statfs_spin);
> +	if (head) {
> +		gfs2_statfs_change_in(&sc, 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(&sc, 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(&sdp->sd_statfs_spin);
> +	mark_buffer_dirty(bh);
> +	brelse(bh);
> +	gfs2_meta_sync(ip->i_gl);
> +
> +out_unlock:
> +	gfs2_glock_dq_uninit(&gh);
> +out:
> +	return error;
> +}
> +
> +static void recover_local_statfs(struct gfs2_jdesc *jd,
> +				 struct gfs2_log_header_host *head)
> +{
> +	struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> +	struct inode *master = NULL, *local = NULL;
> +	int error;
> +
> +	if (!head->lh_local_total && !head->lh_local_free
> +	    && !head->lh_local_dinodes) /* No change */
> +		goto out;
> +
> +	error = lookup_statfs_inodes(jd, &master, &local);
> +	if (error)
> +		goto out;
> +	error = update_statfs_inode(jd, GFS2_I(master), head);
> +	if (error)
> +		goto iput_inodes;
> +	error = update_statfs_inode(jd, GFS2_I(local), NULL);
> +	if (error)
> +		goto iput_inodes;
> +	if (jd->jd_jid == sdp->sd_lockstruct.ls_jid)
> +		memset(&sdp->sd_statfs_local, 0,
> +		       sizeof(struct gfs2_statfs_change_host));
> +
> +iput_inodes:
> +	iput(master);
> +	iput(local);
> +out:
> +	return;
> +}
> +
>  void gfs2_recover_func(struct work_struct *work)
>  {
>  	struct gfs2_jdesc *jd = container_of(work, struct gfs2_jdesc, jd_work);
> @@ -415,6 +535,7 @@ void gfs2_recover_func(struct work_struct *work)
>  				goto fail_gunlock_thaw;
>  		}
>  
> +		recover_local_statfs(jd, &head);
>  		clean_journal(jd, &head);
>  		up_read(&sdp->sd_log_flush_lock);
>  
> --
> 2.20.1
> 
> 
Hi,

Why do we need to look up all these inodes?

Function init_inodes() looks up sd_statfs_inode and init_per_node() looks up
the statfs_changeX file, both of which are called pretty early during mount.

It seems to me sdp->sd_statfs_inode should already contain the master statfs
inode and sdp->sd_sc_inode should contain the current statfs_changeX inode
until unmount.

Of course, since the recover workqueue is initialized earlier, I'm guessing
maybe dlm can call gfs2 to do recovery before this initialization is done?
Maybe we can move the lookups prior to this or the workqueue after it?

It just seems like we can somehow avoid these lookups because they're done
elsewhere. But I haven't traced the code paths and you have, so maybe I'm
off base here.

Regards,

Bob Peterson

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

[Abhijith Das]

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>

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

[Abhijith Das]

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

> ----- 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 <adas@redhat.com>
> > ---
> >  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,
> &gh);
> > +     if (error)
> > +             goto out;
> > +
> > +     error = gfs2_meta_inode_buffer(ip, &bh);
> > +     if (error)
> > +             goto out_unlock;
> > +
> > +     spin_lock(&sdp->sd_statfs_spin);
> > +     if (head) {
> > +             gfs2_statfs_change_in(&sc, 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(&sc, 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(&sdp->sd_statfs_spin);
> > +     mark_buffer_dirty(bh);
> > +     brelse(bh);
> > +     gfs2_meta_sync(ip->i_gl);
> > +
> > +out_unlock:
> > +     gfs2_glock_dq_uninit(&gh);
> > +out:
> > +     return error;
> > +}
> > +
> > +static void recover_local_statfs(struct gfs2_jdesc *jd,
> > +                              struct gfs2_log_header_host *head)
> > +{
> > +     struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> > +     struct inode *master = NULL, *local = NULL;
> > +     int error;
> > +
> > +     if (!head->lh_local_total && !head->lh_local_free
> > +         && !head->lh_local_dinodes) /* No change */
> > +             goto out;
> > +
> > +     error = lookup_statfs_inodes(jd, &master, &local);
> > +     if (error)
> > +             goto out;
> > +     error = update_statfs_inode(jd, GFS2_I(master), head);
> > +     if (error)
> > +             goto iput_inodes;
> > +     error = update_statfs_inode(jd, GFS2_I(local), NULL);
> > +     if (error)
> > +             goto iput_inodes;
> > +     if (jd->jd_jid == sdp->sd_lockstruct.ls_jid)
> > +             memset(&sdp->sd_statfs_local, 0,
> > +                    sizeof(struct gfs2_statfs_change_host));
> > +
> > +iput_inodes:
> > +     iput(master);
> > +     iput(local);
> > +out:
> > +     return;
> > +}
> > +
> >  void gfs2_recover_func(struct work_struct *work)
> >  {
> >       struct gfs2_jdesc *jd = container_of(work, struct gfs2_jdesc,
> jd_work);
> > @@ -415,6 +535,7 @@ void gfs2_recover_func(struct work_struct *work)
> >                               goto fail_gunlock_thaw;
> >               }
> >
> > +             recover_local_statfs(jd, &head);
> >               clean_journal(jd, &head);
> >               up_read(&sdp->sd_log_flush_lock);
> >
> > --
> > 2.20.1
> >
> >
> Hi,
>
> Why do we need to look up all these inodes?
>
> Function init_inodes() looks up sd_statfs_inode and init_per_node() looks
> up
> the statfs_changeX file, both of which are called pretty early during
> mount.
>
> It seems to me sdp->sd_statfs_inode should already contain the master
> statfs
> inode and sdp->sd_sc_inode should contain the current statfs_changeX inode
> until unmount.
>
> Of course, since the recover workqueue is initialized earlier, I'm guessing
> maybe dlm can call gfs2 to do recovery before this initialization is done?
> Maybe we can move the lookups prior to this or the workqueue after it?
>
> It just seems like we can somehow avoid these lookups because they're done
> elsewhere. But I haven't traced the code paths and you have, so maybe I'm
> off base here.
>
> Regards,
>
> Bob Peterson
>
>
If we're recovering for another node, we need to lookup its local statfs
inode. IIRC from my testing, the statfs inodes weren't initialized at the
point where we use them here, but I'll double-check. Regardless, there's a
bit of optimization that can be done here. I'll post a revised version.

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

[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@redhat.com 
> <mailto: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 at redhat.com <mailto: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

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>