[Cluster-devel] [GFS2 PATCH] GFS2: eliminate gfs2_log_write_bh and simplify

[Steven Whitehouse]

Hi,


On 04/12/17 14:23, Bob Peterson wrote:
> 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 <rpeterso at redhat.com>
> | 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
Not really no. gfs2_log_write() takes a page/offset/size and 
gfs2_log_write_bh() is a simple wrapper which documents how to convert a 
buffer_head to that format. It is quite simple and I'm not really sure 
why it needs changing,

Steve.