Nick Piggin <npiggin@xxxxxxx> writes:
> On Mon, Feb 22, 2010 at 04:30:26PM +0300, Dmitry Monakhov wrote:
>> Nick Piggin <npiggin@xxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
