[Cluster-devel] [GFS2 PATCH] GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

[Cluster-devel] [GFS2 PATCH] GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

[Bob Peterson]

Hi,

Before this patch, tiny function gfs2_log_write called small function
gfs2_log_bmap to determine which journal block to write. If function
gfs2_log_bmap encountered a problem in its block mapping calculations,
it would return a block number of -1 to indicate an error. This error
was immediately ignored and forgotten: the caller went ahead and
tried to write to the log anyway. This patch fixes the problem
by checking the results of the block map calculations and doing
an assert withdraw if an error occurs. It also eliminates function
gfs2_log_bmap in favor of inlining the code to make the code more
readable.

This should improve overall journal integrity, as a precursor to
log writing improvements we are planning.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/lops.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 438933eb5a25..bac73cdaace8 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -138,23 +138,6 @@ static void gfs2_log_incr_head(struct gfs2_sbd *sdp)
 		sdp->sd_log_flush_head = 0;
 }
 
-static u64 gfs2_log_bmap(struct gfs2_sbd *sdp)
-{
-	unsigned int lbn = sdp->sd_log_flush_head;
-	struct gfs2_journal_extent *je;
-	u64 block;
-
-	list_for_each_entry(je, &sdp->sd_jdesc->extent_list, list) {
-		if ((lbn >= je->lblock) && (lbn < (je->lblock + je->blocks))) {
-			block = je->dblock + lbn - je->lblock;
-			gfs2_log_incr_head(sdp);
-			return block;
-		}
-	}
-
-	return -1;
-}
-
 /**
  * gfs2_end_log_write_bh - end log write of pagecache data with buffers
  * @sdp: The superblock
@@ -322,10 +305,23 @@ static struct bio *gfs2_log_get_bio(struct gfs2_sbd *sdp, u64 blkno)
 static void gfs2_log_write(struct gfs2_sbd *sdp, struct page *page,
 			   unsigned size, unsigned offset)
 {
-	u64 blkno = gfs2_log_bmap(sdp);
 	struct bio *bio;
+	unsigned int lbn = sdp->sd_log_flush_head;
+	struct gfs2_journal_extent *je;
+	u64 blkno = 0;
 	int ret;
 
+	/* determine the next block location in the log to be written */
+	list_for_each_entry(je, &sdp->sd_jdesc->extent_list, list) {
+		if ((lbn >= je->lblock) && (lbn < (je->lblock + je->blocks))) {
+			blkno = je->dblock + lbn - je->lblock;
+			gfs2_log_incr_head(sdp);
+			break;
+		}
+	}
+	if (gfs2_assert_withdraw(sdp, blkno != 0))
+		return;
+
 	bio = gfs2_log_get_bio(sdp, blkno);
 	ret = bio_add_page(bio, page, size, offset);
 	if (ret == 0) {

[Cluster-devel] [GFS2 PATCH] GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

[Steven Whitehouse]

On 01/12/17 17:34, Bob Peterson wrote:
> Hi,
>
> Before this patch, tiny function gfs2_log_write called small function
> gfs2_log_bmap to determine which journal block to write. If function
> gfs2_log_bmap encountered a problem in its block mapping calculations,
> it would return a block number of -1 to indicate an error. This error
> was immediately ignored and forgotten: the caller went ahead and
> tried to write to the log anyway. This patch fixes the problem
> by checking the results of the block map calculations and doing
> an assert withdraw if an error occurs. It also eliminates function
> gfs2_log_bmap in favor of inlining the code to make the code more
> readable.
>
> This should improve overall journal integrity, as a precursor to
> log writing improvements we are planning.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/lops.c | 32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 438933eb5a25..bac73cdaace8 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -138,23 +138,6 @@ static void gfs2_log_incr_head(struct gfs2_sbd *sdp)
>   		sdp->sd_log_flush_head = 0;
>   }
>   
> -static u64 gfs2_log_bmap(struct gfs2_sbd *sdp)
> -{
> -	unsigned int lbn = sdp->sd_log_flush_head;
> -	struct gfs2_journal_extent *je;
> -	u64 block;
> -
> -	list_for_each_entry(je, &sdp->sd_jdesc->extent_list, list) {
> -		if ((lbn >= je->lblock) && (lbn < (je->lblock + je->blocks))) {
> -			block = je->dblock + lbn - je->lblock;
> -			gfs2_log_incr_head(sdp);
> -			return block;
> -		}
> -	}
> -
> -	return -1;
> -}
> -
>   /**
>    * gfs2_end_log_write_bh - end log write of pagecache data with buffers
>    * @sdp: The superblock
> @@ -322,10 +305,23 @@ static struct bio *gfs2_log_get_bio(struct gfs2_sbd *sdp, u64 blkno)
>   static void gfs2_log_write(struct gfs2_sbd *sdp, struct page *page,
>   			   unsigned size, unsigned offset)
>   {
> -	u64 blkno = gfs2_log_bmap(sdp);
>   	struct bio *bio;
> +	unsigned int lbn = sdp->sd_log_flush_head;
> +	struct gfs2_journal_extent *je;
> +	u64 blkno = 0;
>   	int ret;
>   
> +	/* determine the next block location in the log to be written */
> +	list_for_each_entry(je, &sdp->sd_jdesc->extent_list, list) {
> +		if ((lbn >= je->lblock) && (lbn < (je->lblock + je->blocks))) {
> +			blkno = je->dblock + lbn - je->lblock;
> +			gfs2_log_incr_head(sdp);
> +			break;
> +		}
> +	}
> +	if (gfs2_assert_withdraw(sdp, blkno != 0))
> +		return;
> +
>   	bio = gfs2_log_get_bio(sdp, blkno);
>   	ret = bio_add_page(bio, page, size, offset);
>   	if (ret == 0) {
>

Again, I'm not sure that there is anything to be gained in readability 
by inlining gfs2_log_bmap(). Why not just add the new error check in 
that function?

Steve.

[Cluster-devel] [GFS2 PATCH] GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

[Bob Peterson]

----- Original Message -----
| On 01/12/17 17:34, Bob Peterson wrote:
| > Hi,
| >
| > Before this patch, tiny function gfs2_log_write called small function
| > gfs2_log_bmap to determine which journal block to write. If function
| > gfs2_log_bmap encountered a problem in its block mapping calculations,
| > it would return a block number of -1 to indicate an error. This error
| > was immediately ignored and forgotten: the caller went ahead and
| > tried to write to the log anyway. This patch fixes the problem
| > by checking the results of the block map calculations and doing
| > an assert withdraw if an error occurs. It also eliminates function
| > gfs2_log_bmap in favor of inlining the code to make the code more
| > readable.
| >
| > This should improve overall journal integrity, as a precursor to
| > log writing improvements we are planning.
| Again, I'm not sure that there is anything to be gained in readability
| by inlining gfs2_log_bmap(). Why not just add the new error check in
| that function?
| 
| Steve.

As explained in my previous email: "Death by a thousand cuts."
I much prefer the readability of one 17-line function to two
functions of 9 lines each.

Bob Peterson

[Cluster-devel] [GFS2 PATCH] GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

[Bob Peterson]

| As explained in my previous email: "Death by a thousand cuts."
| I much prefer the readability of one 17-line function to two
| functions of 9 lines each.

Make that one 9 line function and one 11 line function, since
I'd need to add a check for the return code to get the same result.

Bob Peterson

[Cluster-devel] [GFS2 PATCH] GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

[Steven Whitehouse]

On 04/12/17 14:36, Bob Peterson wrote:
> | As explained in my previous email: "Death by a thousand cuts."
> | I much prefer the readability of one 17-line function to two
> | functions of 9 lines each.
>
> Make that one 9 line function and one 11 line function, since
> I'd need to add a check for the return code to get the same result.
>
> Bob Peterson

The function length is really irrelevant... the question is whether it 
does one logical thing. In this case mapping the block that we are about 
to write is one thing, and actually writing it, is another,

Steve.