* [PATCH] btrfs-progs: check if device supports trim
@ 2013-09-20 16:42 David Sterba
2013-09-23 15:08 ` Eric Sandeen
0 siblings, 1 reply; 4+ messages in thread
From: David Sterba @ 2013-09-20 16:42 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs-progs: check if device supports trim
2013-09-20 16:42 [PATCH] btrfs-progs: check if device supports trim David Sterba
@ 2013-09-23 15:08 ` Eric Sandeen
2013-09-23 15:44 ` David Sterba
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2013-09-23 15:08 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
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);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs-progs: check if device supports trim
2013-09-23 15:08 ` Eric Sandeen
@ 2013-09-23 15:44 ` David Sterba
2013-09-23 15:46 ` Eric Sandeen
0 siblings, 1 reply; 4+ messages in thread
From: David Sterba @ 2013-09-23 15:44 UTC (permalink / raw)
To: Eric Sandeen; +Cc: David Sterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs-progs: check if device supports trim
2013-09-23 15:44 ` David Sterba
@ 2013-09-23 15:46 ` Eric Sandeen
0 siblings, 0 replies; 4+ messages in thread
From: Eric Sandeen @ 2013-09-23 15:46 UTC (permalink / raw)
To: dsterba, linux-btrfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-23 15:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-20 16:42 [PATCH] btrfs-progs: check if device supports trim David Sterba
2013-09-23 15:08 ` Eric Sandeen
2013-09-23 15:44 ` David Sterba
2013-09-23 15:46 ` Eric Sandeen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.