From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anton Altaparmakov Subject: Re: [PATCH] replace inode_update_time with file_update_time Date: Mon, 7 Nov 2005 21:40:51 +0000 (GMT) Message-ID: References: <20051029165209.GA26446@lst.de> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: akpm@osdl.org, linux-fsdevel@vger.kernel.org Return-path: Received: from ppsw-7.csi.cam.ac.uk ([131.111.8.137]:32466 "EHLO ppsw-7.csi.cam.ac.uk") by vger.kernel.org with ESMTP id S965200AbVKGVkz (ORCPT ); Mon, 7 Nov 2005 16:40:55 -0500 To: Christoph Hellwig In-Reply-To: <20051029165209.GA26446@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Sat, 29 Oct 2005, Christoph Hellwig wrote: > To allow various options to work per-mount instead of per-sb we need What are those various options? Please spell them out. (I mean it! I really do not know what you have in mind and I cannot see anything that would require a vfs mount wrt cmtime updates.) > a struct vfsmount when updating ctime and mtime. This preparation > patch replaces the inode_update_time routine with a file_update_atime > routine so we can easily get at the vfsmount. (and the file makes more > sense in this context anyway). Also get rid of the unused second > argument - we always want to update the ctime when calling this routine. I believe your patch is wrong/unnecessary because the only option that is per superblock (or in your new world per vfsmount) is the IS_RDONLY(inode) check and if a mount is read-only the VFS should never have allowed inode/file_update_time() to be called. The codepath should have been aborted _much_ earlier than that so it does not make any sense to test for read-only here thus it is not necessary to do a per mountpoint check. Instead we should remove the readonly check altogether and if there are any places where an update of cmtime is attempted on a read-only mount we should fix those instead... A more detailed justification for your patch is IMO required before it be applied to mainline, especially as it breaks existing file systems like ntfs (as discussed offlist). Best regards, Anton > Signed-off-by: Christoph Hellwig > > Index: linux-2.6/fs/inode.c > =================================================================== > --- linux-2.6.orig/fs/inode.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/fs/inode.c 2005-10-29 04:14:47.000000000 +0200 > @@ -1203,16 +1203,16 @@ > EXPORT_SYMBOL(update_atime); > > /** > - * inode_update_time - update mtime and ctime time > - * @inode: inode accessed > - * @ctime_too: update ctime too > + * file_update_time - update mtime and ctime time > + * @file: file accessed > * > - * Update the mtime time on an inode and mark it for writeback. > - * When ctime_too is specified update the ctime too. > + * Update the mtime and ctime members of an inode and mark the inode > + * for writeback. > */ > > -void inode_update_time(struct inode *inode, int ctime_too) > +void file_update_time(struct file *file) > { > + struct inode *inode = file->f_dentry->d_inode; > struct timespec now; > int sync_it = 0; > > @@ -1226,16 +1226,15 @@ > sync_it = 1; > inode->i_mtime = now; > > - if (ctime_too) { > - if (!timespec_equal(&inode->i_ctime, &now)) > - sync_it = 1; > - inode->i_ctime = now; > - } > + if (!timespec_equal(&inode->i_ctime, &now)) > + sync_it = 1; > + inode->i_ctime = now; > + > if (sync_it) > mark_inode_dirty_sync(inode); > } > > -EXPORT_SYMBOL(inode_update_time); > +EXPORT_SYMBOL(file_update_time); > > int inode_needs_sync(struct inode *inode) > { > Index: linux-2.6/fs/ncpfs/file.c > =================================================================== > --- linux-2.6.orig/fs/ncpfs/file.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/fs/ncpfs/file.c 2005-10-29 04:14:47.000000000 +0200 > @@ -262,7 +262,7 @@ > } > vfree(bouncebuffer); > > - inode_update_time(inode, 1); > + file_update_time(file); > > *ppos = pos; > > Index: linux-2.6/fs/pipe.c > =================================================================== > --- linux-2.6.orig/fs/pipe.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/fs/pipe.c 2005-10-29 04:14:47.000000000 +0200 > @@ -347,7 +347,7 @@ > kill_fasync(PIPE_FASYNC_READERS(*inode), SIGIO, POLL_IN); > } > if (ret > 0) > - inode_update_time(inode, 1); /* mtime and ctime */ > + file_update_time(filp); > return ret; > } > > Index: linux-2.6/fs/reiserfs/file.c > =================================================================== > --- linux-2.6.orig/fs/reiserfs/file.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/fs/reiserfs/file.c 2005-10-29 04:14:47.000000000 +0200 > @@ -1360,7 +1360,7 @@ > if (res) > goto out; > > - inode_update_time(inode, 1); /* Both mtime and ctime */ > + file_update_time(file); > > // Ok, we are done with all the checks. > > Index: linux-2.6/fs/xfs/linux-2.6/xfs_lrw.c > =================================================================== > --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_lrw.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/fs/xfs/linux-2.6/xfs_lrw.c 2005-10-29 04:14:47.000000000 +0200 > @@ -740,7 +740,7 @@ > */ > if (!(ioflags & IO_INVIS)) { > xfs_ichgtime(xip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); > - inode_update_time(inode, 1); > + file_update_time(file); > } > > /* > Index: linux-2.6/include/linux/fs.h > =================================================================== > --- linux-2.6.orig/include/linux/fs.h 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/include/linux/fs.h 2005-10-29 04:15:01.000000000 +0200 > @@ -1651,7 +1651,7 @@ > extern int inode_change_ok(struct inode *, struct iattr *); > extern int __must_check inode_setattr(struct inode *, struct iattr *); > > -extern void inode_update_time(struct inode *inode, int ctime_too); > +extern void file_update_time(struct file *file); > > static inline ino_t parent_ino(struct dentry *dentry) > { > Index: linux-2.6/mm/filemap.c > =================================================================== > --- linux-2.6.orig/mm/filemap.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/mm/filemap.c 2005-10-29 04:14:47.000000000 +0200 > @@ -2066,7 +2066,7 @@ > if (err) > goto out; > > - inode_update_time(inode, 1); > + file_update_time(file); > > /* coalesce the iovecs and go direct-to-BIO for O_DIRECT */ > if (unlikely(file->f_flags & O_DIRECT)) { > Index: linux-2.6/mm/filemap_xip.c > =================================================================== > --- linux-2.6.orig/mm/filemap_xip.c 2005-10-29 04:14:44.000000000 +0200 > +++ linux-2.6/mm/filemap_xip.c 2005-10-29 04:14:47.000000000 +0200 > @@ -381,7 +381,7 @@ > if (ret) > goto out_backing; > > - inode_update_time(inode, 1); > + file_update_time(filp); > > ret = __xip_file_write (filp, buf, count, pos, ppos); > > - > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards, Anton -- Anton Altaparmakov (replace at with @) Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/