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

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

[Lukas Czerner]

On Thu, 11 Aug 2011, Lukas Czerner wrote:

> This commit adds discard support for loop devices. Discard is usually
> supported by SSD and thinly provisioned devices as a method for
> reclaiming unused space. This is no different than trying to reclaim
> back space which is not used by the file system on the image, but it
> still occupies space on the host file system.
> 
> We can do the reclamation on file system which does support hole
> punching. So when discard request gets to the loop driver we can
> translate that to punch a hole to the underlying file, hence reclaim
> the free space.
> 
> This is very useful for trimming down the size of the image to only what
> is really used by the file system on that image. Fstrim may be used for
> that purpose.
> 
> It has been tested on ext4, xfs and btrfs with the image file systems
> ext4, ext3, xfs and btrfs. ext4, or ext6 image on ext4 file system has
> some problems but it seems that ext4 punch hole implementation is
> somewhat flawed and it is unrelated to this commit.
> 
> Also this is a very good method of validating file systems punch hole
> implementation.
> 
> Note that when encryption is used, discard support is disabled, because
> using it might leak some information useful for possible attacker.

Adding linux-fsdevel@vger.kernel.org cc.

> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> CC: Jens Axboe <jaxboe@fusionio.com>
> ---
>  drivers/block/loop.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 54 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 76c8da7..a6d6873 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -75,6 +75,7 @@
>  #include <linux/kthread.h>
>  #include <linux/splice.h>
>  #include <linux/sysfs.h>
> +#include <linux/falloc.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -484,6 +485,29 @@ static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
>  			}
>  		}
>  
> +		/*
> +		 * We use punch hole to reclaim the free space used by the
> +		 * image a.k.a. discard. However we do support discard if
> +		 * encryption is enabled, because it may give an attacker
> +		 * useful information.
> +		 */
> +		if (bio->bi_rw & REQ_DISCARD) {
> +			struct file *file = lo->lo_backing_file;
> +			int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
> +
> +			if ((!file->f_op->fallocate) ||
> +			    lo->lo_encrypt_key_size) {
> +				ret = -EOPNOTSUPP;
> +				goto out;
> +			}
> +			ret = file->f_op->fallocate(file, mode, pos,
> +						    bio->bi_size);
> +			if (unlikely(ret && ret != -EINVAL &&
> +				     ret != -EOPNOTSUPP))
> +				ret = -EIO;
> +			goto out;
> +		}
> +
>  		ret = lo_send(lo, bio, pos);
>  
>  		if ((bio->bi_rw & REQ_FUA) && !ret) {
> @@ -814,6 +838,35 @@ static void loop_sysfs_exit(struct loop_device *lo)
>  			   &loop_attribute_group);
>  }
>  
> +static void loop_config_discard(struct loop_device *lo)
> +{
> +	struct file *file = lo->lo_backing_file;
> +	struct inode *inode = file->f_mapping->host;
> +	struct request_queue *q = lo->lo_queue;
> +
> +	/*
> +	 * We use punch hole to reclaim the free space used by the
> +	 * image a.k.a. discard. However we do support discard if
> +	 * encryption is enabled, because it may give an attacker
> +	 * useful information.
> +	 */
> +	if ((!file->f_op->fallocate) ||
> +	    lo->lo_encrypt_key_size) {
> +		q->limits.discard_granularity = 0;
> +		q->limits.discard_alignment = 0;
> +		q->limits.max_discard_sectors = 0;
> +		q->limits.discard_zeroes_data = 0;
> +		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, q);
> +		return;
> +	}
> +
> +	q->limits.discard_granularity = inode->i_sb->s_blocksize;
> +	q->limits.discard_alignment = inode->i_sb->s_blocksize;
> +	q->limits.max_discard_sectors = UINT_MAX >> 9;
> +	q->limits.discard_zeroes_data = 1;
> +	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
> +}
> +
>  static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  		       struct block_device *bdev, unsigned int arg)
>  {
> @@ -1113,6 +1166,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
>  		       info->lo_encrypt_key_size);
>  		lo->lo_key_owner = uid;
>  	}	
> +	loop_config_discard(lo);
>  
>  	return 0;
>  }
> 

-- 

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

[Jeff Moyer]

Lukas Czerner <lczerner@redhat.com> writes:

>> @@ -484,6 +485,29 @@ static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
>>  			}
>>  		}
>>  
>> +		/*
>> +		 * We use punch hole to reclaim the free space used by the
>> +		 * image a.k.a. discard. However we do support discard if
>> +		 * encryption is enabled, because it may give an attacker
>> +		 * useful information.
>> +		 */
>> +		if (bio->bi_rw & REQ_DISCARD) {
>> +			struct file *file = lo->lo_backing_file;
>> +			int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
>> +
>> +			if ((!file->f_op->fallocate) ||
>> +			    lo->lo_encrypt_key_size) {
>> +				ret = -EOPNOTSUPP;
>> +				goto out;
>> +			}
>> +			ret = file->f_op->fallocate(file, mode, pos,
>> +						    bio->bi_size);
>> +			if (unlikely(ret && ret != -EINVAL &&
>> +				     ret != -EOPNOTSUPP))
>> +				ret = -EIO;
>> +			goto out;
>> +		}
>> +

Seems you missed the bizarre case of configuring a loop device over top
of a block device.

Cheers,
Jeff

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

[Lukas Czerner]

On Thu, 18 Aug 2011, Jeff Moyer wrote:

> Lukas Czerner <lczerner@redhat.com> writes:
> 
> >> @@ -484,6 +485,29 @@ static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
> >>  			}
> >>  		}
> >>  
> >> +		/*
> >> +		 * We use punch hole to reclaim the free space used by the
> >> +		 * image a.k.a. discard. However we do support discard if
> >> +		 * encryption is enabled, because it may give an attacker
> >> +		 * useful information.
> >> +		 */
> >> +		if (bio->bi_rw & REQ_DISCARD) {
> >> +			struct file *file = lo->lo_backing_file;
> >> +			int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
> >> +
> >> +			if ((!file->f_op->fallocate) ||
> >> +			    lo->lo_encrypt_key_size) {
> >> +				ret = -EOPNOTSUPP;
> >> +				goto out;
> >> +			}
> >> +			ret = file->f_op->fallocate(file, mode, pos,
> >> +						    bio->bi_size);
> >> +			if (unlikely(ret && ret != -EINVAL &&
> >> +				     ret != -EOPNOTSUPP))
> >> +				ret = -EIO;
> >> +			goto out;
> >> +		}
> >> +
> 
> 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) :).

Thanks!
-Lukas

> 
> Cheers,
> Jeff
> 

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

[Milan Broz]

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.

Thanks,
Milan

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

[Lukas Czerner]

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.

Thanks!
-Lukas

> 
> Thanks,
> Milan
> 

-- 

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

[Jens Axboe]

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.

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.

-- 
Jens Axboe

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

[Lukas Czerner]

On Thu, 18 Aug 2011, Jens Axboe wrote:

> 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.
> 
> 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.
> 

That would be great, thanks!

Btw I am not sure what do you mean by "beat on fs discard support" :).

-Lukas

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

[Jens Axboe]

On 2011-08-18 21:20, Lukas Czerner wrote:
> On Thu, 18 Aug 2011, Jens Axboe wrote:
> 
>> 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.
>>
>> 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.
>>
> 
> That would be great, thanks!

Alright, lets do that.

> Btw I am not sure what do you mean by "beat on fs discard support" :).

Perhaps worded a bit weird, what I mean was "thoroughly exercise and
test file system discard support" :-)

-- 
Jens Axboe

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

[Jeff Moyer]

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...

> 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>

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

[Jens Axboe]

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