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

[tao.ma]

Mark Fasheh wrote:
> On Thu, Oct 25, 2007 at 03:36:25PM +0800, tao.ma wrote:
>   
>> +			      struct ocfs2_dinode *di,
>> +			      void (*func)(void *priv_data,
>> +					   uint32_t hole_start,
>> +					   uint32_t hole_len),
>> +			      void *priv_data)
>> +{
>> +	errcode_t ret;
>> +	uint32_t clusters, v_cluster = 0, p_cluster, num_clusters;
>> +	uint16_t extent_flags;
>> +	ocfs2_cached_inode *ci = NULL;
>> +
>> +	clusters = (di->i_size + fs->fs_clustersize -1 ) /
>> +			fs->fs_clustersize;
>> +
>> +	ret = ocfs2_read_cached_inode(fs, di->i_blkno, &ci);
>> +	if (ret)
>> +		goto bail;
>> +
>> +	while (v_cluster < clusters) {
>> +		ret = ocfs2_get_clusters(ci,
>> +					 v_cluster, &p_cluster,
>> +					 &num_clusters, &extent_flags);
>> +		if (ret)
>> +			goto bail;
>> +
>> +		/*
>> +		 * 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.
> Really, we could then just combine both the if statements into something
> like this:
>
> 	if (!p_cluster) {
> 		if (v_cluster + num_clusters == UINT32_MAX)
> 			num_clusters = cluster - v_cluster;
>
> 		if (func)
> 			func(priv_data, v_cluster, num_clusters);
> 	}
>   
>> +static errcode_t list_sparse_file(ocfs2_filesys *fs,
>> +				  struct ocfs2_dinode *di,
>> +				  struct list_ctxt *ctxt)
>> +{
>> +	errcode_t ret = 0;
>> +	struct multi_link_file *file = NULL;
>> +
>> +	assert(S_ISREG(di->i_mode));
>> +
>> +	ctxt->file_hole_len = 0;
>> +
>> +	if (di->i_links_count > 1) {
>> +		file = multi_link_file_lookup(ctxt, di->i_blkno);
>> +
>> +		if (file) {
>> +			ctxt->file_hole_len = file->clusters;
>> +			ctxt->duplicated = 1;
>> +			goto print;
>>     
>
> Shouldn't we skip printing the same inode number twice?
>   
We are iterating from "/" not all the inodes, remember? And we need to 
print the file name also.
So I am afraid whether the user want to see some file name are missing 
when he go through the whole output.
>
>   
>> +static int list_sparse_func(struct ocfs2_dir_entry *dirent,
>> +			    int offset, int blocksize,
>> +			    char *buf, void *priv_data)
>> +{
>> +	errcode_t ret;
>> +	char *di_buf = NULL;
>> +	struct ocfs2_dinode *di = NULL;
>> +	char file_name[OCFS2_MAX_FILENAME_LEN];
>> +	int file_name_len = 0;
>> +	struct list_ctxt *ctxt = (struct list_ctxt *)priv_data;
>> +	ocfs2_filesys *fs = ctxt->fs;
>> +
>> +	ret = ocfs2_malloc_block(fs->fs_io, &di_buf);
>> +	if (ret)
>> +		goto bail;
>> +
>> +	ret = ocfs2_read_inode(fs, (uint64_t)dirent->inode, di_buf);
>> +	if (ret)
>> +		goto bail;
>> +
>> +	di = (struct ocfs2_dinode *)di_buf;
>> +
>> +	/* currently, we only handle directories and regular files. */
>> +	if (!S_ISDIR(di->i_mode) && !S_ISREG(di->i_mode))
>> +		return 0;
>> +
>> +	strcpy(file_name, ctxt->file_name);
>> +	file_name_len = ctxt->file_name_len;
>> +
>> +	if (dirent->name_len + ctxt->file_name_len + 1 >=
>> +		OCFS2_MAX_FILENAME_LEN) {
>> +		goto bail;
>> +	}
>>     
>
> Can you explain this check to me? Should we be really comparing against
> PATH_MAX instead?
>   
My fault. Thanks.
>
>   
>> +/*
>> + * list_sparse will iterate from "/" and all the orphan_dirs recursively
>> + * and print out all the hole information on the screen.
>> + *
>> + * We will use ocfs2_dir_iterate to iterate from the very beginning and
>> + * tunefs_iterate_func will handle every dir entry.
>> + */
>> +errcode_t list_sparse(ocfs2_filesys *fs)
>> +{
>> +	int i;
>> +	errcode_t ret;
>> +	uint64_t blkno;
>> +	char file_name[OCFS2_MAX_FILENAME_LEN];
>> +	struct list_ctxt ctxt;
>> +	struct ocfs2_super_block *sb = OCFS2_RAW_SB(fs->fs_super);
>> +	uint32_t total_holes = 0, free_clusters = 0;
>> +
>> +	/*
>> +	 * 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.
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.
Joel, please clarify me if I misread your io_cache code.