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

[Eric Sandeen <sandeen@redhat.com>]

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;
>>
>>
>