[Ocfs2-tools-devel] [PATCH] discard unused blocks before mkfs

[Gang He]

Hello Larry,

Please my comments inline.


>>> 
> From: Larry <lchen at suse.com>
> 
> When using a SSD as a block device, if unused SSD blocks could
> be discarded in advance, performance will be improved.
> 
> This patch uses ioctl interface to release unused blocks on SSD
> layer.
> 
> Signed-off-by: Larry Chen <lchen at suse.com>
> ---
>  mkfs.ocfs2/mkfs.c | 56 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  mkfs.ocfs2/mkfs.h | 10 ++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/mkfs.ocfs2/mkfs.c b/mkfs.ocfs2/mkfs.c
> index 354b2ee5..2eeeb5cc 100644
> --- a/mkfs.ocfs2/mkfs.c
> +++ b/mkfs.ocfs2/mkfs.c
> @@ -543,6 +543,49 @@ static void finish_normal_format(State *s)
>  	ocfs2_close(fs);
>  }
>  
> +static inline errcode_t __discard_blocks(State *s, uint64_t block,
> +		uint64_t count)  
Prefer to use discard_blocks function name, since __ function prefix looks a little obsolete. 

> +{
> +	int ret;
> +	uint64_t range[2];
> +
> +	range[0] = (uint64_t)(block) << s->blocksize_bits;
> +	range[1] = (uint64_t)(count) << s->blocksize_bits;
> +
> +	ret = ioctl(s->fd, BLKDISCARD, &range);
> +	if (ret < 0) { 
Here, I think you should return ioctl returned value directly, since the below function will call this function many times,
It can handle the returned value.

> +		if (!s->quiet) { 
Please NOT print any message if the return error is EOPNOTSUPP, since this should be considered as a normal case.

> +			com_err(s->progname, errno,
> +				"While discarding unused blocks...skip\n");
> +		}
> +		return errno;
> +	}
> +	return 0;
> +}
> +
> +static int discard_blocks(State *s)
Please use discard_fs_blocks name, or similar name, which should be more closer to what the function does.

> +{
> +	uint64_t blocks = s->volume_size_in_blocks;
> +	uint64_t count = DISCARD_STEP_MB;
> +	uint64_t cur = 0;
> +	int retval = 0;
> +
> +	count *= (1024 * 1024);
> +	count >>= s->blocksize_bits;
> +
> +	while (cur < blocks) {
> +		if (cur + count > blocks)
> +			count = blocks - cur;
> +
> +		retval = __discard_blocks(s, cur, count);
> +		if (retval)
> +			break;
> +		cur += count;
> +	}
> +
> +	return retval;
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -615,6 +658,10 @@ main(int argc, char **argv)
>  		return 0;
>  	}
>  
> +	if (s->discard_blocks) {
> +		discard_blocks(s);
> +	}
> +
>  	clear_both_ends(s);
>  
>  	init_record(s, &superblock_rec, SFI_OTHER, S_IFREG | 0644);
> @@ -889,6 +936,7 @@ get_state(int argc, char **argv)
>  	int no_backup_super = -1;
>  	enum ocfs2_feature_levels level = OCFS2_FEATURE_LEVEL_DEFAULT;
>  	ocfs2_fs_options feature_flags = {0,0,0}, reverse_flags = {0,0,0};
> +	int discard_blocks = 0;
>  
>  	static struct option long_options[] = {
>  		{ "block-size", 1, 0, 'b' },
> @@ -903,6 +951,7 @@ get_state(int argc, char **argv)
>  		{ "force", 0, 0, 'F'},
>  		{ "mount", 1, 0, 'M'},
>  		{ "dry-run", 0, 0, 'n' },
> +		{ "discard-blocks", 0, 0, 'd'},
I refer to use ON as the default value, since if the device does not support trim operation, just return EOPNOTSUPP in  discard_blocks function and skip
to the next things, do not impact mkfs.ocfs2 performance, the mkfs.ext4 also turn this option on by default.
Finally, please also update man page, since this change will bring a new interface to the users.

Thanks
Gang

>  		{ "no-backup-super", 0, 0, BACKUP_SUPER_OPTION },
>  		{ "fs-feature-level=", 1, 0, FEATURE_LEVEL },
>  		{ "fs-features=", 1, 0, FEATURES_OPTION },
> @@ -918,7 +967,7 @@ get_state(int argc, char **argv)
>  		progname = "mkfs.ocfs2";
>  
>  	while (1) {
> -		c = getopt_long(argc, argv, "b:C:L:N:J:M:vnqVFHxT:U:",
> +		c = getopt_long(argc, argv, "b:C:L:N:J:M:vnqVFHxT:U:d",
>  				long_options, NULL);
>  
>  		if (c == -1)
> @@ -1103,6 +1152,10 @@ get_state(int argc, char **argv)
>  			globalhb = 1;
>  			break;
>  
> +		case 'd':
> +			discard_blocks = 1;
> +			break;
> +
>  		default:
>  			usage(progname);
>  			break;
> @@ -1148,6 +1201,7 @@ get_state(int argc, char **argv)
>  	s->quiet         = quiet;
>  	s->force         = force;
>  	s->dry_run       = dry_run;
> +	s->discard_blocks = discard_blocks;
>  
>  	s->prompt        = xtool ? 0 : 1;
>  
> diff --git a/mkfs.ocfs2/mkfs.h b/mkfs.ocfs2/mkfs.h
> index f9ba4dcf..b144341f 100644
> --- a/mkfs.ocfs2/mkfs.h
> +++ b/mkfs.ocfs2/mkfs.h
> @@ -41,6 +41,7 @@
>  #include <inttypes.h>
>  #include <ctype.h>
>  #include <assert.h>
> +#include <sys/ioctl.h>
>  
>  #include <uuid/uuid.h>
>  
> @@ -92,6 +93,14 @@
>  
>  #define MAX_EXTALLOC_RESERVE_PERCENT	5
>  
> +#define DISCARD_STEP_MB         2048
> +
> +#if defined(__linux__) && !defined(BLKDISCARD)
> +#define BLKDISCARD		_IO(0x12,119)
> +#endif
> +
> +
> +
>  enum {
>  	SFI_JOURNAL,
>  	SFI_CLUSTER,
> @@ -193,6 +202,7 @@ struct _State {
>  	int inline_data;
>  	int dx_dirs;
>  	int dry_run;
> +	int discard_blocks;
>  
>  	uint32_t blocksize;
>  	uint32_t blocksize_bits;
> -- 
> 2.16.2