[Ocfs2-tools-devel] [PATCH 2/5] Add "list-sparse" option in tunefs.ocfs2, take 2

[tao.ma]

Mark Fasheh wrote:
> On Wed, Oct 31, 2007 at 01:22:13PM +0800, tao.ma wrote:
>   
>>>> +		/*
>>>> +		 * If the tail of the file is a hole, let the
>>>> +		 * hole length only cover the last i_size.
>>>> +		 */
>>>> +		if (v_cluster + num_clusters == UINT32_MAX)
>>>> +			num_clusters = clusters - v_cluster;
>>>>     
>>>>         
>>> I think you want: 
>>> 	if (!p_cluster && (v_cluster + num_clusters == UINT32_MAX))
>>> 		num_clusters = cluster - v_cluster;
>>>
>>> Otherwise, we might concatenate the last extent length in a very large 
>>> file.
>>>   
>>>       
>> Does it really happen? Even if it happens, I think clusters - v_cluster 
>> really works since clusters come out of i_size and i_size should exists in 
>> the last allocated clusters, otherwise, the file is corrupted.
>> But you are somewhat right. The comment is a little confusing that lead to 
>> that situation. ;)
>> so I will modify it as you suggested below.
>>     
>
> I was mostly thinking of the code below that which added num_clusters to
> v_cluster to get the next lookup. In that case, we'd do multiple lookups in
> the same extent, which is more of an efficiency problem than anything else.
>   
Sorry, but I don't know clearly about what you said. In which situation 
do you think we will do multiple lookups in the same extents.
You mean a very large file which have allocated clusters and v_cluster + 
num_clusters > UINT32_MAX?
>   
>
>   
>>>> +	/*
>>>> +	 * We will use block cache in io, so that the extent block
>>>> +	 * information can be cached to speed up our extent iteration.
>>>> +	 * If io_init_cache failed, we will go on the list work without
>>>> +	 * the io_cache, so there is no check here.
>>>> +	 */
>>>> +	io_init_cache(fs->fs_io, ocfs2_extent_recs_per_eb(fs->fs_blocksize));
>>>>     
>>>>         
>>> I think it's best if we just do this up in the main body of tunefs.c 
>>> (after
>>> the cluster checks) so that all operations get the benefit of caching. It
>>> should also be in a seperate patch.
>>> 	--Mark
>>>
>>>   
>>>       
>> en, there is a problem of how many blocks tunefs wants. For list sparse, we 
>> need "ocfs2_extent_recs_per_eb(fs->fs_blocksize)", but for others they may 
>> only need 1 or 2 blocks. So I am not sure they can be organized together.
>>     
>
> I was thinking we'd just pick some sane defaults and go with that. I'm not
> really concerned much with memory usage of the cache at this point.
>
>
>   
>> And I think that it is also the reason why io cache isn't initialized by 
>> fs_io itself. In that case, all the block read/write can benefit from it. 
>> But that isn't Joel's original thought I think and the operation itself 
>> should handle the cache size.
>>     
>
> I think we didn't want to initialize it in fs_io because there was a worry
> that it wouldn't be correct for all possible usages (especially when we
> didn't have a cluster lock).
>   
Your explanation is fair. I will move the io_cache initialization to 
tunefs.c.