Re: [PATCH 3/4] rbd: simplify rbd_merge_bvec()

[Dan Mick <dan.mick@inktank.com>]

'segment' is better than 'chunk', but as these are RADOS objects, I 
really prefer just "object" here to make that clear.  Maybe we can add
a small block comment explaining the terminology just to make it 
crystal-clear?

rbd_object would be fine with me too.  The problem with "segment" is it 
tends to make me think I'm missing a subdivision of the RADOS objects 
that make up an rbd image that I didn't know about.

Otherwise,

Reviewed-by: Dan Mick <dan.mick@inktank.com>

On 10/22/2012 09:51 AM, Alex Elder wrote:
> The aim of this patch is to make what's going on rbd_merge_bvec() a
> bit more obvious than it was before.  This was an issue when a
> recent btrfs bug led us to question whether the merge function was
> working correctly.
>
> Use "seg" rather than "chunk" to indicate the units whose boundaries
> we care about we call "segments."
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   51
> +++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4858d92..4ccce4d 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1566,22 +1566,41 @@ static int rbd_merge_bvec(struct request_queue
> *q, struct bvec_merge_data *bmd,
>   			  struct bio_vec *bvec)
>   {
>   	struct rbd_device *rbd_dev = q->queuedata;
> -	unsigned int chunk_sectors;
> -	sector_t sector;
> -	unsigned int bio_sectors;
> -	int max;
> -
> -	chunk_sectors = 1 << (rbd_dev->header.obj_order - SECTOR_SHIFT);
> -	sector = bmd->bi_sector + get_start_sect(bmd->bi_bdev);
> -	bio_sectors = bmd->bi_size >> SECTOR_SHIFT;
> -
> -	max =  (chunk_sectors - ((sector & (chunk_sectors - 1))
> -				 + bio_sectors)) << SECTOR_SHIFT;
> -	if (max < 0)
> -		max = 0; /* bio_add cannot handle a negative return */
> -	if (max <= bvec->bv_len && bio_sectors == 0)
> -		return bvec->bv_len;
> -	return max;
> +	sector_t sector_offset;
> +	sector_t sectors_per_seg;
> +	sector_t seg_sector_offset;
> +	int ret;
> +
> +	/*
> +	 * Find how far into its rbd segment the partition-relative
> +	 * bio start sector is to offset relative to the enclosing
> +	 * device.
> +	 */
> +	sector_offset = get_start_sect(bmd->bi_bdev) + bmd->bi_sector;
> +	sectors_per_seg = 1 << (rbd_dev->header.obj_order - SECTOR_SHIFT);
> +	seg_sector_offset = sector_offset & (sectors_per_seg - 1);
> +
> +	/*
> +	 * Compute the number of bytes from that offset to the end
> +	 * of the segment.  Account for what's already used by the bio.
> +	 */
> +	ret = (int) (sectors_per_seg - seg_sector_offset) << SECTOR_SHIFT;
> +	if (ret >= bmd->bi_size)
> +		ret -= bmd->bi_size;
> +	else
> +		ret = 0;
> +
> +	/*
> +	 * Don't send back more than was asked for.  And if the bio
> +	 * was empty, let the whole thing through because:  "Note
> +	 * that a block device *must* allow a single page to be
> +	 * added to an empty bio."
> +	 */
> +	rbd_assert(bvec->bv_len <= PAGE_SIZE);
> +	if (ret > (int) bvec->bv_len || !bmd->bi_size)
> +		ret = (int) bvec->bv_len;
> +
> +	return ret;
>   }
>
>   static void rbd_free_disk(struct rbd_device *rbd_dev)
>