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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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); >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html