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

[Dmitry Monakhov <dmonakhov@openvz.org>]

Nick Piggin <npiggin@suse.de> writes:

> On Mon, Feb 22, 2010 at 04:30:26PM +0300, Dmitry Monakhov wrote:
>> Nick Piggin <npiggin@suse.de> writes:
>> 
>> > On Mon, Feb 22, 2010 at 11:35:17AM +0100, Jan Kara wrote:
>> >> > 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 :)
>
> Oh, but you attempted it (partially?) by moving the inode size
> check into inode_change_ok().
>
>> As soon as i understand this patch add changes only for core part.
>> And later other filesystems will handle the rest.
>
> Yes. Most of them we have converted to the new sequence in
> subsequent patches. From that point it should be easier to improve
> error handling.
>
>> 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"
>  
> Hmm, I wonder if it would be safer to rename the function if
> changing behaviour like this so it loudly breaks external modules.
>
Yeah. Since your patch is mandatory as a start point. And i'm trying
to solve slightly  different issue. Which result in core patch pending.
Let it goes in to vfs tree as is. Later i'll convert all fsXXX_setattr()
to sane attribute checks logic.
ACK from me.