[PATCH] btrfs-progs: check if device supports trim

[PATCH] btrfs-progs: check if device supports trim

[David Sterba]

The message about trim was printed unconditionally, we should check if
trim is supported at all.

Signed-off-by: David Sterba <dsterba@suse.cz>
---
 utils.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/utils.c b/utils.c
index 5fa193b..6c74654 100644
--- a/utils.c
+++ b/utils.c
@@ -597,13 +597,16 @@ int btrfs_prepare_device(int fd, char *file, int zero_end, u64 *block_count_ret,
 	}
 
 	if (discard) {
-		fprintf(stderr, "Performing full device TRIM (%s) ...\n",
-				pretty_size(block_count));
 		/*
-		 * We intentionally ignore errors from the discard ioctl.  It is
-		 * not necessary for the mkfs functionality but just an optimization.
+		 * We intentionally ignore errors from the discard ioctl.  It
+		 * is not necessary for the mkfs functionality but just an
+		 * optimization.
 		 */
-		discard_blocks(fd, 0, block_count);
+		if (discard_blocks(fd, 0, 0) == 0) {
+			fprintf(stderr, "Performing full device TRIM (%s) ...\n",
+				pretty_size(block_count));
+			discard_blocks(fd, 0, block_count);
+		}
 	}
 
 	ret = zero_dev_start(fd);
-- 
1.8.3.1

Re: [PATCH] btrfs-progs: check if device supports trim

[Eric Sandeen]

On 9/20/13 11:42 AM, David Sterba wrote:
> The message about trim was printed unconditionally, we should check if
> trim is supported at all.

Good idea, but I wonder if there's any risk that discard(0,0) will ever
be optimized away on the kernel side & pass unconditionally?

I was thinking we could get this from blkid, but maybe not.

In the meantime it does do the right thing, so:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
>  utils.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/utils.c b/utils.c
> index 5fa193b..6c74654 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -597,13 +597,16 @@ int btrfs_prepare_device(int fd, char *file, int zero_end, u64 *block_count_ret,
>  	}
>  
>  	if (discard) {
> -		fprintf(stderr, "Performing full device TRIM (%s) ...\n",
> -				pretty_size(block_count));
>  		/*
> -		 * We intentionally ignore errors from the discard ioctl.  It is
> -		 * not necessary for the mkfs functionality but just an optimization.
> +		 * We intentionally ignore errors from the discard ioctl.  It
> +		 * is not necessary for the mkfs functionality but just an
> +		 * optimization.
>  		 */
> -		discard_blocks(fd, 0, block_count);
> +		if (discard_blocks(fd, 0, 0) == 0) {
> +			fprintf(stderr, "Performing full device TRIM (%s) ...\n",
> +				pretty_size(block_count));
> +			discard_blocks(fd, 0, block_count);
> +		}
>  	}
>  
>  	ret = zero_dev_start(fd);
> 

Re: [PATCH] btrfs-progs: check if device supports trim

[David Sterba]

On Mon, Sep 23, 2013 at 10:08:08AM -0500, Eric Sandeen wrote:
> On 9/20/13 11:42 AM, David Sterba wrote:
> > The message about trim was printed unconditionally, we should check if
> > trim is supported at all.
> 
> Good idea, but I wonder if there's any risk that discard(0,0) will ever
> be optimized away on the kernel side & pass unconditionally?

I hope the checks in blkdev_issue_discard() stay in the order as of now:

 40 int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 41                 sector_t nr_sects, gfp_t gfp_mask, unsigned long flags)
 42 {
...
 52
 53         if (!q)
 54                 return -ENXIO;
 55
 56         if (!blk_queue_discard(q))
 57                 return -EOPNOTSUPP;

here it returns no matter what the arguments are, setting length to 0 is
just cautious.

 59         /* Zero-sector (unknown) and one-sector granularities are the same.  */
 60         granularity = max(q->limits.discard_granularity >> 9, 1U);
 61         alignment = bdev_discard_alignment(bdev) >> 9;
 62         alignment = sector_div(alignment, granularity);
 63

> I was thinking we could get this from blkid, but maybe not.

Possibly yes, with other information like rotational etc.

Alternatively,

/sys/block/sdx/queue/dicard_granularity > 0 means that the device
supports discard, but that's imo even more fragile than the direct
call to discard.

Re: [PATCH] btrfs-progs: check if device supports trim

[Eric Sandeen]

On 9/23/13 10:44 AM, David Sterba wrote:
> On Mon, Sep 23, 2013 at 10:08:08AM -0500, Eric Sandeen wrote:
>> On 9/20/13 11:42 AM, David Sterba wrote:
>>> The message about trim was printed unconditionally, we should check if
>>> trim is supported at all.
>>
>> Good idea, but I wonder if there's any risk that discard(0,0) will ever
>> be optimized away on the kernel side & pass unconditionally?
> 
> I hope the checks in blkdev_issue_discard() stay in the order as of now:
> 
>  40 int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  41                 sector_t nr_sects, gfp_t gfp_mask, unsigned long flags)
>  42 {
> ...
>  52
>  53         if (!q)
>  54                 return -ENXIO;
>  55
>  56         if (!blk_queue_discard(q))
>  57                 return -EOPNOTSUPP;
> 
> here it returns no matter what the arguments are, setting length to 0 is
> just cautious.
> 
>  59         /* Zero-sector (unknown) and one-sector granularities are the same.  */
>  60         granularity = max(q->limits.discard_granularity >> 9, 1U);
>  61         alignment = bdev_discard_alignment(bdev) >> 9;
>  62         alignment = sector_div(alignment, granularity);
>  63
> 
>> I was thinking we could get this from blkid, but maybe not.
> 
> Possibly yes, with other information like rotational etc.
> 
> Alternatively,
> 
> /sys/block/sdx/queue/dicard_granularity > 0 means that the device
> supports discard, but that's imo even more fragile than the direct
> call to discard.

Perhaps; and I don't think libblkid gives us easy access to that anyway,
at least I didn't see it on a quick look.

So yeah, I think it's fine as you sent it; it doesn't actually change
behavior anyway other than the printf.

Thanks,
-Eric