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