[Ocfs2-tools-devel] [PATCH 06/10] Dir operation modification for inline data.

[Tao Ma]

Joel Becker wrote:
> On Sat, Jul 12, 2008 at 10:13:07AM +0800, Tao Ma wrote:
>> Joel Becker wrote:
>>> On Fri, Jul 11, 2008 at 02:20:59PM +0800, Tao Ma wrote:
>>>> +	ocfs2_dinode_new_extent_list(fs, di);
>>>> +
>>>> +	di->i_size = 0;
>>>> +	di->i_ctime = di->i_mtime = time(NULL);
>>>> +
>>>> +	ret = ocfs2_write_inode(fs, dir, di_buf);
>>> 	This is a little weird, writing out a zero size, even
>>> temporarily.  If we crash, we lose the dir entries, right?
>> yes, you see ocfs2_insert_extent below needs us to send the blkno and  
>> read the inode info by itself, so I have to write it out here. by now, I  
>> haven't find a perfect way to handle this(no journal in user space is  
>> really painful).
> 
> 	I know why you did it, I just don't like the consequences.
> ocfs2_insert_extent() can't handle an i_size that isn't 0?
no, ocfs2_insert_extent need the initialized extent list. So I 
initialized it and write it out(actually it doesn't care i_size).
> 
>>>> @@ -128,15 +129,24 @@ errcode_t ocfs2_link(ocfs2_filesys *fs, uint64_t dir, const char *name,
>>>>  	ls.flags = flags;
>>>>  	ls.done = 0;
>>>>  	ls.sb = fs->fs_super;
>>>> -
>>>> +again:
>>>>  	retval = ocfs2_dir_iterate(fs, dir,
>>>>                                     OCFS2_DIRENT_FLAG_INCLUDE_EMPTY,
>>>>                                     NULL, link_proc, &ls);
>>>>  	if (retval)
>>>>  		return retval;
>>>>  -	if (!ls.done)
>>>> -		return OCFS2_ET_DIR_NO_SPACE;
>>>> +	if (!ls.done) {
>>>> +		if (allocation)
>>>> +			return OCFS2_ET_INTERNAL_FAILURE;
>>>> +
>>>> +		retval = ocfs2_expand_dir(fs, dir);
>>>> +		if (retval)
>>>> +			return retval;
>>>> +
>>>> +		allocation = 1;
>>>> +		goto again;
>>>> +	}
>>> 	Can we make this a loop instead of a goto?  It's not complex.
>> en, not agree, if we use loop, use "for" or "while"?
>> if with "for", then use "for(i=0;i<2;i++)" isn't looks as clear as this  
>> one. the same goes with "whle(i<2)".
> 
> 	Sure, but you don't even need to loop - you can just code it
> like we used to in other functions.  Saving 3 whole lines of code isn't
> worth gotos.  Here's a few ways to do it:
> 
> [Loop]
> 	do {
>  		retval = ocfs2_dir_iterate(fs, dir,
> 					   OCFS2_DIRENT_FLAG_INCLUDE_EMPTY,
> 					   NULL, link_proc, &ls);
> 		if (retval)
> 			return retval;
> 		if (ls.done)
> 			break;
> 		if (allocation)
> 			return OCFS2_ET_INTERNAL_FAILURE;
> 
> 		retval = ocfs2_expand_dir(fs, dir);
> 		if (retval)
> 			return retval;
> 
> 		allocation = 1;
> 	} while (1);
> 
> [No loop at all]
>  	retval = ocfs2_dir_iterate(fs, dir,
>                                    OCFS2_DIRENT_FLAG_INCLUDE_EMPTY,
>                                    NULL, link_proc, &ls);
> 	if (retval)
> 		return retval;
> 	if (!ls.done) {
> 		retval = ocfs2_expand_dir(fs, dir);
> 		if (retval)
> 			return retval;
> 
> 		retval = ocfs2_dir_iterate(fs, dir,
> 					   OCFS2_DIRENT_FLAG_INCLUDE_EMPTY,
> 					   NULL, link_proc, &ls);
> 		if (retval)
> 			return retval;
> 		if (!ls.done)
> 			return OCFS2_ET_INTERNAL_FAILURE;
> 	}
Yeah, I like the 2nd one. It is a little like what we used to call 
ocfs2_link/ocfs2_expand_dir, hehe. Thanks.

Regards,
Tao