linux-ext4 - Re: [PATCH 1/2 v2] e2fsck: Discard only unused parts of inode table

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 01 Mar 2012 11:35:59 -0600
From:	Eric Sandeen <sandeen@...hat.com>
To:	Lukas Czerner <lczerner@...hat.com>
CC:	Eric Sandeen <sandeen@...deen.net>, linux-ext4@...r.kernel.org,
	tytso@....edu, psusi@...ntu.com
Subject: Re: [PATCH 1/2 v2] e2fsck: Discard only unused parts of inode table

On 2/29/12 1:22 AM, Lukas Czerner wrote:
> On Tue, 28 Feb 2012, Eric Sandeen wrote:

...

>>> +			/*
>>> +			 * If the last inode is free, we can discard it as well.
>>> +			 */
>>> +			if (inodes >= first_free)
>>> +				e2fsck_discard_inodes(ctx, group, first_free,
>>> +						      inodes - first_free + 1);
>>
>> This is unrelated to the changelog
>>
>>>  			/*
>>>  			 * 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)) &&
>>> +			    !(ctx->options & E2F_OPT_NO) &&
>>
>> This is unrelated as well...
> 
> So do you want me to separate those unrelated changes to a separate
> patch ? Those are pretty small changes..

They are, but they are completely separate fixes, and not described in the changelog.

Ideally I'd say they should be a separate testable commit, but at least it
should be mentioned in the changelog (and patch summary).

Sorry, compound commits with unrelated changes are my pet peeve; they make bisecting
harder, and make it harder to find fixes when looking back over git history.

-Eric

>>
>> Rather than continuing to check _OPT_NO here and in e2fsck_discard_blocks(), it
>> might be better (in a separate patch) ;) to simply turn off OPT_DISCARD right
>> after options processing if -n was specified, and not worry about it down here.
> 
> Makes sense, I can change it.
> 
> Thanks!
> -Lukas
> 
>>
>>> +			    io_channel_discard_zeroes_data(fs->io) &&
>>>  			    !(ext2fs_bg_flags_test(fs, group,
>>> -						  EXT2_BG_INODE_ZEROED))) {
>>> +						   EXT2_BG_INODE_ZEROED))) {
>>>  				ext2fs_bg_flags_set(fs, group,
>>>  						    EXT2_BG_INODE_ZEROED);
>>>  				ext2fs_group_desc_csum_set(fs, group);
>>>  			}
>>>  
>>> +			first_free = fs->super->s_inodes_per_group + 1;
>>> +			free_array[group] = group_free;
>>> +			dir_array[group] = dirs_count;
>>>  			group ++;
>>>  			inodes = 0;
>>>  			skip_group = 0;
>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html