Re: [PATCH 2/3] dm-thin: fix discard support

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Jul 16, 2012 at 02:35:18PM -0400, Mikulas Patocka wrote:
> dm-thin: fix discard support
> 
> There is a bug in dm_thin regarding processing discards.
> When dm-thin receives a discard request with size equal to block size
> that is not aligned on block size boundary, io_overlaps_block returns
> true, process_discard treats this discard as a full block discard,
  ^^^^
> deletes the full block - the result is that some data that shouldn't be
> discarded are discarded.

Looking at io_overlaps_block(), it looks like it will return false (and
not true) for bios which are not aligned to block size boundary.

static int io_overlaps_block(struct pool *pool, struct bio *bio)
{
        return !(bio->bi_sector & pool->offset_mask) &&
                (bio->bi_size == (pool->sectors_per_block << SECTOR_SHIFT));

}

Hence for block which crosses block size boundary, we should be sending
discard down for partial block as per the current code and no harm should
be done?


> 
> This patch sets the variable "ti->split_discard_requests", so that
> device mapper core splits discard requests on a block boundary.
> 
> Consequently, a discard request that spans multiple blocks is never sent
> to dm-thin. The patch also removes some code in process_discard that
> deals with discards that span multiple blocks.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-thin.c |   18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
> ===================================================================
> --- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c	2012-07-16 18:46:18.000000000 +0200
> +++ linux-3.5-rc6-fast/drivers/md/dm-thin.c	2012-07-16 20:07:19.000000000 +0200
> @@ -1246,17 +1246,10 @@ static void process_discard(struct thin_
>  			}
>  		} else {
>  			/*
> -			 * This path is hit if people are ignoring
> -			 * limits->discard_granularity.  It ignores any
> -			 * part of the discard that is in a subsequent
> -			 * block.
> +			 * The dm makes sure that the discard doesn't span
> +			 * a block boundary. So we submit the discard
> +			 * to the appropriate block.
>  			 */
> -			sector_t offset = pool->sectors_per_block_shift >= 0 ?
> -			      bio->bi_sector & (pool->sectors_per_block - 1) :
> -			      bio->bi_sector - block * pool->sectors_per_block;
> -			unsigned remaining = (pool->sectors_per_block - offset) << SECTOR_SHIFT;
> -			bio->bi_size = min(bio->bi_size, remaining);
> -

So previous code will also send down partial block discard and this code
will also send down partial discard. So nothing has changed from
functionality point of view?

Thanks
Vivek