GFS2: Fix gfs2_log_write and eliminate gfs2_log_bmap

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.

---
 fs/gfs2/lops.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

index 438933eb5a25..bac73cdaace8 100644
@@ -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) {

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

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