2005-11-09 22:48:51

by Anton Altaparmakov

[permalink] [raw]
Subject: [PATCH] Remove read-only check from inode_update_time().

Hi Andrew,

The read-only check in inode_update_time() (or file_update_time() as it is
now in -mm) is unnecessary as the VFS better have done all the read-only
checks and aborted much earlier in the file write code paths where
inode/file_update_time() is only called from.

(In case you were not following the ntfs discussion, Christoph Hellwig
agreed that check is unnecessary and can be removed.)

Patch against latest Linus git tree is below, please apply. If you prefer
a patch on top of Christoph's file_update_time() check please let me
know...

Best regards,

Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (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/

---

The read-only check in inode_update_time() (or file_update_time() as it is
now in -mm) is unnecessary as the VFS better have done all the read-only
checks and aborted much earlier in the file write code paths where
inode/file_update_time() is only called from.

Signed-off-by: Anton Altaparmakov <[email protected]>

--- inode.c 2005-11-09 19:23:35.000000000 +0000
+++ inode.c.new 2005-11-09 22:45:21.000000000 +0000
@@ -1219,8 +1219,6 @@ void inode_update_time(struct inode *ino

if (IS_NOCMTIME(inode))
return;
- if (IS_RDONLY(inode))
- return;

now = current_fs_time(inode->i_sb);
if (!timespec_equal(&inode->i_mtime, &now))


2005-11-09 23:04:15

by Dave Kleikamp

[permalink] [raw]
Subject: Re: [PATCH] Remove read-only check from inode_update_time().

On Wed, 2005-11-09 at 22:48 +0000, Anton Altaparmakov wrote:
> Hi Andrew,
>
> The read-only check in inode_update_time() (or file_update_time() as it is
> now in -mm) is unnecessary as the VFS better have done all the read-only
> checks and aborted much earlier in the file write code paths where
> inode/file_update_time() is only called from.

I notice inode_update_time is called from pipe_writev. I don't know how
likely it would be in practice, but wouldn't it be possible to write to
a pipe on a read-only partition? In that case the read-only check still
makes sense.

> (In case you were not following the ntfs discussion, Christoph Hellwig
> agreed that check is unnecessary and can be removed.)
>
> Patch against latest Linus git tree is below, please apply. If you prefer
> a patch on top of Christoph's file_update_time() check please let me
> know...
>
> Best regards,
>
> Anton
--
David Kleikamp
IBM Linux Technology Center

2005-11-09 23:23:09

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [PATCH] Remove read-only check from inode_update_time().

On Wed, 9 Nov 2005, Dave Kleikamp wrote:
> On Wed, 2005-11-09 at 22:48 +0000, Anton Altaparmakov wrote:
> > Hi Andrew,
> >
> > The read-only check in inode_update_time() (or file_update_time() as it is
> > now in -mm) is unnecessary as the VFS better have done all the read-only
> > checks and aborted much earlier in the file write code paths where
> > inode/file_update_time() is only called from.
>
> I notice inode_update_time is called from pipe_writev. I don't know how
> likely it would be in practice, but wouldn't it be possible to write to
> a pipe on a read-only partition? In that case the read-only check still
> makes sense.

It would still make sense but only if you can write to a pipe on a
read-only partition which I have always assumed is not possible.

However, now that you queried this, I went and tried it and yes, you can
write to a named pipe after remounting read-only, so you are right, the
check does make sense in this case. One learns something new every day.
(-:

Andrew, please do not apply my patch...

Best regards,

Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (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/