Re: [PATCH 2/7] Btrfs: separate DISCARD from __btrfs_map_block

[Liu Bo <bo.li.liu@oracle.com>]

On Mon, Feb 20, 2017 at 11:54:31AM +0800, Qu Wenruo wrote:
> 
> 
> At 02/18/2017 09:28 AM, Liu Bo wrote:
> > Since DISCARD is not as important as an operation like write, we don't
> > copy it to target device during replace, and it makes __btrfs_map_block
> > less complex.
> 
> Makes sense to me.
> 
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/volumes.c | 306 +++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 192 insertions(+), 114 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index c52b0fe..96228f3 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -5294,6 +5294,175 @@ void btrfs_put_bbio(struct btrfs_bio *bbio)
> >  		kfree(bbio);
> >  }
> > 
> > +/* can REQ_OP_DISCARD be sent with other REQ like REQ_OP_WRITE? */
> > +/*
> > + * Please note that, discard won't be sent to target device of device
> > + * replace.
> > + */
> > +static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
> > +					 u64 logical, u64 length,
> > +					 struct btrfs_bio **bbio_ret)
> > +{
> > +	struct extent_map_tree *em_tree = &fs_info->mapping_tree.map_tree;
> > +	struct extent_map *em;
> > +	struct map_lookup *map;
> > +	struct btrfs_bio *bbio;
> > +	u64 offset;
> > +	u64 stripe_nr;
> > +	u64 stripe_nr_end;
> > +	u64 stripe_end_offset;
> > +	u64 stripe_cnt;
> > +	u64 stripe_len;
> > +	u64 stripe_offset;
> > +	u64 num_stripes;
> > +	u32 stripe_index;
> > +	u32 factor = 0;
> > +	u32 sub_stripes = 0;
> > +	u64 stripes_per_dev = 0;
> > +	u32 remaining_stripes = 0;
> > +	u32 last_stripe = 0;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	/* discard always return a bbio */
> > +	ASSERT(bbio_ret);
> > +
> > +	read_lock(&em_tree->lock);
> > +	em = lookup_extent_mapping(em_tree, logical, length);
> > +	read_unlock(&em_tree->lock);
> 
> It seems that get_chunk_map() in previous patch can replace such searching
> and error message.
>

Yeah, I forgot to update with it.

> > +
> > +	if (!em) {
> > +		btrfs_crit(fs_info, "unable to find logical %llu len %llu",
> > +			logical, length);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (em->start > logical || em->start + em->len < logical) {
> > +		btrfs_crit(fs_info,
> > +			   "found a bad mapping, wanted %Lu, found %Lu-%Lu",
> > +			   logical, em->start, em->start + em->len);
> > +		free_extent_map(em);
> > +		return -EINVAL;
> > +	}
> > +
> > +	map = em->map_lookup;
> > +	/* we don't discard raid56 yet */
> > +	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	offset = logical - em->start;
> > +	length = min_t(u64, em->len - offset, length);
> > +
> > +	stripe_len = map->stripe_len;
> > +	/*
> > +	 * stripe_nr counts the total number of stripes we have to stride
> > +	 * to get to this block
> > +	 */
> > +	stripe_nr = div64_u64(offset, stripe_len);
> > +	stripe_offset = stripe_nr * stripe_len;
> > +	ASSERT(offset >= stripe_offset);
> 
> What about a DIV_ROUND_DOWN helper?
> Surprisingly we only have DIR_ROUND_UP, not not DIV_ROUND_DOWN.
> 
> And if we're only going to support 64K stripe len, then round_down() is good
> for current usage.
> 
> > +
> > +	/* stripe_offset is the offset of this block in its stripe */
> > +	stripe_offset = offset - stripe_offset;
> 
> This is a little confusing.
> What about using another variable called @stripe_start instead of using the
> same variable @stripe_offset to temporarily store stripe start bytenr.
> 
> I prefer to do it in one run without resuing @stripe_offset variable to
> avoid confusion.

Right, I was trying to keep the check of (offset >= stripe_offset), but it's not
necessary.

> 
> > +
> > +	stripe_nr_end = ALIGN(offset + length, map->stripe_len);
> 
> round_up() causes less confusion.
> 
> And IIRC, ALIGN/round_up can only handle power of 2, this implies the
> stripe_len must be power of 2, which is OK for now.
> If using ALIGN here, we can also use round_down() in previous stripe_nr.
>

Good point.

Thanks,

-liubo