Re: + fs-prepare_write-fixes.patch added to -mm tree

Re: + fs-prepare_write-fixes.patch added to -mm tree

[Nick Piggin]

Mark Fasheh wrote:
> On Thu, Oct 19, 2006 at 07:25:37AM +0200, Nick Piggin wrote:
> 
>>I sent an RFC to linux-fsdevel, did you get that?
> 
> Yeah, I don't think I thought of my concerns at the time.
> 
> 
> 
>>I was planning to cc some maintainers, including you, for those
>>filesystems that are non-trivial. I just hadn't had a chance to
>>test it properly last night.
> 
> Cool, I appreciate that.

OK, I will be posting that mail tomorrow or next day... I'll summarise
your concerns you've posted in this thread too.


>>OK thanks for looking at that. If the length of the commit is greater
>>than 0 (but still short), then the page is uptodate so it should be
>>fine to commit what we have written, I think?
> 
> That seems to make sense to me.
> 
>  
> 
>>If the length is zero, then we probably want to roll back entirely.
> 
> The thing is, rollback might be hard (or expensive) for some file systems
> with more complicated tree implementations, etc.
> 
> Do we have the option in this case of just zeroing the newly allocated
> portions and writing them out? Userspace would then just be seeing
> that like any other hole.

Sure that's possible. You could even recognise that it is a hole in your
prepare_write, and zero the page and set it uptodate there.

You probably don't actually want to do that, because it means a double
overwrite in the case of a page sized,aligned write... but you have a
fairly broad scope of what you can do here. You are holding i_mutex and
the page lock, and the rest is up to you.

zeroing out the hole and marking it uptodate in case of a 0 length
->commit_write does sound like the right way to go. I probably haven't
handled that correctly if it needs to be done in ext? or generic fs/
routines...

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: + fs-prepare_write-fixes.patch added to -mm tree

[Mark Fasheh]

On Fri, Oct 20, 2006 at 09:27:02AM +1000, Nick Piggin wrote:
> >Cool, I appreciate that.
> 
> OK, I will be posting that mail tomorrow or next day... I'll summarise
> your concerns you've posted in this thread too.
Thanks.


> zeroing out the hole and marking it uptodate in case of a 0 length
> ->commit_write does sound like the right way to go. I probably haven't
> handled that correctly if it needs to be done in ext? or generic fs/
> routines...
I *think* we want to handle this in generic_file_buffered_write(). Between
->prepare_write() and ->commit_write(). Here's what I'm thinking:

generic_file_buffered_write() notices that it got a short copy from
copy_from_user_inatomic(). It can then call a helper which walks the buffer
heads attached to the page looking for BH_New regions which haven't been
written to yet. It can then zero those out. We pass the normal from/to
arguments over to ->commit_write() and those callbacks don't have to change
- they just continue as usual. The newly allocated regions get written out,
filling the holes with valid data and we avoid returning garbage from disk
on subsequent reads.

Ideas? "Holes" in the design? :)
	--Mark

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