linux-ext4 - Re: [PATCH] e2fsck: add optimization for heavily hard-linked file systems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 11 Sep 2017 12:19:05 +0200
From:   Jaco Kroon <jaco@....co.za>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Doug Porter <dsp@...com>, linux-ext4@...r.kernel.org,
        Omar Sandoval <osandov@...com>
Subject: Re: [PATCH] e2fsck: add optimization for heavily hard-linked file
 systems

Hi Theodore,

I got severely side-tracked after the previous look at this.

With the mentions below, ACK.

Kind Regards,
Jaco


On 26/08/2017 17:33, Theodore Ts'o wrote:
> On Thu, Aug 24, 2017 at 11:08:52AM +0200, Jaco Kroon wrote:
>>> +#define E2F_OPT_ICOUNT_FULLMAP	0x20000 /* don't optimize extents */
>> description here seems wrong?  Perhaps "use fullmap for inode link count" ?
> Oops, fixed.
>
>>> --- a/e2fsck/pass1.c
>>> +++ b/e2fsck/pass1.c
>>> @@ -711,6 +711,7 @@ extern errcode_t e2fsck_setup_icount(e2fsck_t ctx, const char *icount_name,
>>>    	errcode_t		retval;
>>>    	char			*tdb_dir;
>>>    	int			enable;
>>> +	int			full_map;
>>>    	*ret = 0;
>>> @@ -734,6 +735,8 @@ extern errcode_t e2fsck_setup_icount(e2fsck_t ctx, const char *icount_name,
>>>    	}
>>>    	e2fsck_set_bitmap_type(ctx->fs, EXT2FS_BMAP64_RBTREE, icount_name,
>>>    			       &save_type);
>>> +	if (ctx->options & E2F_OPT_ICOUNT_FULLMAP)
>>> +		flags |= EXT2_ICOUNT_OPT_FULLMAP;
>>>    	retval = ext2fs_create_icount2(ctx->fs, flags, 0, hint, ret);
>>>    	ctx->fs->default_bitmap_type = save_type;
>>>    	return retval;
>> This isn't used for pass1 (I did originally use full_map either way then
>> decided to only use it for pass2) - and we to receive flags coming into this
>> function - shouldn't this perhaps rather go into pass2.c (e2fsck_pass2
>> function to be more specific)?  This really is cosmetic imho.
> The e2fsck_setup_icount() function is used for both pass1 and pass2.
>
>>> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
>>> index ff961483b..939862f1a 100644
>>> --- a/e2fsck/unix.c
>>> +++ b/e2fsck/unix.c
>>> @@ -709,9 +709,18 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
>>>    		} else if (strcmp(token, "nodiscard") == 0) {
>>>    			ctx->options &= ~E2F_OPT_DISCARD;
>>>    			continue;
>>> +		} else if (strcmp(token, "optimize_extents") == 0) {
>>> +			ctx->options &= ~E2F_OPT_NOOPT_EXTENTS;
>>> +			continue;
>> unrelated, but makes sense :).
> Yes, this was a cleanup I was doing as I weant.
>
>>> +		if (!retval) {
>>> +			memset(icount->fullmap, 0,
>>> +			*sizeof(__u32)*  * fs->super->s_inodes_count); <-------
>> this of course should be sizeof(__u16), or sizeof(*count->fullmap) which
>> will track the type pointed to.  It might be better practice to store
>> "sizeof(*icount->fullmap) * fs->super->s_inodes_count" to a variable and
>> just re-use it.
> Good point.  Fixed.
>
>>>    		goto errout;
>>> +
>> Extra line can be removed.
>>>    	if (flags & EXT2_ICOUNT_OPT_INCREMENT) {
> Removed.
>
> Thanks for taking a look at the patch!
>
> 						- Ted