On Sat 22-12-12 00:43:30, Andy Lutomirski wrote: > On Sat, Dec 22, 2012 at 12:29 AM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > NAK, we went through great trouble to get rid of the nasty layering > > violation where the VM called file_update_time directly just a short > > while ago, reintroducing that is a massive step back. > > > > Make sure whatever "solution" for your problem you come up with keeps > > the file update in the filesystem or generic helpers. > > > There's an inode operation ->update_time that is called (if it exists) > in these patches to update the time. Is that insufficient? Filesystems need more choice in when they modify time stamps because that can mean complex work like starting a transaction which has various locking implications. Just making a separate callback for this doesn't really solve the problem. Although I don't see particular problem with the call sites you chose Christoph is right we probably don't want to reintroduce updates of time stamps from generic code because eventually someone will have problem with that. > I could add a new inode operation ->modified_by_mmap that would be > called in mapping_flush_cmtime if that would be better. Not really. That's only uglier ;) > The original version of this patch did the update in ->writepage and > ->writepages, but that may have had lock ordering issues. (I wasn't > able to confirm that there was any actual problem.) Well, your call of mapping_flush_cmtime() from do_writepages() is easy to move to generic_writepages(). Thus filesystem can easily implement it's own ->writepages() callback if time update doesn't suit it. With the call from remove_vma() it is more problematic (and the calling context there is harder as well because we hold mmap_sem). We could maybe leave the call upto filesystem's ->release callback (and provide generic ->release handler which just calls mapping_flush_cmtime()). It won't be perfect because that gets called only after the last file descriptor for that struct file is closed (i.e., if a process forks and child inherits mappings, ->release gets called only after both parent and the child unmap the file) but it should catch 99% of the real world cases. Christoph, would the be OK with you? Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>