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

[Larry Chen]

Hi Gang,

Thanks for your review.

On 02/27/2018 02:54 PM, Gang He wrote:
> 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.
>
I'll rename __discard_blocks as discard_blocks
>> +{
>> +	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.
Fine, I'll do it later. thanks.
>> +		if (!s->quiet) {
> Please NOT print any message if the return error is EOPNOTSUPP, since this should be considered as a normal case.
>
Reasonable.
>> +			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.
>
I'll use discard_device_blocks as the function name.
>> +{
>> +	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.
I'll remove -d option, and open discard option as default.
Besides, add new option --no-discard for users to specify their decision.
>
> 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