On Mon, Jul 06, 2009 at 06:35:40AM -0400, Christoph Hellwig wrote: > On Mon, Jul 06, 2009 at 11:08:04AM +0200, Nick Piggin wrote: > > OK, hmm, but I wonder -- most of the time do_truncate will need to > > call notify_change anyway, so I wonder if avoiding the double > > indirection saves us anything? (It requires 2 indirect calls either > > way). And if we call ->setsize from ->setattr, then a filesystem > > which implements its own ->setattr could avoid one of those indirect > > calls. Not so if do_truncate has to call ->setattr then ->setsize. > > I don't quite understand what you mean here. In the end there should > be one single indirect call, ->setsize (or whatever it's called by > then). > > In the first round we'd split up a helper just for size updates from > notify_change, ala: > > int vfs_truncate(struct dentry *dentry, loff_t size, int flags, file) > { > int error; > > error = security_inode_truncate(dentry, size, flags, file); > if (error) > return error; > > if (inode->i_op->setsize) { > inode->i_op->setsize(dentry, size, flags, file); > > } else { > <... built up iattr here ...> > > if (inode->i_op->setattr) { > down_write(&dentry->d_inode->i_alloc_sem); > error = inode->i_op->setattr(dentry, attr); > up_write(&dentry->d_inode->i_alloc_sem); > } else { > down_write(&dentry->d_inode->i_alloc_sem); > error = inode_setattr(inode, attr); > up_write(&dentry->d_inode->i_alloc_sem); > } > } > > if (!error) > fsnotify_truncate(dentry, size, flags); > return error; > } > > One all filesistem are converted to have a setsize method (either their > own or simple_setsize) the !inode->i_op->setsize case can go away. > > Note that the above variant moves taking i_alloc_sem into ->setsize as > it's not required for most filesystems (I think only extN need for > O_DIRECT). > > Also the above doesn't deal with killing the SUID/SGID bits yet, we'll > need some good way for that. > > Actually it might be better to just pass the iattr to ->setsize to so > we can have the parsing for those arguments once, and that filesystems > can re-use parts of their ->setattr for ->setsize if it's complex enough > (timestamp updates and suid/sgid killing) ^^^^ Yes this was the problem I was thinking about. Because for exampe the truncate setattr call is also used for timestamp update as well as should_remove_setuid. The alternative to both ->setsize and ->setattr calls here is to reuse some of the ->setattr functionality in ->setsize as you say. So it will be a simpler change to call the new ->setsize from inside ->setattr. I guess that doesn't avoid your i_alloc_sem probem, but maybe we should move that into implementations if only a few filesystems require it. -- 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