[Ocfs2-tools-devel] [patch 3/7] Adds slot remove mechanism in tunefs.ocfs2.

[tao.ma]

Joel Becker wrote:
> On Tue, Jun 12, 2007 at 02:44:02PM +0800, tao.ma at oracle.com wrote:
>   
>> +/*
>> + * This function will iterate the chain_rec and do the following modifications:
>> + * 1. modify  Sub Alloc Slot in extent block/inodes accordingly.
>> + * 2. change the GROUP_PARENT according to its future owner.
>> + * 3. link the chain to the new slot files.
>> + */
>> +static errcode_t move_chain_rec(ocfs2_filesys *fs, struct relink_ctxt *ctxt)
>>     
>
> Tao,
> 	Looking at this function, there are a couple of things that
> worry me.
> 	First, you walk an entire chain, changing all inodes or extent
> blocks.  Then, you move each group.  Like so:
>
>  1  for each group:
>  2      for each set bit:
>  3          modify suballoc slot
>  4      push group onto stack
>  5  while stack not empty:
>  6      pop group from stack
>  7      link group to new inode
>
> The thing that worries me is that you change every inode or extent block
> before you start moving the groups.  If you crash between lines 4 and 5,
> every metadata block in the chain has an incorrect suballoc slot.
> 	Now, fsck should be able to fix this.  However, if you modified
> the metadata blocks right before you move the group, you only have one
> group incorrect at a time:
>
>  1  for each group:
>  2      push group onto stack
>  3  while stack not empty:
>  4      pop group from stack
>  5      for each set bit:
>  6         modify suballoc slot
>  7      link group to new inode
>
> If you crash between 2 & 3, nothing has changed.  If you crash between 6
> & 7, only one group is incorrect.
> 	In this fashion, fsck would only have to fix one group of
> metadata blocks, instead of N.  It's faster.
>   
Cool and I will modify according to your advice.
I used to divide the complicated process into different steps so that 
the code is clear and a mistake isn't easy to happen :(. This is really 
a bad habit since now I am working on Linux kernel and tools, and the 
efficiency may be the most important thing for me. So great thanks for 
your advice. I will get used to thinking about how to improve the 
performance every time. ;)
>   
>> +		/* Record the group in the relink_ctxt.
>> +		 *
>> +		 * We record the group in a reverse order, so the first group
>> +		 * will be at the end of the group list. This is useful for
>> +		 * fsck.ocfs2 when any error happens during the move of groups
>> +		 * and we can safely move the group also.
>> +		 */
>> +		ret = ocfs2_malloc0(sizeof(struct moved_group), &group);
>> +		if (ret)
>> +			goto bail;
>> +
>> +		group->blkno = gd_blkno;
>> +		group->next = ctxt->groups;
>> +		ctxt->groups = group;
>> +
>> +		gd_blkno = gd->bg_next_group;
>>     
>
> 	Here's my second nitpick.  Why not save off the group block here? 
>
>                 ret = ocfs2_malloc_block(fs, &group->block);
>                 if (ret)
>                           goto bail;
>                 memcpy(group->block, ctxt->gd_buf, fs->fs_blocksize);
>
> Then you can avoid re-reading it in move_groups().  This should improve
> the performance of chain relocation by 10-30%.
>   
Yeah, you are right, another place to improve the efficiency. I will 
change it. Thanks.
>   
>> +	}
>> +
>> +	/* move the chain to the new slots. */
>> +	ret = move_groups(fs, ctxt);
>> +
>> +bail:
>> +	group = ctxt->groups;
>> +	while (group) {
>> +		group_next = group->next;
>>     
>
>                 if (group->block)
>                         ocfs2_free(&group->block);
>
>   
>> +		ocfs2_free(&group);
>> +		group = group_next;
>> +	}
>> +	ctxt->groups = NULL;
>> +	return ret;
>> +}
>>     
>
> Joel
>
>   
<http://www.oracle.com/cdc/>