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