[Cluster-devel] [GFS2 PATCH] GFS2: Reduce number of log flushes

[Cluster-devel] [GFS2 PATCH] GFS2: Reduce number of log flushes

[Bob Peterson]

Hi,

This patch reduces the number of log flushes.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
---
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 4bdcf37..7d795e7 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -118,7 +118,13 @@ void gfs2_ail_flush(struct gfs2_glock *gl, bool fsync)
 		return;
 	__gfs2_ail_flush(gl, fsync);
 	gfs2_trans_end(sdp);
-	gfs2_log_flush(sdp, NULL);
+	/* if fsync is true, we were called from gfs2_fsync, which means
+	   we just called sync_inode_metadata, which calls sync_inode,
+	   which calls writeback_single_inode, which calls write_inode,
+	   which calls write_inode which calls gfs2_log_flush. So the call
+	   is redundant. */
+	if (!fsync)
+		gfs2_log_flush(sdp, NULL);
 }
 
 /**
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 3cbac68..5904ce0 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1542,7 +1542,11 @@ static void gfs2_evict_inode(struct inode *inode)
 	goto out_unlock;
 
 out_truncate:
-	gfs2_log_flush(sdp, ip->i_gl);
+	/* write_inode_now calls writeback_single_inode, which calls
+	   gfs2_write_inode. gfs2_write_inode calls gfs2_log_flush if
+	   (wbc->sync_mode == WB_SYNC_ALL), which it is since we pass 1.
+	   therefore, this call to gfs2_log_flush is redundant:
+	   gfs2_log_flush(sdp, ip->i_gl); */
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);
 

[Cluster-devel] [GFS2 PATCH] GFS2: Reduce number of log flushes

[Steven Whitehouse]

Hi,

On Wed, 2012-08-08 at 16:10 -0400, Bob Peterson wrote:
> Hi,
> 
> This patch reduces the number of log flushes.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> ---
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 4bdcf37..7d795e7 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -118,7 +118,13 @@ void gfs2_ail_flush(struct gfs2_glock *gl, bool fsync)
>  		return;
>  	__gfs2_ail_flush(gl, fsync);
>  	gfs2_trans_end(sdp);
> -	gfs2_log_flush(sdp, NULL);
> +	/* if fsync is true, we were called from gfs2_fsync, which means
> +	   we just called sync_inode_metadata, which calls sync_inode,
> +	   which calls writeback_single_inode, which calls write_inode,
> +	   which calls write_inode which calls gfs2_log_flush. So the call
> +	   is redundant. */
> +	if (!fsync)
> +		gfs2_log_flush(sdp, NULL);
>  }
What ensures that the revokes are on disk?

>  
>  /**
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 3cbac68..5904ce0 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1542,7 +1542,11 @@ static void gfs2_evict_inode(struct inode *inode)
>  	goto out_unlock;
>  
>  out_truncate:
> -	gfs2_log_flush(sdp, ip->i_gl);
> +	/* write_inode_now calls writeback_single_inode, which calls
> +	   gfs2_write_inode. gfs2_write_inode calls gfs2_log_flush if
> +	   (wbc->sync_mode == WB_SYNC_ALL), which it is since we pass 1.
> +	   therefore, this call to gfs2_log_flush is redundant:
> +	   gfs2_log_flush(sdp, ip->i_gl); */
>  	write_inode_now(inode, 1);
>  	gfs2_ail_flush(ip->i_gl, 0);
>  
> 
This one looks like it is probably correct, however there is no point in
adding a comment to refer to code that has just been removed. Just
remove that call and put the explanation in the patch description, or
maybe include just a short comment so say that there is a log flush
implicit in the write_inode_now() call,

Steve.

[Cluster-devel] [GFS2 PATCH][TRY #2] GFS2: eliminate redundant log flush

[Bob Peterson]

----- Original Message -----
| What ensures that the revokes are on disk?
(snip)
| This one looks like it is probably correct, however there is no point
| in
| adding a comment to refer to code that has just been removed. Just
| remove that call and put the explanation in the patch description, or
| maybe include just a short comment so say that there is a log flush
| implicit in the write_inode_now() call,
| 
| Steve.

You're right about the revokes; my bad. Here is a respin, with just
a comment in the patch rather than in the code:

Hi,

In function gfs2_evict_inode, there was a call to gfs2_log_flush
that is unnecessary because it immediately calls write_inode_now
which makes another call to gfs2_log_flush. This patch eliminates
the redundant log flush.

Regards,

Bob Peterson
Red Hat GFS

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
---
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 3cbac68..ba4fde5 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1542,7 +1542,6 @@ static void gfs2_evict_inode(struct inode *inode)
 	goto out_unlock;
 
 out_truncate:
-	gfs2_log_flush(sdp, ip->i_gl);
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);
 

[Cluster-devel] [GFS2 PATCH][TRY #2] GFS2: eliminate redundant log flush

[Bob Peterson]

----- Original Message -----
| ----- Original Message -----
| | What ensures that the revokes are on disk?
| (snip)
| | This one looks like it is probably correct, however there is no
| | point
| | in
| | adding a comment to refer to code that has just been removed. Just
| | remove that call and put the explanation in the patch description,
| | or
| | maybe include just a short comment so say that there is a log flush
| | implicit in the write_inode_now() call,
| | 
| | Steve.
| 
| You're right about the revokes; my bad. Here is a respin, with just
| a comment in the patch rather than in the code:
| 
| Hi,
| 
| In function gfs2_evict_inode, there was a call to gfs2_log_flush
| that is unnecessary because it immediately calls write_inode_now
| which makes another call to gfs2_log_flush. This patch eliminates
| the redundant log flush.
| 
| Regards,
| 
| Bob Peterson
| Red Hat GFS
| 
| Signed-off-by: Bob Peterson <rpeterso@redhat.com>
| ---
| diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
| index 3cbac68..ba4fde5 100644
| --- a/fs/gfs2/super.c
| +++ b/fs/gfs2/super.c
| @@ -1542,7 +1542,6 @@ static void gfs2_evict_inode(struct inode
| *inode)
|  	goto out_unlock;
|  
|  out_truncate:
| -	gfs2_log_flush(sdp, ip->i_gl);
|  	write_inode_now(inode, 1);
|  	gfs2_ail_flush(ip->i_gl, 0);
|  
| 

Hi,

I've still not heard back on this revised patch from 09 Aug, and it
has not gone into -nmw. Perhaps it was missed?

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [GFS2 PATCH][TRY #2] GFS2: eliminate redundant log flush

[Steven Whitehouse]

Hi,

On Fri, 2012-09-14 at 10:42 -0400, Bob Peterson wrote:
> ----- Original Message -----
> | ----- Original Message -----
> | | What ensures that the revokes are on disk?
> | (snip)
> | | This one looks like it is probably correct, however there is no
> | | point
> | | in
> | | adding a comment to refer to code that has just been removed. Just
> | | remove that call and put the explanation in the patch description,
> | | or
> | | maybe include just a short comment so say that there is a log flush
> | | implicit in the write_inode_now() call,
> | | 
> | | Steve.
> | 
> | You're right about the revokes; my bad. Here is a respin, with just
> | a comment in the patch rather than in the code:
> | 
> | Hi,
> | 
> | In function gfs2_evict_inode, there was a call to gfs2_log_flush
> | that is unnecessary because it immediately calls write_inode_now
> | which makes another call to gfs2_log_flush. This patch eliminates
> | the redundant log flush.
> | 
> | Regards,
> | 
> | Bob Peterson
> | Red Hat GFS
> | 
> | Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> | ---
> | diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> | index 3cbac68..ba4fde5 100644
> | --- a/fs/gfs2/super.c
> | +++ b/fs/gfs2/super.c
> | @@ -1542,7 +1542,6 @@ static void gfs2_evict_inode(struct inode
> | *inode)
> |  	goto out_unlock;
> |  
> |  out_truncate:
> | -	gfs2_log_flush(sdp, ip->i_gl);
> |  	write_inode_now(inode, 1);
> |  	gfs2_ail_flush(ip->i_gl, 0);
> |  
> | 
> 
> Hi,
> 
> I've still not heard back on this revised patch from 09 Aug, and it
> has not gone into -nmw. Perhaps it was missed?
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems

Yes, I think that might have fallen off the stack. Apologies for the
delay. However I think we should check with Ben as this is the same bit
of code that he is looking at currently,

Steve.