[Cluster-devel] [PATCH 3/4] GFS2: Implement iomap for block_map

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 20/10/16 17:14, Bob Peterson wrote:
> This patch implements iomap for block mapping, and switches the
> block_map function to use it under the covers.
Definitely going in the right direction now... a few comments below.

> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/bmap.c | 244 ++++++++++++++++++++++++++++++++++++++++++---------------
>   fs/gfs2/bmap.h |   4 +
>   2 files changed, 185 insertions(+), 63 deletions(-)
>
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index 774bdb8..b1bcdd6 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -13,6 +13,7 @@
>   #include <linux/blkdev.h>
>   #include <linux/gfs2_ondisk.h>
>   #include <linux/crc32.h>
> +#include <linux/iomap.h>
>   
>   #include "gfs2.h"
>   #include "incore.h"
> @@ -594,104 +595,221 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
>   }
>   
>   /**
> - * gfs2_block_map - Map a block from an inode to a disk block
> - * @inode: The inode
> - * @lblock: The logical block number
> - * @bh_map: The bh to be mapped
> - * @create: True if its ok to alloc blocks to satify the request
> + * hole_size - figure out the size of a hole
> + * @ip: The inode
> + * @lblock: The logical starting block number
> + * @mp: The metapath
>    *
> - * Sets buffer_mapped() if successful, sets buffer_boundary() if a
> - * read of metadata will be required before the next block can be
> - * mapped. Sets buffer_new() if new blocks were allocated.
> + * Returns: The hole size in bytes
>    *
> - * Returns: errno
>    */
> +static u64 hole_size(struct inode *inode, sector_t lblock, struct metapath *mp)
> +{
> +	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_sbd *sdp = GFS2_SB(inode);
> +	struct metapath mp_eof;
> +	unsigned int end_of_metadata = ip->i_height - 1;
> +	u64 factor = 1;
> +	int hgt = end_of_metadata;
> +	u64 holesz = 0, holestep;
> +	const __be64 *first, *end, *ptr;
> +	const struct buffer_head *bh;
> +	u64 isize_blks = (i_size_read(inode) - 1) >> inode->i_blkbits;
> +	int zeroptrs;
> +	bool done = false;
> +
> +	/* Get another metapath, to the very last byte */
> +	find_metapath(sdp, isize_blks, &mp_eof, ip->i_height);
> +	for (hgt = end_of_metadata; hgt >= 0 && !done; hgt--) {
> +		bh = mp->mp_bh[hgt];
> +		if (bh) {
> +			zeroptrs = 0;
> +			first = metapointer(hgt, mp);
> +			end = (const __be64 *)(bh->b_data + bh->b_size);
> +
> +			for (ptr = first; ptr < end; ptr++) {
> +				if (*ptr) {
> +					done = true;
> +					break;
> +				} else {
> +					zeroptrs++;
> +				}
> +			}
> +		} else {
> +			zeroptrs = sdp->sd_inptrs;
> +		}
> +		holestep = min(factor * zeroptrs,
> +			       isize_blks - (lblock + (zeroptrs * holesz)));
> +		holesz += holestep;
> +		if (lblock + holesz >= isize_blks) {
> +			holesz = isize_blks - lblock;
> +			break;
> +		}
>   
> -int gfs2_block_map(struct inode *inode, sector_t lblock,
> -		   struct buffer_head *bh_map, int create)
> +		factor *= sdp->sd_inptrs;
> +		if (hgt && (mp->mp_list[hgt - 1] < mp_eof.mp_list[hgt - 1]))
> +			(mp->mp_list[hgt - 1])++;
> +	}
> +	return holesz << inode->i_blkbits;
> +}
> +
> +/**
> + * gfs2_get_iomap - Map blocks from an inode to disk blocks
> + * @mapping: The address space
> + * @pos: Starting position in bytes
> + * @length: Length to map, in bytes
> + * @iomap: The iomap structure
> + * @mp: An uninitialized metapath structure
> + * @release_mpath: if true, we need to release the metapath when done
> + *
> + * Returns: errno
> + */
> +static int _gfs2_get_iomap(struct inode *inode, loff_t pos, ssize_t length,
> +			   struct iomap *iomap, struct metapath *mp,
> +			   bool release_mpath)
>   {
>   	struct gfs2_inode *ip = GFS2_I(inode);
>   	struct gfs2_sbd *sdp = GFS2_SB(inode);
>   	unsigned int bsize = sdp->sd_sb.sb_bsize;
> -	const size_t maxlen = bh_map->b_size >> inode->i_blkbits;
>   	const u64 *arr = sdp->sd_heightsize;
>   	__be64 *ptr;
> -	u64 size;
> -	struct metapath mp;
> +	sector_t lblock = pos >> sdp->sd_sb.sb_bsize_shift;
>   	int ret;
>   	int eob;
>   	unsigned int len;
>   	struct buffer_head *bh;
>   	u8 height;
> -	bool zero_new = false;
> -	sector_t dblock;
> -	unsigned int dblks = 0;
> -
> -	BUG_ON(maxlen == 0);
> -
> -	memset(mp.mp_bh, 0, sizeof(mp.mp_bh));
> -	bmap_lock(ip, create);
> -	clear_buffer_mapped(bh_map);
> -	clear_buffer_new(bh_map);
> -	clear_buffer_boundary(bh_map);
> -	trace_gfs2_bmap(ip, bh_map, lblock, create, 1);
> +	loff_t isize = i_size_read(inode);
> +
> +	if (length == 0)
> +		return -EINVAL;
> +
> +	iomap->offset = pos;
> +	iomap->blkno = IOMAP_NULL_BLOCK;
> +	iomap->type = IOMAP_HOLE;
> +	iomap->length = length;
> +	mp->mp_aheight = 1;
> +	memset(&mp->mp_bh, 0, sizeof(mp->mp_bh));
> +	bmap_lock(ip, 0);
>   	if (gfs2_is_dir(ip)) {
>   		bsize = sdp->sd_jbsize;
>   		arr = sdp->sd_jheightsize;
>   	}
>   
> -	ret = gfs2_meta_inode_buffer(ip, &mp.mp_bh[0]);
> +	ret = gfs2_meta_inode_buffer(ip, &(mp->mp_bh[0]));
>   	if (ret)
>   		goto out;
>   
>   	height = ip->i_height;
> -	size = (lblock + 1) * bsize;
> -	while (size > arr[height])
> +	while ((lblock + 1) * bsize > arr[height])
>   		height++;
> -	find_metapath(sdp, lblock, &mp, height);
> -	mp.mp_aheight = 1;
> +	find_metapath(sdp, lblock, mp, height);
>   	if (height > ip->i_height || gfs2_is_stuffed(ip))
> -		goto do_alloc;
> -	ret = lookup_metapath(ip, &mp);
> +		goto out;
> +
> +	ret = lookup_metapath(ip, mp);
>   	if (ret < 0)
>   		goto out;
> -	if (mp.mp_aheight != ip->i_height)
> -		goto do_alloc;
> -	ptr = metapointer(ip->i_height - 1, &mp);
> -	if (*ptr == 0)
> -		goto do_alloc;
> -	map_bh(bh_map, inode->i_sb, be64_to_cpu(*ptr));
> -	bh = mp.mp_bh[ip->i_height - 1];
> -	len = gfs2_extent_length(bh->b_data, bh->b_size, ptr, maxlen, &eob);
> -	bh_map->b_size = (len << inode->i_blkbits);
> -	if (eob)
> -		set_buffer_boundary(bh_map);
> +
>   	ret = 0;
> +	if (mp->mp_aheight != ip->i_height) {
> +		iomap->length = hole_size(inode, lblock, mp);
> +		goto out;
> +	}
> +
> +	ptr = metapointer(ip->i_height - 1, mp);
> +	if (*ptr) {
> +		iomap->type = IOMAP_MAPPED;
> +		iomap->blkno = be64_to_cpu(*ptr);
> +	}
> +
> +	bh = mp->mp_bh[ip->i_height - 1];
> +	len = gfs2_extent_length(bh->b_data, bh->b_size, ptr,
> +				 length >> inode->i_blkbits, &eob);
> +	iomap->length = len << sdp->sd_sb.sb_bsize_shift;
> +	/* If we go past eof, round up to the nearest block */
> +	if (iomap->offset + iomap->length >= isize)
> +		iomap->length = round_up((isize - iomap->offset), bsize);
> +
>   out:
> -	release_metapath(&mp);
> -	trace_gfs2_bmap(ip, bh_map, lblock, create, ret);
> -	bmap_unlock(ip, create);
> +	if (ret || release_mpath)
> +		release_metapath(mp);
> +	bmap_unlock(ip, 0);
>   	return ret;
> +}
> +
> +int gfs2_get_iomap(struct inode *inode, loff_t pos, ssize_t length,
> +		   struct iomap *iomap)
> +{
> +	struct metapath mp;
> +
> +	return _gfs2_get_iomap(inode, pos, length, iomap, &mp, true);
> +}
> +
> +/**
> + * gfs2_block_map - Map a block from an inode to a disk block
> + * @inode: The inode
> + * @lblock: The logical block number
> + * @bh_map: The bh to be mapped
> + * @create: True if its ok to alloc blocks to satify the request
> + *
> + * Sets buffer_mapped() if successful, sets buffer_boundary() if a
> + * read of metadata will be required before the next block can be
> + * mapped. Sets buffer_new() if new blocks were allocated.
> + *
> + * Returns: errno
> + */
>   
> -do_alloc:
> -	/* All allocations are done here, firstly check create flag */
> -	if (!create) {
> -		BUG_ON(gfs2_is_stuffed(ip));
> -		ret = 0;
> +int gfs2_block_map(struct inode *inode, sector_t lblock,
> +		   struct buffer_head *bh_map, int create)
> +{
> +	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_sbd *sdp = GFS2_SB(inode);
> +	const size_t maxlen = bh_map->b_size >> inode->i_blkbits;
> +	struct metapath mp;
> +	struct iomap iomap;
> +	sector_t dblock;
> +	unsigned int dblks;
> +	bool zero_new = false;
> +	int ret;
> +
> +	BUG_ON(maxlen == 0);
> +
> +	clear_buffer_mapped(bh_map);
> +	clear_buffer_new(bh_map);
> +	clear_buffer_boundary(bh_map);
> +	trace_gfs2_bmap(ip, bh_map, lblock, create, 1);
> +
> +	/* If we're asked to alloc any missing blocks, we need to preserve
> +	 * the metapath buffers and release them ourselves. */
> +	ret = _gfs2_get_iomap(inode, lblock << sdp->sd_sb.sb_bsize_shift,
> +			      bh_map->b_size, &iomap, &mp, false);
> +
> +	bh_map->b_size = iomap.length;
> +	if (maxlen >= iomap.length)
> +		set_buffer_boundary(bh_map);
> +
> +	if (ret)
>   		goto out;
> -	}
>   
> -	/* At this point ret is the tree depth of already allocated blocks */
> -	if (buffer_zeronew(bh_map))
> -		zero_new = true;
> -	ret = gfs2_bmap_alloc(inode, lblock, zero_new, &mp, maxlen, &dblock,
> -			      &dblks);
> -	if (ret == 0) {
> -		map_bh(bh_map, inode->i_sb, dblock);
> -		bh_map->b_size = dblks << inode->i_blkbits;
> -		set_buffer_new(bh_map);
So far, so good I think...

> +	if (iomap.type == IOMAP_MAPPED) {
> +		map_bh(bh_map, inode->i_sb, iomap.blkno);
> +		bh_map->b_size = iomap.length;
> +	} else if (create) {
> +		if (buffer_zeronew(bh_map))
> +			zero_new = true;
> +		ret = gfs2_bmap_alloc(inode, lblock, zero_new, &mp, maxlen,
> +				      &dblock, &dblks);
however gfs2_bmap_alloc() should take the struct iomap and not the 
individual parameters. Also why is this call to gfs2_bmap_alloc() done 
here rather than in gfs2_get_iomap() ? That way there is no need to pass 
the mp around. The zero_new flag should just become an iomap flag too, 
and that should clean this bit up.

I think that gfs2_block_map() should just be a wrapper for the new 
gfs2_get_iomap() function and not try to bypass it for the create case,

Steve.

> +		if (ret == 0) {
> +			map_bh(bh_map, inode->i_sb, dblock);
> +			bh_map->b_size = dblks << inode->i_blkbits;
> +			set_buffer_new(bh_map);
> +		}
>   	}
> -	goto out;
> +	release_metapath(&mp);
> +out:
> +	trace_gfs2_bmap(ip, bh_map, lblock, create, ret);
> +	return ret;
>   }
>   
>   /*
> diff --git a/fs/gfs2/bmap.h b/fs/gfs2/bmap.h
> index 81ded5e..8da2429 100644
> --- a/fs/gfs2/bmap.h
> +++ b/fs/gfs2/bmap.h
> @@ -10,6 +10,8 @@
>   #ifndef __BMAP_DOT_H__
>   #define __BMAP_DOT_H__
>   
> +#include <linux/iomap.h>
> +
>   #include "inode.h"
>   
>   struct inode;
> @@ -47,6 +49,8 @@ static inline void gfs2_write_calc_reserv(const struct gfs2_inode *ip,
>   extern int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page);
>   extern int gfs2_block_map(struct inode *inode, sector_t lblock,
>   			  struct buffer_head *bh, int create);
> +extern int gfs2_get_iomap(struct inode *inode, loff_t pos,
> +			  ssize_t length, struct iomap *iomap);
>   extern int gfs2_extent_map(struct inode *inode, u64 lblock, int *new,
>   			   u64 *dblock, unsigned *extlen);
>   extern int gfs2_setattr_size(struct inode *inode, u64 size);