Re: [PATCH 0/7] vfs: notify_changes() error handling

[Dmitry Monakhov]

 Nick Piggin <npiggin@xxxxxxx> writes:

> On Mon, Feb 22, 2010 at 11:35:17AM +0100, Jan Kara wrote:
>>   Hi Dmitry,
>> 
>> > Current inode attr setting path looks like follows
>> > 
>> >  ret = inode_change_ok()
>> >  if(ret)
>> >      goto out;
>> >  /*
>> >   perform private-fs specific logic here
>> >   */
>> >  if(ia_valid & ATTR_UID || ...)
>> >     ret = vfs_dq_transfer()
>> > 
>> >  /*
>> >    more private-fs specific logic
>> >    for example update on_disk data structures.
>> >    */
>> > 
>> >   ret = inode_setattr()
>> > 
>> > In fact inode_setattr() call vmtruncate() which may fail in number
>> > of reasons IS_SWAPFILE, RLIMIT_FSIZE. After this many filesystem is
>> > unable to rollback changes. And just live inode in inconsistent
>> > state. We may check IS_SWAPFILE at the very beginning(currently it
>> > is not checked), but RLIMIT_FSIZE may changed under our feet.
>> > In order make things straight. Let's divide vmtruncate() in to
>> > two parts which perform all checks, and second which can not fail.
>> > After this notify_change() perform all necessary checks inside
>> > inode_change_ok() and simply call nofail version of vmtruncate().
>>   Actually, there are more problems than these in the truncate path.  Some
>> filesystems can decide to fail truncate only in their .truncate method but that
>> is called only after i_size is set which is too late.  Nick Piggin has a patch
>> set which was addressing this problem and should be basically a superset of
>> your changes. But I'm not sure whether the patch series is available somewhere
>> or what it's current status. Nick?
>
> I think Al is happy with it in principle, I changed it as he suggested.
> Maybe it was put on hold for other reasons. Anyway, here is the core
> patch again. It now is basically just adding more helpers, so it's not
> so intrusive.
>
> Al, what are your thoughts on merging? It doesn't appear to conflict
> with the -vfs tree.
>
> Dmitry, this doesn't solve your quota problem, but they obviously clash
> rather heavily. Do you see any problems with the way my patch goes?
In fact i dont tried to solve quota issue. I just want to understand
why error paths in my code becomes so huge and why they so differ
from existing code, now i do understand why :)
As soon as i understand this patch add changes only for core part.
And later other filesystems will handle the rest.
I am agree with it, vmtruncate is nightmare.
But imho also we have to solve generic inode attr check/set issue
fs_XXX_setattr()
{
...
   ret = inode_size_ok(inode, attr)
   if (ret)
        goto bad;
   if(private_fs_update_on_disk_data(new_size))
        goto bad;
   if(simple_setsize(inode,new_size)) {
     /* We still can get in to this because RLIMIT_FSIZE may be
      * changed after inode_size_ok();
      * So we have to roll back all fs-speciffic data which may be 
      * paintfull or impossible
      */
      ret = private_fs_update_on_disk_data(old_size)
      BUG_ON(ret)
    }
}
So my purpose is:
1) to move inode_size_ok check in to inode_change_ok()
2) Introduce simple_setsize_nocheck() which just do it's work
   after all checks was already done.
   And call simple_setsize_nocheck() inside fsXXX_setattr instead
   of simple_setsize().

Patch prepared agains yours "truncate: introduce new sequence"