[PATCH 0/6 v2] e2fsprogs: Using discard in e2fsprogs tools

[PATCH 0/6 v2] e2fsprogs: Using discard in e2fsprogs tools

[Lukas Czerner]

Hi all,

We have came to consensus about using discard in e2fsprogs tools. These
patches generalize use of discard in e2fsprogs tools introduce changes
in mke2fs and e2fsck.

Short summary
-------------

 [PATCH 1/6] e2fsprogs: Add discard function into struct_io_manager
   * generalize use of discard in e2fsprogs and let any tool in e2fsprogs
     take advantage of it withou need to write its own BLKDISCARD wrappers.

 [PATCH 2/6] e2fsprogs: Add CHANNEL_FLAGS_DISCARD_ZEROES flag for io_manager
   * Give the opportunity for any io_manager to check if device discard
     support zeroes data and save the results into io_channel for use in
     any e2fsprofs tool.

 [PATCH 3/6] e2fsck: Discard free data and inode blocks.
   * In pass 5 after the group descriptors has been changed discard free
     data and inode blocks. The consensus was that it should be OFF by
     default, so it is.
   * Introduce new paid of extended options discard/nodiscard.

 [PATCH 4/6] mke2fs: Deprecate -K option, introduce discard/nodiscard
   * To the same of consistency and to gain ability to easily default it
     the new pair of extended options has been added.
   * The consensus was that it should stay ON by default, so it is.

 [PATCH 5/6] mke2fs: Use unix_discard() for discards

 [PATCH 6/6] mke2fs: Add discard option into mke2fs.conf

Any comments appreciated!

Thanks!

-Lukas

---
[PATCH 1/6] e2fsprogs: Add discard function into struct_io_manager
[PATCH 2/6] e2fsprogs: Add CHANNEL_FLAGS_DISCARD_ZEROES flag for io_manager
[PATCH 3/6] e2fsck: Discard free data and inode blocks.
[PATCH 4/6] mke2fs: Deprecate -K option, introduce discard/nodiscard
[PATCH 5/6] mke2fs: Use unix_discard() for discards
[PATCH 6/6] mke2fs: Add discard option into mke2fs.conf

 e2fsck/e2fsck.8.in    |   14 +++++++
 e2fsck/e2fsck.h       |    1 +
 e2fsck/pass5.c        |   96 ++++++++++++++++++++++++++++++++++++++++++++++++
 e2fsck/unix.c         |   10 ++++-
 lib/ext2fs/ext2_io.h  |    5 ++
 lib/ext2fs/unix_io.c  |   45 ++++++++++++++++++++++-
 misc/mke2fs.8.in      |   18 +++++---
 misc/mke2fs.c         |   98 ++++++++++++++-----------------------------------
 misc/mke2fs.conf.5.in |    5 ++
 9 files changed, 213 insertions(+), 79 deletions(-)

[PATCH 1/6] e2fsprogs: Add discard function into struct_io_manager

[Lukas Czerner]

In order to provide generic "discard" function for all e2fsprogs tools
add a discard function prototype into struct_io_manager. Specific
function for specific io managers can be crated that way.

This commit also creates unix_discard function which uses BLKDISCARD
ioctl to discard data blocks on the block device and bind it into
unit_io_manager structure to be available for all e2fsprogs tools.
Note that BLKDISCARD is still Linux specific ioctl, however other
unix systems may provide similar functionality. So far the
unix_discard() remains linux specific hence is embedded in #ifdef
__linux__ macro.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/ext2_io.h |    2 ++
 lib/ext2fs/unix_io.c |   31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
index ccc9c8b..c8ec8e5 100644
--- a/lib/ext2fs/ext2_io.h
+++ b/lib/ext2fs/ext2_io.h
@@ -83,6 +83,8 @@ struct struct_io_manager {
 					int count, void *data);
 	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
 					int count, const void *data);
+	errcode_t (*discard)(io_channel channel, unsigned long long block,
+			     unsigned long long count, unsigned int blocksize);
 	long	reserved[16];
 };
 
diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index 1df1fdd..991de50 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
 			       int count, void *data);
 static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
 				int count, const void *data);
+static errcode_t unix_discard(io_channel channel, unsigned long long block,
+			      unsigned long long count, unsigned int blocksize);
 
 static struct struct_io_manager struct_unix_manager = {
 	EXT2_ET_MAGIC_IO_MANAGER,
@@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
 	unix_get_stats,
 	unix_read_blk64,
 	unix_write_blk64,
+	unix_discard,
 };
 
 io_manager unix_io_manager = &struct_unix_manager;
@@ -834,3 +837,31 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
 	}
 	return EXT2_ET_INVALID_ARGUMENT;
 }
+
+#ifdef __linux__
+
+#ifndef BLKDISCARD
+#define BLKDISCARD	_IO(0x12,119)
+#endif
+
+static errcode_t unix_discard(io_channel channel, unsigned long long block,
+			     unsigned long long count, unsigned int blocksize)
+{
+	struct unix_private_data *data;
+	__uint64_t	range[2];
+
+	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
+	data = (struct unix_private_data *) channel->private_data;
+	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
+
+	range[0] = (__uint64_t)(block);
+	range[1] = (__uint64_t)(count);
+	range[0] *= (__uint64_t)(blocksize);
+	range[1] *= (__uint64_t)(blocksize);
+
+	return ioctl(data->dev, BLKDISCARD, &range);
+}
+
+#else
+#define unix_discard(channel, block, count, blocksize)	EXT2_ET_UNIMPLEMENTED
+#endif
-- 
1.7.2.3

[PATCH 2/6] e2fsprogs: Add CHANNEL_FLAGS_DISCARD_ZEROES flag for io_manager

[Lukas Czerner]

When the device have discard support and simultaneously discard zeroes
data (and it is properly advertised), then we can take advantage of such
behavior in several e2fsprogs tools.

Add new flag CHANNEL_FLAGS_DISCARD_ZEROES for struct_io_channel so
each io_manager can take advantage of this. The flag is properly set
according to BLKDISCARDZEROES ioctl in unix_open.

Also remove old mke2fs_discard_zeroes_data() function and substitute it
with helper which test this flag.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/ext2_io.h |    3 +++
 lib/ext2fs/unix_io.c |   14 +++++++++++++-
 misc/mke2fs.c        |   23 +----------------------
 3 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
index c8ec8e5..ec5d2d1 100644
--- a/lib/ext2fs/ext2_io.h
+++ b/lib/ext2fs/ext2_io.h
@@ -29,6 +29,9 @@ typedef struct struct_io_channel *io_channel;
 typedef struct struct_io_stats *io_stats;
 
 #define CHANNEL_FLAGS_WRITETHROUGH	0x01
+#define CHANNEL_FLAGS_DISCARD_ZEROES	0x02
+
+#define ext2fs_discard_zeroes_data(i) (i->flags & CHANNEL_FLAGS_DISCARD_ZEROES)
 
 struct struct_io_channel {
 	errcode_t	magic;
diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index 991de50..703df86 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -425,12 +425,18 @@ static errcode_t flush_cached_blocks(io_channel channel,
 }
 #endif /* NO_IO_CACHE */
 
+#ifdef __linux__
+#ifndef BLKDISCARDZEROES
+#define BLKDISCARDZEROES _IO(0x12,124)
+#endif
+#endif
+
 static errcode_t unix_open(const char *name, int flags, io_channel *channel)
 {
 	io_channel	io = NULL;
 	struct unix_private_data *data = NULL;
 	errcode_t	retval;
-	int		open_flags;
+	int		open_flags, zeroes = 0;
 	struct stat	st;
 #ifdef __linux__
 	struct 		utsname ut;
@@ -487,6 +493,12 @@ static errcode_t unix_open(const char *name, int flags, io_channel *channel)
 	}
 #endif
 
+#ifdef BLKDISCARDZEROES
+	ioctl(data->dev, BLKDISCARDZEROES, &zeroes);
+	if (zeroes)
+		io->flags |= CHANNEL_FLAGS_DISCARD_ZEROES;
+#endif
+
 #if defined(__CYGWIN__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	/*
 	 * Some operating systems require that the buffers be aligned,
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 0980045..3c5d8b7 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1903,10 +1903,6 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 #define BLKDISCARD	_IO(0x12,119)
 #endif
 
-#ifndef BLKDISCARDZEROES
-#define BLKDISCARDZEROES _IO(0x12,124)
-#endif
-
 /*
  * Return zero if the discard succeeds, and -1 if the discard fails.
  */
@@ -1941,23 +1937,6 @@ static int mke2fs_discard_blocks(ext2_filsys fs)
 	return ret;
 }
 
-static int mke2fs_discard_zeroes_data(ext2_filsys fs)
-{
-	int fd;
-	int ret;
-	int discard_zeroes_data = 0;
-
-	fd = open64(fs->device_name, O_RDWR);

[PATCH 3/6] e2fsck: Discard free data and inode blocks.

[Lukas Czerner]

In Pass 5 when we are checking block and inode bitmaps we have great
opportunity to discard free space and unused inodes on the device,
because bitmaps has just been verified as valid. This commit takes
advantage of this opportunity and discards both, all free space and
unused inodes.

I have added new set of options, 'nodiscard' and 'discard'. When the
underlying devices does not support discard, or discard ends with an
error, or when any kind of error occurs on the filesystem, no further
discard attempt will be made and the e2fsck will behave as it would
with nodiscard option provided.

As an addition, when there is any not-yet-zeroed inode table and
discard zeroes data, then inode table is marked as zeroed.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/e2fsck.8.in |   14 +++++++
 e2fsck/e2fsck.h    |    1 +
 e2fsck/pass5.c     |   96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 e2fsck/unix.c      |   10 +++++-
 4 files changed, 120 insertions(+), 1 deletions(-)

diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
index 3fb15e6..4c2dd98 100644
--- a/e2fsck/e2fsck.8.in
+++ b/e2fsck/e2fsck.8.in
@@ -189,6 +189,20 @@ be 1 or 2.  The default extended attribute version format is 2.
 .BI fragcheck
 During pass 1, print a detailed report of any discontiguous blocks for
 files in the filesystem.
+.TP
+.BI discard
+Attempt to discard free blocks and unused inode blocks after the full
+filesystem check (discarding blocks is useful on solid state devices and sparse
+/ thin-provisioned storage). Note that discard is done in pass 5 AFTER the
+filesystem has been fully checked and only if it does not contain recognizable
+errors. However there might be cases where
+.B e2fsck
+does not fully recognise a problem and hence in this case this
+option may prevent you from further manual data recovery.
+.TP
+.BI nodiscard
+Do not attempt to discard free blocks and unused inode blocks. This option is
+exacly the opposite of discard option. This is set as default.
 .RE
 .TP
 .B \-f
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index d4df5f3..a377246 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -155,6 +155,7 @@ struct resource_track {
 #define E2F_OPT_WRITECHECK	0x0200
 #define E2F_OPT_COMPRESS_DIRS	0x0400
 #define E2F_OPT_FRAGCHECK	0x0800
+#define E2F_OPT_DISCARD		0x1000
 
 /*
  * E2fsck flags
diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index cbc12f3..8f31816 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -10,9 +10,18 @@
  *
  */
 
+#include <stdint.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <errno.h>
+
 #include "e2fsck.h"
 #include "problem.h"
 
+#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
+
 static void check_block_bitmaps(e2fsck_t ctx);
 static void check_inode_bitmaps(e2fsck_t ctx);
 static void check_inode_end(e2fsck_t ctx);
@@ -64,6 +73,35 @@ void e2fsck_pass5(e2fsck_t ctx)
 	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
 }
 
+static int e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
+				  blk64_t start, blk64_t count)
+{
+	ext2_filsys fs = ctx->fs;
+	int ret = 0;
+
+	/*
+	 * If the filesystem has changed it means that there was an corruption
+	 * which should be repaired, but in some cases just one e2fsck run is
+	 * not enough to fix the problem, hence it is not safe to run discard
+	 * in this case.
+	 */
+	if (ext2fs_test_changed(ctx->fs))
+		ctx->options &= ~E2F_OPT_DISCARD;
+
+	if (ctx->options & E2F_OPT_DISCARD)
+		ret = manager->discard(fs->io, start, count, fs->blocksize);
+
+	if (ret) {
+		printf("Discard failed while trimming blocks %llu -> %llu "
+		       "with blocksize %lu\n", start, start + count,
+		       fs->blocksize);
+		ctx->flags |= E2F_FLAG_ABORT;
+		ctx->options &= ~E2F_OPT_DISCARD;
+	}
+
+	return ret;
+}
+
 #define NO_BLK ((blk64_t) -1)
 
 static void print_bitmap_problem(e2fsck_t ctx, int problem,
@@ -108,6 +146,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
 	int	group = 0;
 	int	blocks = 0;
 	blk64_t	free_blocks = 0;
+	blk64_t first_free = ext2fs_blocks_count(fs->super);
 	int	group_free = 0;
 	int	actual, bitmap;
 	struct problem_context	pctx;
@@ -120,6 +159,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
 	int	cmp_block = 0;
 	int	redo_flag = 0;
 	blk64_t	super_blk, old_desc_blk, new_desc_blk;
+	io_manager	manager = ctx->fs->io->manager;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -281,10 +321,26 @@ redo_counts:
 		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
 		had_problem++;
 
+		/*
+		 * If there a problem we should turn off the discard so we
+		 * do not compromise the filesystem.
+		 */
+		ctx->options &= ~E2F_OPT_DISCARD;
+
 	do_counts:
 		if (!bitmap && (!skip_group || csum_flag)) {
 			group_free++;
 			free_blocks++;
+			if (first_free > i)
+				first_free = i;
+		} else {
+			if ((i > first_free) &&
+			   (ctx->options & E2F_OPT_DISCARD)) {
+				if (e2fsck_discard_blocks(ctx, manager,
+					first_free, (i - first_free)))
+					goto errout;
+			}
+			first_free = ext2fs_blocks_count(fs->super);
 		}
 		blocks ++;
 		if ((blocks == fs->super->s_blocks_per_group) ||
@@ -381,6 +437,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
 	int		csum_flag;
 	int		skip_group = 0;
 	int		redo_flag = 0;
+	io_manager	manager = ctx->fs->io->manager;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -500,6 +557,11 @@ redo_counts:
 		}
 		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
 		had_problem++;
+		/*
+		 * If there a problem we should turn off the discard so we
+		 * do not compromise the filesystem.
+		 */
+		ctx->options &= ~E2F_OPT_DISCARD;
 
 do_counts:
 		if (bitmap) {
@@ -509,11 +571,45 @@ do_counts:
 			group_free++;
 			free_inodes++;
 		}
+
 		inodes++;
 		if ((inodes == fs->super->s_inodes_per_group) ||
 		    (i == fs->super->s_inodes_count)) {
+
 			free_array[group] = group_free;
 			dir_array[group] = dirs_count;
+
+			/* Discard inode table */
+			if (ctx->options & E2F_OPT_DISCARD) {
+				blk64_t used_blks, blk, num;
+
+				used_blks = DIV_ROUND_UP(
+					(EXT2_INODES_PER_GROUP(fs->super) -
+					group_free),
+					EXT2_INODES_PER_BLOCK(fs->super));
+
+				blk = ext2fs_inode_table_loc(fs, group) +
+				      used_blks;
+				num = fs->inode_blocks_per_group -
+				      used_blks;
+				if (e2fsck_discard_blocks(ctx, manager,
+							       blk, num))
+					goto errout;
+			}
+
+			/*
+			 * If discard zeroes data and the group inode table
+			 * was not zeroed yet, set itable as zeroed
+			 */
+			if ((ctx->options & E2F_OPT_DISCARD) &&
+			    (ext2fs_discard_zeroes_data(fs->io)) &&
+			    !(ext2fs_bg_flags_test(fs, group,
+						  EXT2_BG_INODE_ZEROED))) {
+				ext2fs_bg_flags_set(fs, group,
+						    EXT2_BG_INODE_ZEROED);
+				ext2fs_group_desc_csum_set(fs, group);
+			}
+
 			group ++;
 			inodes = 0;
 			skip_group = 0;
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 7eb269c..4cf55a9 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -594,6 +594,12 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		} else if (strcmp(token, "fragcheck") == 0) {
 			ctx->options |= E2F_OPT_FRAGCHECK;
 			continue;
+		} else if (strcmp(token, "discard") == 0) {
+			ctx->options |= E2F_OPT_DISCARD;
+			continue;
+		} else if (strcmp(token, "nodiscard") == 0) {
+			ctx->options &= ~E2F_OPT_DISCARD;
+			continue;
 		} else {
 			fprintf(stderr, _("Unknown extended option: %s\n"),
 				token);
@@ -609,6 +615,8 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		       "Valid extended options are:\n"), stderr);
 		fputs(("\tea_ver=<ea_version (1 or 2)>\n"), stderr);
 		fputs(("\tfragcheck\n"), stderr);
+		fputs(("\tdiscard\n"), stderr);
+		fputs(("\tnodiscard\n"), stderr);
 		fputc('\n', stderr);
 		exit(1);
 	}
@@ -667,6 +675,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
 		ctx->program_name = *argv;
 	else
 		ctx->program_name = "e2fsck";
+
 	while ((c = getopt (argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDk")) != EOF)
 		switch (c) {
 		case 'C':
@@ -943,7 +952,6 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
 	return retval;
 }
 

[PATCH 4/6] mke2fs: Deprecate -K option, introduce discard/nodiscard

[Lukas Czerner]

It would be nice to have consistent "discard" options in every system
tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
and "nodiscard" is more descriptive instead of just "-K" and can be
easily defaulted and it is something we can not do with "-K".

With this commit you need to specify extended option like this:

./mke2fs -T <fstype> -E nodiscard <device>

in order make a filesystem without discarding the device first. And

./mke2fs -T <fstype> -E discard <device>

respectively.

-K option is with this commit deprecated and should not be used anymore.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.8.in |   18 +++++++++++-------
 misc/mke2fs.c    |   12 +++++++++++-
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
index b46e7e2..c587d81 100644
--- a/misc/mke2fs.8.in
+++ b/misc/mke2fs.8.in
@@ -45,9 +45,6 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
 .I journal-options
 ]
 [
-.B \-K
-]
-[
 .B \-N
 .I number-of-inodes
 ]
@@ -240,6 +237,17 @@ enable lazy inode table initialization.
 .B test_fs
 Set a flag in the filesystem superblock indicating that it may be
 mounted using experimental kernel code, such as the ext4dev filesystem.
+.TP
+.BI discard
+Attempt to discard blocks at mkfs time (discarding blocks initially is useful
+on solid state devices and sparse / thin-provisioned storage). When the device
+advertises that discard also zeroes data (any subsequent read after the discard
+and before write returns zero), then mark all not-yet-zeroed inode tables as
+zeroed. This significantly speeds up filesystem initialization. This is set
+as default.
+.TP
+.BI nodiscard
+Do not attempt to discard blocks at mkfs time. This is the default.
 .RE
 .TP
 .BI \-f " fragment-size"
@@ -369,10 +377,6 @@ and may be no more than 102,400 filesystem blocks.
 @JDEV@.BR size " or " device
 @JDEV@options can be given for a filesystem.
 .TP
-.BI \-K
-Keep, do not attempt to discard blocks at mkfs time (discarding blocks initially
-is useful on solid state devices and sparse / thin-provisioned storage).
-.TP
 .BI \-l " filename"
 Read the bad blocks list from
 .IR filename .
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 3c5d8b7..d63c27f 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -753,6 +753,10 @@ static void parse_extended_opts(struct ext2_super_block *param,
 				lazy_itable_init = strtoul(arg, &p, 0);
 			else
 				lazy_itable_init = 1;
+		} else if (!strcmp(token, "discard")) {
+			discard = 1;
+		} else if (!strcmp(token, "nodiscard")) {
+			discard = 0;
 		} else {
 			r_usage++;
 			badopt = token;
@@ -768,7 +772,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
 			"\tstripe-width=<RAID stride * data disks in blocks>\n"
 			"\tresize=<resize maximum size in blocks>\n"
 			"\tlazy_itable_init=<0 to disable, 1 to enable>\n"
-			"\ttest_fs\n\n"),
+			"\ttest_fs\n"
+			"\tdiscard\n"
+			"\tnodiscard\n\n"),
 			badopt ? badopt : "");
 		free(buf);
 		exit(1);
@@ -1248,6 +1254,10 @@ static void PRS(int argc, char *argv[])
 			parse_journal_opts(optarg);
 			break;
 		case 'K':
+			fprintf(stderr, _("Warning: -K option is deprecated and "
+					  "should not be used anymore. Use "
+					  "\'-E nodiscard\' extended option "
+					  "instead!\n"));
 			discard = 0;
 			break;
 		case 'j':
-- 
1.7.2.3

[PATCH 5/6] mke2fs: Use unix_discard() for discards

[Lukas Czerner]

There is generic discard function in struct_io_manager, or in
unix_io_manager to be specific. So use this instead of
mke2fs_discard_blocks().

Since mke2fs_discard_blocks() is not used anymore (and should not be)
remove it.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.c |   54 +++++++++++-------------------------------------------
 1 files changed, 11 insertions(+), 43 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index d63c27f..e000058 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1907,48 +1907,6 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 	return retval;
 }
 
-#ifdef __linux__
-
-#ifndef BLKDISCARD
-#define BLKDISCARD	_IO(0x12,119)
-#endif
-
-/*
- * Return zero if the discard succeeds, and -1 if the discard fails.
- */
-static int mke2fs_discard_blocks(ext2_filsys fs)
-{
-	int fd;
-	int ret;
-	int blocksize;
-	__u64 blocks;
-	__uint64_t range[2];
-
-	blocks = ext2fs_blocks_count(fs->super);
-	blocksize = EXT2_BLOCK_SIZE(fs->super);
-	range[0] = 0;
-	range[1] = blocks * blocksize;
-
-	fd = open64(fs->device_name, O_RDWR);
-
-	if (fd > 0) {
-		ret = ioctl(fd, BLKDISCARD, &range);
-		if (verbose) {
-			printf(_("Calling BLKDISCARD from %llu to %llu "),
-			       (unsigned long long) range[0],
-			       (unsigned long long) range[1]);
-			if (ret)
-				printf(_("failed.\n"));
-			else
-				printf(_("succeeded.\n"));
-		}
-		close(fd);
-	}
-	return ret;
-}
-
-#endif
-
 int main (int argc, char *argv[])
 {
 	errcode_t	retval = 0;
@@ -2009,7 +1967,17 @@ int main (int argc, char *argv[])
 
 	/* Can't undo discard ... */
 	if (discard && (io_ptr != undo_io_manager)) {
-		retval = mke2fs_discard_blocks(fs);
+		blk64_t blocks = ext2fs_blocks_count(fs->super);
+		retval = io_ptr->discard(fs->io, 0, blocks, fs->blocksize);
+
+		if (verbose) {
+			printf(_("Calling BLKDISCARD from 0 to %llu "),
+			       (unsigned long long) blocks);
+			if (retval)
+				printf(_("failed.\n"));
+			else
+				printf(_("succeeded.\n"));
+		}
 
 		if (!retval && ext2fs_discard_zeroes_data(fs->io)) {
 			if (verbose)
-- 
1.7.2.3

[PATCH 6/6] mke2fs: Add discard option into mke2fs.conf

[Lukas Czerner]

Allow to specify discard in mke2fs.conf. Also change the way how to
specify default value for lazy_itable_init. It is better to have all
this defaulting done in the same place so do it in definition (as we do
with discard).

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.c         |    9 +++++----
 misc/mke2fs.conf.5.in |    5 +++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index e000058..2b423d7 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -83,12 +83,12 @@ int	cflag;
 int	verbose;
 int	quiet;
 int	super_only;
-int	discard = 1;
+int	discard = 1;	/* attempt  to discard device before fs creation */
 int	force;
 int	noaction;
 int	journal_size;
 int	journal_flags;
-int	lazy_itable_init;	/* use lazy inode table init */
+int	lazy_itable_init = 0;	/* do not use lazy inode table init */
 char	*bad_blocks_filename;
 __u32	fs_stride;
 
@@ -1686,8 +1686,9 @@ static void PRS(int argc, char *argv[])
 
 	blocksize = EXT2_BLOCK_SIZE(&fs_param);
 
-	lazy_itable_init = get_bool_from_profile(fs_types,
-						 "lazy_itable_init", 0);
+	lazy_itable_init = get_bool_from_profile(fs_types, "lazy_itable_init",
+						 lazy_itable_init);
+	discard = get_bool_from_profile(fs_types, "discard" , discard);
 
 	/* Get options from profile */
 	for (cpp = fs_types; *cpp; cpp++) {
diff --git a/misc/mke2fs.conf.5.in b/misc/mke2fs.conf.5.in
index d212947..e9913cb 100644
--- a/misc/mke2fs.conf.5.in
+++ b/misc/mke2fs.conf.5.in
@@ -360,6 +360,11 @@ option.  This can be used to configure the default extended options used
 by
 .BR mke2fs (8)
 on a per-filesystem type basis.
+.TP
+.I discard
+This relation is a boolean which specifies whether the
+.BR mke2fs (8)
+should attempt to discard device prior to filesystem creation.
 .SH FILES
 .TP
 .I /etc/mke2fs.conf
-- 
1.7.2.3

Re: [1/6] e2fsprogs: Add discard function into struct_io_manager

[Ted Ts'o]

On Thu, Nov 18, 2010 at 03:38:36AM -0000, Lukas Czerner wrote:
> In order to provide generic "discard" function for all e2fsprogs tools
> add a discard function prototype into struct_io_manager. Specific
> function for specific io managers can be crated that way.
> 
> This commit also creates unix_discard function which uses BLKDISCARD
> ioctl to discard data blocks on the block device and bind it into
> unit_io_manager structure to be available for all e2fsprogs tools.
> Note that BLKDISCARD is still Linux specific ioctl, however other
> unix systems may provide similar functionality. So far the
> unix_discard() remains linux specific hence is embedded in #ifdef
> __linux__ macro.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

I had to make a few cleanup changes before I could merge this into the
master branch.

1) unix_discard() was defined as a macro in the !__linux__ case.
Since unix_discard() gets used to fill in a function pointer in the
io_channel_ops structure, this would have caused non-linux compiles to
break.

2) I added a io_channel_discard() function which checks to see if
io_channel->discard is non-NULL before dereferencing the function
pointer (you had mke2fs and e2fsck blindly dereferencing the function
pointer before checking to see if it was non-NULL; this is bad,
because not all io_managers, in particular, not the test_io manager in
your patches, support discard).

3) I added support for discard to the test_io manager, which makes
debugging much easier (just compile with --enable-testio-debug, and
then set the TEST_IO_FLAGS environment variable).

4) There was no need to pass the block size to the discard function.
The blocksize is part of the io_channel abstraction, and is stored in
the channel structure.

					- Ted

The resulting patch looked like this.

commit e90a59ed434d6c5e38dd148aa4ba5b22b8f7eb24
Author: Lukas Czerner <lczerner@redhat.com>
Date:   Thu Nov 18 03:38:36 2010 +0000

    e2fsprogs: Add discard function into struct_io_manager
    
    In order to provide generic "discard" function for all e2fsprogs tools
    add a discard function prototype into struct_io_manager. Specific
    function for specific io managers can be crated that way.
    
    This commit also creates unix_discard function which uses BLKDISCARD
    ioctl to discard data blocks on the block device and bind it into
    unit_io_manager structure to be available for all e2fsprogs tools.
    Note that BLKDISCARD is still Linux specific ioctl, however other
    unix systems may provide similar functionality. So far the
    unix_discard() remains linux specific hence is embedded in #ifdef
    __linux__ macro.
    
    Signed-off-by: Lukas Czerner <lczerner@redhat.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
index ccc9c8b..f26b569 100644
--- a/lib/ext2fs/ext2_io.h
+++ b/lib/ext2fs/ext2_io.h
@@ -83,6 +83,8 @@ struct struct_io_manager {
 					int count, void *data);
 	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
 					int count, const void *data);
+	errcode_t (*discard)(io_channel channel, unsigned long long block,
+			     unsigned long long count);
 	long	reserved[16];
 };
 
diff --git a/lib/ext2fs/io_manager.c b/lib/ext2fs/io_manager.c
index 6d0e234..80f9dfc 100644
--- a/lib/ext2fs/io_manager.c
+++ b/lib/ext2fs/io_manager.c
@@ -99,3 +99,14 @@ errcode_t io_channel_write_blk64(io_channel channel, unsigned long long block,
 	return (channel->manager->write_blk)(channel, (unsigned long) block,
 					     count, data);
 }
+
+errcode_t io_channel_discard(io_channel channel, unsigned long long block,
+			     unsigned long long count)
+{
+	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
+
+	if (channel->manager->discard)
+		return (channel->manager->discard)(channel, block, count);
+
+	return EXT2_ET_UNIMPLEMENTED;
+}
diff --git a/lib/ext2fs/test_io.c b/lib/ext2fs/test_io.c
index 8d887a8..242d442 100644
--- a/lib/ext2fs/test_io.c
+++ b/lib/ext2fs/test_io.c
@@ -73,7 +73,8 @@ static errcode_t test_write_byte(io_channel channel, unsigned long offset,
 static errcode_t test_set_option(io_channel channel, const char *option,
 				 const char *arg);
 static errcode_t test_get_stats(io_channel channel, io_stats *stats);
-
+static errcode_t test_discard(io_channel channel, unsigned long long block,
+			      unsigned long long count);
 
 static struct struct_io_manager struct_test_manager = {
 	EXT2_ET_MAGIC_IO_MANAGER,
@@ -89,6 +90,7 @@ static struct struct_io_manager struct_test_manager = {
 	test_get_stats,
 	test_read_blk64,
 	test_write_blk64,
+	test_discard,
 };
 
 io_manager test_io_manager = &struct_test_manager;
@@ -120,6 +122,7 @@ void (*test_io_cb_write_byte)
 #define TEST_FLAG_FLUSH			0x08
 #define TEST_FLAG_DUMP			0x10
 #define TEST_FLAG_SET_OPTION		0x20
+#define TEST_FLAG_DISCARD		0x40
 
 static void test_dump_block(io_channel channel,
 			    struct test_private_data *data,
@@ -495,3 +498,21 @@ static errcode_t test_get_stats(io_channel channel, io_stats *stats)
 	}
 	return retval;
 }
+
+static errcode_t test_discard(io_channel channel, unsigned long long block,
+			      unsigned long long count)
+{
+	struct test_private_data *data;
+	errcode_t	retval = 0;
+
+	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
+	data = (struct test_private_data *) channel->private_data;
+	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_TEST_IO_CHANNEL);
+
+	retval = io_channel_discard(channel, block, count);
+	if (data->flags & TEST_FLAG_DISCARD)
+		fprintf(data->outfile,
+			"Test_io: discard(%llu, %llu) returned %s\n",
+			block, count, retval ? error_message(retval) : "OK");
+	return retval;
+}
diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index 1df1fdd..2302374 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
 			       int count, void *data);
 static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
 				int count, const void *data);
+static errcode_t unix_discard(io_channel channel, unsigned long long block,
+			      unsigned long long count);
 
 static struct struct_io_manager struct_unix_manager = {
 	EXT2_ET_MAGIC_IO_MANAGER,
@@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
 	unix_get_stats,
 	unix_read_blk64,
 	unix_write_blk64,
+	unix_discard,
 };
 
 io_manager unix_io_manager = &struct_unix_manager;
@@ -834,3 +837,31 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
 	}
 	return EXT2_ET_INVALID_ARGUMENT;
 }
+
+#if defined(__linux__) && !defined(BLKDISCARD)
+#define BLKDISCARD	_IO(0x12,119)
+#endif
+
+static errcode_t unix_discard(io_channel channel, unsigned long long block,
+			      unsigned long long count)
+{
+#ifdef BLKDISCARD
+	struct unix_private_data *data;
+	__uint64_t	range[2];
+	int		ret;
+
+	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
+	data = (struct unix_private_data *) channel->private_data;
+	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
+
+	range[0] = (__uint64_t)(block) * channel->block_size;
+	range[1] = (__uint64_t)(count) * channel->block_size;
+
+	ret = ioctl(data->dev, BLKDISCARD, &range);
+	if (ret < 0)
+		return errno;
+	return 0;
+#else
+	return EXT2_ET_UNIMPLEMENTED;
+#endif
+}

Re: [2/6] e2fsprogs: Add CHANNEL_FLAGS_DISCARD_ZEROES flag for io_manager

[Ted Ts'o]

On Thu, Nov 18, 2010 at 03:38:37AM -0000, Lukas Czerner wrote:
> When the device have discard support and simultaneously discard zeroes
> data (and it is properly advertised), then we can take advantage of such
> behavior in several e2fsprogs tools.
> 
> Add new flag CHANNEL_FLAGS_DISCARD_ZEROES for struct_io_channel so
> each io_manager can take advantage of this. The flag is properly set
> according to BLKDISCARDZEROES ioctl in unix_open.
> 
> Also remove old mke2fs_discard_zeroes_data() function and substitute it
> with helper which test this flag.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

I merged this patch with a minor change; I renamed
ext2fs_discard_zeroes_data() to io_channel_discard_zeros_data().

								- Ted

Re: [3/6] e2fsck: Discard free data and inode blocks.

[Ted Ts'o]

On Thu, Nov 18, 2010 at 03:38:38AM -0000, Lukas Czerner wrote:
> In Pass 5 when we are checking block and inode bitmaps we have great
> opportunity to discard free space and unused inodes on the device,
> because bitmaps has just been verified as valid. This commit takes
> advantage of this opportunity and discards both, all free space and
> unused inodes.
> 
> I have added new set of options, 'nodiscard' and 'discard'. When the
> underlying devices does not support discard, or discard ends with an
> error, or when any kind of error occurs on the filesystem, no further
> discard attempt will be made and the e2fsck will behave as it would
> with nodiscard option provided.

Unfortunately, your patch set the ABORT flag and then returned an
error, which then cause pass 5 processing to stop.  I've fixed in my
patches.

> 
> As an addition, when there is any not-yet-zeroed inode table and
> discard zeroes data, then inode table is marked as zeroed.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

					- Ted

commit efa1a355a1e4142b2d057be06931eb8fc0903ba3
Author: Lukas Czerner <lczerner@redhat.com>
Date:   Thu Nov 18 03:38:38 2010 +0000

    e2fsck: Discard free data and inode blocks.
    
    In Pass 5 when we are checking block and inode bitmaps we have great
    opportunity to discard free space and unused inodes on the device,
    because bitmaps has just been verified as valid. This commit takes
    advantage of this opportunity and discards both, all free space and
    unused inodes.
    
    I have added new set of options, 'nodiscard' and 'discard'. When the
    underlying devices does not support discard, or discard ends with an
    error, or when any kind of error occurs on the filesystem, no further
    discard attempt will be made and the e2fsck will behave as it would
    with nodiscard option provided.
    
    As an addition, when there is any not-yet-zeroed inode table and
    discard zeroes data, then inode table is marked as zeroed.
    
    Signed-off-by: Lukas Czerner <lczerner@redhat.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
index 3fb15e6..4c2dd98 100644
--- a/e2fsck/e2fsck.8.in
+++ b/e2fsck/e2fsck.8.in
@@ -189,6 +189,20 @@ be 1 or 2.  The default extended attribute version format is 2.
 .BI fragcheck
 During pass 1, print a detailed report of any discontiguous blocks for
 files in the filesystem.
+.TP
+.BI discard
+Attempt to discard free blocks and unused inode blocks after the full
+filesystem check (discarding blocks is useful on solid state devices and sparse
+/ thin-provisioned storage). Note that discard is done in pass 5 AFTER the
+filesystem has been fully checked and only if it does not contain recognizable
+errors. However there might be cases where
+.B e2fsck
+does not fully recognise a problem and hence in this case this
+option may prevent you from further manual data recovery.
+.TP
+.BI nodiscard
+Do not attempt to discard free blocks and unused inode blocks. This option is
+exacly the opposite of discard option. This is set as default.
 .RE
 .TP
 .B \-f
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index d4df5f3..a377246 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -155,6 +155,7 @@ struct resource_track {
 #define E2F_OPT_WRITECHECK	0x0200
 #define E2F_OPT_COMPRESS_DIRS	0x0400
 #define E2F_OPT_FRAGCHECK	0x0800
+#define E2F_OPT_DISCARD		0x1000
 
 /*
  * E2fsck flags
diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index cbc12f3..b423d28 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -10,9 +10,18 @@
  *
  */
 
+#include <stdint.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <errno.h>
+
 #include "e2fsck.h"
 #include "problem.h"
 
+#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
+
 static void check_block_bitmaps(e2fsck_t ctx);
 static void check_inode_bitmaps(e2fsck_t ctx);
 static void check_inode_end(e2fsck_t ctx);
@@ -64,6 +73,26 @@ void e2fsck_pass5(e2fsck_t ctx)
 	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
 }
 
+static void e2fsck_discard_blocks(e2fsck_t ctx, io_manager manager,
+				  blk64_t start, blk64_t count)
+{
+	ext2_filsys fs = ctx->fs;
+	int ret = 0;
+
+	/*
+	 * If the filesystem has changed it means that there was an corruption
+	 * which should be repaired, but in some cases just one e2fsck run is
+	 * not enough to fix the problem, hence it is not safe to run discard
+	 * in this case.
+	 */
+	if (ext2fs_test_changed(ctx->fs))
+		ctx->options &= ~E2F_OPT_DISCARD;
+
+	if ((ctx->options & E2F_OPT_DISCARD) &&
+	    (io_channel_discard(fs->io, start, count)))
+		ctx->options &= ~E2F_OPT_DISCARD;
+}
+
 #define NO_BLK ((blk64_t) -1)
 
 static void print_bitmap_problem(e2fsck_t ctx, int problem,
@@ -108,6 +137,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
 	int	group = 0;
 	int	blocks = 0;
 	blk64_t	free_blocks = 0;
+	blk64_t first_free = ext2fs_blocks_count(fs->super);
 	int	group_free = 0;
 	int	actual, bitmap;
 	struct problem_context	pctx;
@@ -120,6 +150,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
 	int	cmp_block = 0;
 	int	redo_flag = 0;
 	blk64_t	super_blk, old_desc_blk, new_desc_blk;
+	io_manager	manager = ctx->fs->io->manager;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -281,10 +312,25 @@ redo_counts:
 		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
 		had_problem++;
 
+		/*
+		 * If there a problem we should turn off the discard so we
+		 * do not compromise the filesystem.
+		 */
+		ctx->options &= ~E2F_OPT_DISCARD;
+
 	do_counts:
 		if (!bitmap && (!skip_group || csum_flag)) {
 			group_free++;
 			free_blocks++;
+			if (first_free > i)
+				first_free = i;
+		} else {
+			if ((i > first_free) &&
+			   (ctx->options & E2F_OPT_DISCARD)) {
+				e2fsck_discard_blocks(ctx, manager, first_free,
+						      (i - first_free));
+			}
+			first_free = ext2fs_blocks_count(fs->super);
 		}
 		blocks ++;
 		if ((blocks == fs->super->s_blocks_per_group) ||
@@ -381,6 +427,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
 	int		csum_flag;
 	int		skip_group = 0;
 	int		redo_flag = 0;
+	io_manager	manager = ctx->fs->io->manager;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -500,6 +547,11 @@ redo_counts:
 		}
 		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
 		had_problem++;
+		/*
+		 * If there a problem we should turn off the discard so we
+		 * do not compromise the filesystem.
+		 */
+		ctx->options &= ~E2F_OPT_DISCARD;
 
 do_counts:
 		if (bitmap) {
@@ -509,11 +561,43 @@ do_counts:
 			group_free++;
 			free_inodes++;
 		}
+
 		inodes++;
 		if ((inodes == fs->super->s_inodes_per_group) ||
 		    (i == fs->super->s_inodes_count)) {
+
 			free_array[group] = group_free;
 			dir_array[group] = dirs_count;
+
+			/* Discard inode table */
+			if (ctx->options & E2F_OPT_DISCARD) {
+				blk64_t used_blks, blk, num;
+
+				used_blks = DIV_ROUND_UP(
+					(EXT2_INODES_PER_GROUP(fs->super) -
+					group_free),
+					EXT2_INODES_PER_BLOCK(fs->super));
+
+				blk = ext2fs_inode_table_loc(fs, group) +
+				      used_blks;
+				num = fs->inode_blocks_per_group -
+				      used_blks;
+				e2fsck_discard_blocks(ctx, manager, blk, num);
+			}
+
+			/*
+			 * If discard zeroes data and the group inode table
+			 * was not zeroed yet, set itable as zeroed
+			 */
+			if ((ctx->options & E2F_OPT_DISCARD) &&
+			    (io_channel_discard_zeroes_data(fs->io)) &&
+			    !(ext2fs_bg_flags_test(fs, group,
+						  EXT2_BG_INODE_ZEROED))) {
+				ext2fs_bg_flags_set(fs, group,
+						    EXT2_BG_INODE_ZEROED);
+				ext2fs_group_desc_csum_set(fs, group);
+			}
+
 			group ++;
 			inodes = 0;
 			skip_group = 0;
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 7eb269c..4cf55a9 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -594,6 +594,12 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		} else if (strcmp(token, "fragcheck") == 0) {
 			ctx->options |= E2F_OPT_FRAGCHECK;
 			continue;
+		} else if (strcmp(token, "discard") == 0) {
+			ctx->options |= E2F_OPT_DISCARD;
+			continue;
+		} else if (strcmp(token, "nodiscard") == 0) {
+			ctx->options &= ~E2F_OPT_DISCARD;
+			continue;
 		} else {
 			fprintf(stderr, _("Unknown extended option: %s\n"),
 				token);
@@ -609,6 +615,8 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		       "Valid extended options are:\n"), stderr);
 		fputs(("\tea_ver=<ea_version (1 or 2)>\n"), stderr);
 		fputs(("\tfragcheck\n"), stderr);
+		fputs(("\tdiscard\n"), stderr);
+		fputs(("\tnodiscard\n"), stderr);
 		fputc('\n', stderr);
 		exit(1);
 	}
@@ -667,6 +675,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
 		ctx->program_name = *argv;
 	else
 		ctx->program_name = "e2fsck";
+
 	while ((c = getopt (argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDk")) != EOF)
 		switch (c) {
 		case 'C':
@@ -943,7 +952,6 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
 	return retval;
 }
 

Re: [4/6] mke2fs: Deprecate -K option, introduce discard/nodiscard

[Ted Ts'o]

On Thu, Nov 18, 2010 at 03:38:39AM -0000, Lukas Czerner wrote:
> It would be nice to have consistent "discard" options in every system
> tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
> and "nodiscard" is more descriptive instead of just "-K" and can be
> easily defaulted and it is something we can not do with "-K".
> 
> With this commit you need to specify extended option like this:
> 
> ./mke2fs -T <fstype> -E nodiscard <device>
> 
> in order make a filesystem without discarding the device first. And
> 
> ./mke2fs -T <fstype> -E discard <device>
> 
> respectively.
> 
> -K option is with this commit deprecated and should not be used anymore.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied to the maint branch of e2fsprogs, so it will make 1.41.13.

	       	     	       		     - Ted

Re: [5/6] mke2fs: Use unix_discard() for discards

[Ted Ts'o]

On Thu, Nov 18, 2010 at 03:38:40AM -0000, Lukas Czerner wrote:
> There is generic discard function in struct_io_manager, or in
> unix_io_manager to be specific. So use this instead of
> mke2fs_discard_blocks().
> 
> Since mke2fs_discard_blocks() is not used anymore (and should not be)
> remove it.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Changed to use io_channel_discard() instead of derferencing the
function pointer directly.  I've applied this to the master branch.

	 	 	    	 	 - Ted

Re: [6/6] mke2fs: Add discard option into mke2fs.conf

[Ted Ts'o]

On Thu, Nov 18, 2010 at 03:38:41AM -0000, Lukas Czerner wrote:
> Allow to specify discard in mke2fs.conf. Also change the way how to
> specify default value for lazy_itable_init. It is better to have all
> this defaulting done in the same place so do it in definition (as we do
> with discard).
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied to the maint branch, so it will git e2fsprogs 1.41.13.

	       	     	     	   	- Ted

Re: [1/6] e2fsprogs: Add discard function into struct_io_manager

[Lukas Czerner]

On Tue, 23 Nov 2010, Ted Ts'o wrote:

> On Thu, Nov 18, 2010 at 03:38:36AM -0000, Lukas Czerner wrote:
> > In order to provide generic "discard" function for all e2fsprogs tools
> > add a discard function prototype into struct_io_manager. Specific
> > function for specific io managers can be crated that way.
> > 
> > This commit also creates unix_discard function which uses BLKDISCARD
> > ioctl to discard data blocks on the block device and bind it into
> > unit_io_manager structure to be available for all e2fsprogs tools.
> > Note that BLKDISCARD is still Linux specific ioctl, however other
> > unix systems may provide similar functionality. So far the
> > unix_discard() remains linux specific hence is embedded in #ifdef
> > __linux__ macro.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> I had to make a few cleanup changes before I could merge this into the
> master branch.
> 
> 1) unix_discard() was defined as a macro in the !__linux__ case.
> Since unix_discard() gets used to fill in a function pointer in the
> io_channel_ops structure, this would have caused non-linux compiles to
> break.
> 
> 2) I added a io_channel_discard() function which checks to see if
> io_channel->discard is non-NULL before dereferencing the function
> pointer (you had mke2fs and e2fsck blindly dereferencing the function
> pointer before checking to see if it was non-NULL; this is bad,
> because not all io_managers, in particular, not the test_io manager in
> your patches, support discard).
> 
> 3) I added support for discard to the test_io manager, which makes
> debugging much easier (just compile with --enable-testio-debug, and
> then set the TEST_IO_FLAGS environment variable).
> 
> 4) There was no need to pass the block size to the discard function.
> The blocksize is part of the io_channel abstraction, and is stored in
> the channel structure.
> 
> 					- Ted

Hi Ted,

sorry for the delay. Thanks a lot for your review, fixes and merge, it
looks good.

Thanks!

-Lukas

> 
> The resulting patch looked like this.
> 
> commit e90a59ed434d6c5e38dd148aa4ba5b22b8f7eb24
> Author: Lukas Czerner <lczerner@redhat.com>
> Date:   Thu Nov 18 03:38:36 2010 +0000
> 
>     e2fsprogs: Add discard function into struct_io_manager
>     
>     In order to provide generic "discard" function for all e2fsprogs tools
>     add a discard function prototype into struct_io_manager. Specific
>     function for specific io managers can be crated that way.
>     
>     This commit also creates unix_discard function which uses BLKDISCARD
>     ioctl to discard data blocks on the block device and bind it into
>     unit_io_manager structure to be available for all e2fsprogs tools.
>     Note that BLKDISCARD is still Linux specific ioctl, however other
>     unix systems may provide similar functionality. So far the
>     unix_discard() remains linux specific hence is embedded in #ifdef
>     __linux__ macro.
>     
>     Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
> index ccc9c8b..f26b569 100644
> --- a/lib/ext2fs/ext2_io.h
> +++ b/lib/ext2fs/ext2_io.h
> @@ -83,6 +83,8 @@ struct struct_io_manager {
>  					int count, void *data);
>  	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
>  					int count, const void *data);
> +	errcode_t (*discard)(io_channel channel, unsigned long long block,
> +			     unsigned long long count);
>  	long	reserved[16];
>  };
>  
> diff --git a/lib/ext2fs/io_manager.c b/lib/ext2fs/io_manager.c
> index 6d0e234..80f9dfc 100644
> --- a/lib/ext2fs/io_manager.c
> +++ b/lib/ext2fs/io_manager.c
> @@ -99,3 +99,14 @@ errcode_t io_channel_write_blk64(io_channel channel, unsigned long long block,
>  	return (channel->manager->write_blk)(channel, (unsigned long) block,
>  					     count, data);
>  }
> +
> +errcode_t io_channel_discard(io_channel channel, unsigned long long block,
> +			     unsigned long long count)
> +{
> +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> +
> +	if (channel->manager->discard)
> +		return (channel->manager->discard)(channel, block, count);
> +
> +	return EXT2_ET_UNIMPLEMENTED;
> +}
> diff --git a/lib/ext2fs/test_io.c b/lib/ext2fs/test_io.c
> index 8d887a8..242d442 100644
> --- a/lib/ext2fs/test_io.c
> +++ b/lib/ext2fs/test_io.c
> @@ -73,7 +73,8 @@ static errcode_t test_write_byte(io_channel channel, unsigned long offset,
>  static errcode_t test_set_option(io_channel channel, const char *option,
>  				 const char *arg);
>  static errcode_t test_get_stats(io_channel channel, io_stats *stats);
> -
> +static errcode_t test_discard(io_channel channel, unsigned long long block,
> +			      unsigned long long count);
>  
>  static struct struct_io_manager struct_test_manager = {
>  	EXT2_ET_MAGIC_IO_MANAGER,
> @@ -89,6 +90,7 @@ static struct struct_io_manager struct_test_manager = {
>  	test_get_stats,
>  	test_read_blk64,
>  	test_write_blk64,
> +	test_discard,
>  };
>  
>  io_manager test_io_manager = &struct_test_manager;
> @@ -120,6 +122,7 @@ void (*test_io_cb_write_byte)
>  #define TEST_FLAG_FLUSH			0x08
>  #define TEST_FLAG_DUMP			0x10
>  #define TEST_FLAG_SET_OPTION		0x20
> +#define TEST_FLAG_DISCARD		0x40
>  
>  static void test_dump_block(io_channel channel,
>  			    struct test_private_data *data,
> @@ -495,3 +498,21 @@ static errcode_t test_get_stats(io_channel channel, io_stats *stats)
>  	}
>  	return retval;
>  }
> +
> +static errcode_t test_discard(io_channel channel, unsigned long long block,
> +			      unsigned long long count)
> +{
> +	struct test_private_data *data;
> +	errcode_t	retval = 0;
> +
> +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> +	data = (struct test_private_data *) channel->private_data;
> +	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_TEST_IO_CHANNEL);
> +
> +	retval = io_channel_discard(channel, block, count);
> +	if (data->flags & TEST_FLAG_DISCARD)
> +		fprintf(data->outfile,
> +			"Test_io: discard(%llu, %llu) returned %s\n",
> +			block, count, retval ? error_message(retval) : "OK");
> +	return retval;
> +}
> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> index 1df1fdd..2302374 100644
> --- a/lib/ext2fs/unix_io.c
> +++ b/lib/ext2fs/unix_io.c
> @@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
>  			       int count, void *data);
>  static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
>  				int count, const void *data);
> +static errcode_t unix_discard(io_channel channel, unsigned long long block,
> +			      unsigned long long count);
>  
>  static struct struct_io_manager struct_unix_manager = {
>  	EXT2_ET_MAGIC_IO_MANAGER,
> @@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
>  	unix_get_stats,
>  	unix_read_blk64,
>  	unix_write_blk64,
> +	unix_discard,
>  };
>  
>  io_manager unix_io_manager = &struct_unix_manager;
> @@ -834,3 +837,31 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
>  	}
>  	return EXT2_ET_INVALID_ARGUMENT;
>  }
> +
> +#if defined(__linux__) && !defined(BLKDISCARD)
> +#define BLKDISCARD	_IO(0x12,119)
> +#endif
> +
> +static errcode_t unix_discard(io_channel channel, unsigned long long block,
> +			      unsigned long long count)
> +{
> +#ifdef BLKDISCARD
> +	struct unix_private_data *data;
> +	__uint64_t	range[2];
> +	int		ret;
> +
> +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> +	data = (struct unix_private_data *) channel->private_data;
> +	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
> +
> +	range[0] = (__uint64_t)(block) * channel->block_size;
> +	range[1] = (__uint64_t)(count) * channel->block_size;
> +
> +	ret = ioctl(data->dev, BLKDISCARD, &range);
> +	if (ret < 0)
> +		return errno;
> +	return 0;
> +#else
> +	return EXT2_ET_UNIMPLEMENTED;
> +#endif
> +}
> 

--