Re: [PATCH] loop: add discard support for loop devices

[Jens Axboe <jaxboe@fusionio.com>]

On 2011-08-18 21:23, Jeff Moyer wrote:
> Jens Axboe <jaxboe@fusionio.com> writes:
> 
>> On 2011-08-18 21:08, Lukas Czerner wrote:
>>> On Thu, 18 Aug 2011, Milan Broz wrote:
>>>
>>>> On 08/18/2011 05:49 PM, Lukas Czerner wrote:
>>>>> On Thu, 18 Aug 2011, Jeff Moyer wrote:
>>>>>> Seems you missed the bizarre case of configuring a loop device over top
>>>>>> of a block device.
>>>>>
>>>>> Wow, that is a bizarre case I did not think about at all. But since it
>>>>> is so bizarre, do we even care ? The thing is that the only case where
>>>>> it would make a difference is if the loop device is put on top of block
>>>>> device which actually supports discard.
>>>>>
>>>>> In order to fix that I would need to dig out the actual limits for that
>>>>> device and set that appropriately for the loop device. Is that worth it
>>>>> ? It is not like someone will ever do that (or should) :).
>>>>
>>>> It is bizarre (and being device-mapper developer I surely know better way :-)
>>>> but people are still using that.
>>>>
>>>> Historically one of the use of underlying block device was cryptoloop, but here
>>>> I think it should be completely deprecated (cryptsetup can handle all old loop
>>>> modes as well and default modes for cryptoloop are not safe).
>>>> [Can we finally remove crypto loop option it from kernel? ... ok, just tried:)]
>>>>
>>>> There is also out of tree loop-aes based on heavily patched loop device
>>>> which usually uses block device underneath
>>>> (cryptsetup already can handle all loop-aes modes as well).
>>>>
>>>> Sometimes it is used with --offset parameter for some reason
>>>> (like linear device-mapper mapping).
>>>>
>>>> So I do not care if you do not support discard here but please do not break
>>>> support for block device mapped through loop.
>>>
>>> I do not think that this is the case with my patch. Also, as you know using
>>> discard on encrypted device is not a good idea.
>>
>> It's not a bizarre use case at all, so would be nice to support like we
>> support anything else over a bdev as well. Your patch should not break
>> it, so looks fine.
> 
> C'mon... it's bizarre!  Loopback mounting a block device, to make it
> look like a block device?  Anyway...

I take it you've never done that, I have :-). The --offset thing is kind
of handy. Besides, loop on bdev has been in existance since forever. So
I'd consider it core functionality of the driver, even if it's usually
used on a file.

>> Shall we queue it up for 3.2? It's a good way to beat on fs discard
>> support, fio could be easily configured for that.
> 
> Fine by me.
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Added, thanks.

-- 
Jens Axboe