Re: [PATCH 1/4] Check that the target supports discard

[Mike Snitzer <snitzer@redhat.com>]

On Fri, Jul 02 2010 at 11:17am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Check that the target supports discard
> 
> Because of a various race conditions, discard request may be submitted to
> device mapper even if device mapper advertises that it doesn't support
> discards. (for example, the device is linear, dm advertises discard support,
> discard request is constructed, the table is reloaded with mirror, the discard
> is submitted to the mirror target that doesn't support it).
> 
> To solve these problems we must check that the target supports discard
> just before submitting discard request to it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6.35-rc3-fast/drivers/md/dm.c
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/drivers/md/dm.c	2010-07-02 16:10:25.000000000 +0200
> +++ linux-2.6.35-rc3-fast/drivers/md/dm.c	2010-07-02 16:11:22.000000000 +0200
> @@ -1223,6 +1223,9 @@ static int __clone_and_map_discard(struc
>  	if (!dm_target_is_valid(ti))
>  		return -EIO;
>  
> +	if (!(ti->type->features & DM_TARGET_SUPPORTS_DISCARDS))
> +		return -EOPNOTSUPP;
> +
>  	max = max_io_len(ci->md, ci->sector, ti);
>  
>  	if (ci->sector_count > max)
> 

The current model is that the static DM_TARGET_SUPPORTS_DISCARDS is
consulted when the target is added to table.  And then at the end of the
table load we check all devices to see if they actually do support
discards.

That said, I understand that IOs in flight before a table is suspended
will be attempted after the table is reload.  So this patch is good.

It should be noted that checking for this race condition gets much
harder once DM supports splitting discards that span devices.  We'll
have potential for partial completion that in the end returns
-EOPNOTSUPP.  Which is fine; but I think we'll need to be careful to not
return -EOPNOTSUPP before outstanding IO (associated with the discard
we're intending to return -EOPNOTSUPP for) completes?

Anyway, we'll cross that bridge later.  Nice catch.

Acked-by: Mike Snitzer <snitzer@redhat.com>