[Ocfs2-tools-devel] [PATCH] Add extent flag as the parameter to block iteration function.

[Mark Fasheh]

On Wed, Oct 10, 2007 at 09:12:06AM +0800, tao.ma wrote:
> Mark Fasheh wrote:
>> On Tue, Oct 09, 2007 at 02:01:38PM +0800, tao.ma wrote:
>>   
>>> As we have added unwritten extent in ocfs2, block iteration function
>>> needs to know whether the content in the block is good. So add a new
>>> parameter in block iteration function.
>>>
>>> As for the block writting, please don't use these block iteration
>>> functions. Just use ocfs2_file_write since it knows everything about
>>> unwritten extents. ;)
>>>     
>>
>> Ok, most of that looks good - we're deprecating this api so adding the 
>> flags
>> is questionable, but I think in the end it's probably useful to complete 
>> it
>> in case the existing callers need a quick fix (for example, fsck should
>> probably check for and correct the unwritten flag in directory extents).
>>
>> Can you please move the OCFS2_EXTENT_*, OCFS2_BLOCK_* flag / return code 
>> definitions
>> and ocfs2_extent_iterate{_inode}/ocfs2_block_iterate{_inode} prototypes to
>> the bottom of libocfs2/include/ocfs2.h?
>>
>> Above them, we should add a comment indicating that the api is now
>> deprecated.
>>
>> Maybe something like this:
>>
>> /*
>>  * Deprecated extent/block iterate functions.
>>  */
>>
>>
>>   
> OK, no problem. I will move it.
>>> diff --git a/libocfs2/fileio.c b/libocfs2/fileio.c
>>> index 85730b2..d10d141 100644
>>> --- a/libocfs2/fileio.c
>>> +++ b/libocfs2/fileio.c
>>> @@ -45,12 +45,17 @@ struct read_whole_context {
>>>  static int read_whole_func(ocfs2_filesys *fs,
>>>  			   uint64_t blkno,
>>>  			   uint64_t bcount,
>>> +			   uint16_t ext_flags,
>>>  			   void *priv_data)
>>>  {
>>>  	struct read_whole_context *ctx = priv_data;
>>>  -	ctx->errcode = io_read_block(fs->fs_io, blkno,
>>> -				     1, ctx->ptr);
>>> +	if (ext_flags & OCFS2_EXT_UNWRITTEN) {
>>> +		memset(ctx->ptr, 0, fs->fs_blocksize);
>>> +		ctx->errcode = 0;
>>> +	} else
>>> +		ctx->errcode = io_read_block(fs->fs_io, blkno,
>>> +					     1, ctx->ptr);
>>>  	if (ctx->errcode)
>>>  		return OCFS2_BLOCK_ABORT;
>>>     
>>
>> Can you just fix up ocfs2_read_whole_file() to use ocfs2_file_read()
>> instead? This way it won't need any modification for inline-data.
>>   
> After I re-read the function of ocfs2_read_whole_file, I have some new 
> thought about it. If you go through the code, you will see that it only 
> allocate the di->i_clusters and iterate all the clusters and read them. So 
> considering that now we have sparse file support, what should we do if we 
> meet with a large hole? It is useless for us to read many number of zeros 
> from ocfs2_file_read. Thanks to god that after I go through all the caller 
> of this function in ocfs2-tools, it is only used for slot_map and heartbeat 
> files.

Well, returning a bunch of zero's is what's expected by the api - the same
thing happens when an application calls sys_read - it doesn't get an error
that there's a hole, it gets zero's in the buffer that was passed.


> I have to say this function should also be deprecated. Maybe I have to also 
> move its definition to the bottom of libocfs2/include/ocfs2.h since it 
> isn't suitable for sparse file now.
> So my suggestion is that just leave it as what I have modified. You don't 
> have to modify it for inline-data and I will move the definition to the 
> bottom of libocfs2/include/ocfs2.h. Is it OK?

How useful do we think ocfs2_read_whole_file() is? It really shouldn't be
too much to fix it up to just use ocfs2_file_read() directly. I guess the
different between this and the iterate functions is that the iterate
functions had no clear and obvious path to working with the new file system
features - this can still work pretty easily.
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh at oracle.com