GFS2: eliminate gfs2_log_write_bh and simplify

Hi,

This patch makes no functional changes. Its goal is to simplify the code and make function gfs2_before_commit more readable. It does this by eliminating function gfs2_log_write_bh which was a one-line function that simply called gfs2_log_write. It simplifies things by introducing a variable bh to reduce the references to bd2->bd_bh.

Signed-off-by Bob Peterson <[email protected]>

---
  fs/gfs2/lops.c | 37 ++++++++++++-------------------------
  1 file changed, 12 insertions(+), 25 deletions(-)

fs/gfs2/lops.c +12 -25

index c8ff7b7954f0..438933eb5a25 100644
@@ -336,21 +336,6 @@ static void gfs2_log_write(struct gfs2_sbd *sdp, struct page *page,
  	}
  }
 
-/**
- * gfs2_log_write_bh - write a buffer's content to the log
- * @sdp: The super block
- * @bh: The buffer pointing to the in-place location
- *
- * This writes the content of the buffer to the next available location
- * in the log. The buffer will be unlocked once the i/o to the log has
- * completed.
- */
-
-static void gfs2_log_write_bh(struct gfs2_sbd *sdp, struct buffer_head *bh)
-{
-	gfs2_log_write(sdp, bh->b_page, bh->b_size, bh_offset(bh));
-}
-
  /**
   * gfs2_log_write_page - write one block stored in a page, into the log
   * @sdp: The superblock

@@ -454,25 +439,27 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
 
  		n = 0;
  		list_for_each_entry_continue(bd2, blist, bd_list) {
-			get_bh(bd2->bd_bh);
+			struct buffer_head *bh = bd2->bd_bh;
+
+			get_bh(bh);
  			gfs2_log_unlock(sdp);
-			lock_buffer(bd2->bd_bh);
+			lock_buffer(bh);
 
-			if (buffer_escaped(bd2->bd_bh)) {
+			if (buffer_escaped(bh)) {
  				void *kaddr;
  				page = mempool_alloc(gfs2_page_pool, GFP_NOIO);
  				ptr = page_address(page);
-				kaddr = kmap_atomic(bd2->bd_bh->b_page);
-				memcpy(ptr, kaddr + bh_offset(bd2->bd_bh),
-				       bd2->bd_bh->b_size);
+				kaddr = kmap_atomic(bh->b_page);
+				memcpy(ptr, kaddr + bh_offset(bh), bh->b_size);
  				kunmap_atomic(kaddr);
  				*(__be32 *)ptr = 0;
-				clear_buffer_escaped(bd2->bd_bh);
-				unlock_buffer(bd2->bd_bh);
-				brelse(bd2->bd_bh);
+				clear_buffer_escaped(bh);
+				unlock_buffer(bh);
+				brelse(bh);
  				gfs2_log_write_page(sdp, page);
  			} else {
-				gfs2_log_write_bh(sdp, bd2->bd_bh);
+				gfs2_log_write(sdp, bh->b_page, bh->b_size,
+					       bh_offset(bh));
  			}
  			gfs2_log_lock(sdp);
  			if (++n >= num)

Hi,

----- Original Message -----

| On 01/12/17 17:31, Bob Peterson wrote:
| > Hi,
| >
| > This patch makes no functional changes. Its goal is to simplify the
| > code and make function gfs2_before_commit more readable. It does
| > this by eliminating function gfs2_log_write_bh which was a one-line
| > function that simply called gfs2_log_write. It simplifies things
| > by introducing a variable bh to reduce the references to bd2->bd_bh.
| >
| > Signed-off-by: Bob Peterson <[email protected]>
| While using the new bh variable might improve things, I'm not at all
| convinced that eliminating gfs2_log_write_bh is a good plan, since it is
| a useful helper that is self-documenting,
|
| Steve.

I guess it comes down to coding preference. The official coding-style documentation doesn't give a suggested minimum size of a function; it only cautions against them being too long. It says:

"They should fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24, as we all know)"

But IMHO, one 24-line function is much preferred to 24 lines broken into six single-line, single-call functions like:

Function a()

{
    b();
}

If a function gets too long, it is difficult to understand, but IMHO the opposite is also true: If there are too many functions, the reader's brain suffers from "death by a thousand cuts."

There's also the argument against "jumping around in the code." If I read through a function of 24 lines, it makes some logical sense. If I can't read it through because I need to keep interrupting the flow by jumping to the sub-functions to analyze what they do, it makes it twice as hard to understand.

In my way of thinking, a one-line function that simply calls another function is especially useless when it is only called in one place like this one. The only thing it achieves is to obfuscate the code and make it harder for maintainers to understand. If it's called in two places, then sure; it's useful.

IOW, a one-line self-documenting helper doesn't make the code more clear; it only adds to the levels of indirection needed in one's brain to keep them all straight. It only points out the shortcomings of the calling function to make things clear.

Having all these tiny little functions may seem helpful as they break the code down into more digestible morsels. But in the long run, it's harder because it becomes impossible to keep all the helpers straight.

Would it be more acceptable if I moved the explanation comment before the call? So instead of:

/**
* gfs2_log_write_bh - write a buffer's content to the log
* @sdp: The super block
* @bh: The buffer pointing to the in-place location
*
* This writes the content of the buffer to the next available location
* in the log. The buffer will be unlocked once the i/o to the log has
* completed.
*/

static void gfs2_log_write_bh(struct gfs2_sbd *sdp, struct buffer_head *bh)

{
	gfs2_log_write(sdp, bh->b_page, bh->b_size, bh_offset(bh));
}
(-----------------long span of code to interrupt your brain---------------)
  			} else {
				gfs2_log_write_bh(sdp, bd2->bd_bh);

to something more succinct like this?

  			} else {
				/* write the content of the buffer to the
				   next available location in the log. The
				   buffer will be unlocked once the i/o to
				   the log has completed.
				 */
				gfs2_log_write(sdp, bd2->bd_bh);

Regards,

Bob Peterson