[Ocfs2-tools-devel] [PATCH 5/6] Resend: Add unwritten extent support in ocfs2-tools, take 2

[tao.ma]

Mark Fasheh wrote:
> On Wed, Sep 26, 2007 at 06:50:16PM -0700, Joel Becker wrote:
>   
>> On Thu, Sep 27, 2007 at 09:40:21AM +0800, tao.ma wrote:
>>     
>>>> 	Oho!  We have a problem.  In ocfs2_read_whole_file, we have a
>>>> reader function (read_whole_func) that doesn't check unwritten extents.
>>>> That needs to be added to patch #6.  Of course, how to tell the
>>>> function?
>>>> 	Also, extras/set_random_bits.c uses it.  I think that the
>>>> fsck.ocfs2 users are safe (symlinks and directories).
>>>>   
>>>>         
>>> Yes, that is what I worry about. the callback has no idea of whether the 
>>> block is marked as unwritten. So maybe as I have said previously, add a new 
>>> parameter in callback's definition that this block exists in an unwritten 
>>> extent?
>>> Do you have any suggestion or just agree with me? :)
>>>       
>> 	If it was just read, we could easily pass a flag and expect the
>> callback to do the right thing.  However, write is much harder.
>>     
>
> Passing a flags field is probably the right solution for reads.
>
> Block iteration in general is a very bad interface for doing data writes on
> ocfs2 though. There's a reason why we moved away from get_block based writes
> in kernel. In all fairness, it made no difference until we started
> supporting sparse files.
>
> ocfs2_extent_iterate_inode() doesn't handle holes either. And since it looks
> like ocfs2_block_iterate_inode() is based on it, I'll assume the same there.
> Hopefully all the read code is smart enough to at least put zero's where a
> hole or unwritten extent is.
>
>
>   
>> 	I don't quite know the solution.  The rule is, if you have an
>> unwritten extent, you must zero the parts you don't actually write.  But
>> block_iterate() is block-based.  It doesn't know from extents.  I think
>> that block_iterate() will have to know about it.  Somehow know that a
>> block was written to by the callback and thus zero around it.  Mark, any
>> thoughts?
>>     
>
> Disallow hole-filling or writes to unwritten regions via those functions?
>
> Honestly, there's not much else unless we teach them all how to mark an
> extent written or allocate clusters - and then they're not exactly block
> iteration functions any more ;)
>
> Imho we should have code which wants to read and write data to regular files
> just use ocfs2_file_read() and ocfs2_file_write(). There's still a place for
> block or extent iteration, but I think it's no longer suitable for basic
> file data i/o.
> 	--Mark
>   
OK, so let me generalize the consensus we make:
1. Add a new paremater to ocfs2_block_iterate like function, so that 
return zero if they need to read.
2. As for write, the user should really call ocfs2_file_write instead of 
these iterate things. ;)
3. As for extent record iteration, we really should not touch the flag 
to avoid splitting or merging. But we should have the ability the modify 
the cluster length like ocfs2_truncate. And it would work already with 
current code I think.