xfs
[Top] [All Lists]

Re: [PATCH] always set a/c/mtime through ->setattr

To: hch@xxxxxx
Subject: Re: [PATCH] always set a/c/mtime through ->setattr
From: Miklos Szeredi <miklos@xxxxxxxxxx>
Date: Tue, 20 May 2008 09:40:56 +0200
Cc: viro@xxxxxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, Artem.Bityutskiy@xxxxxxxxx
In-reply-to: <20080520060838.GA6436@xxxxxx> (message from Christoph Hellwig on Tue, 20 May 2008 08:08:38 +0200)
References: <20080520060838.GA6436@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
> Currently touch_atime and file_update_time directly update a/c/mtime
> in the inode and just mark the inode dirty afterwards.  This is pretty
> bad for some more complex filesystems that have various different types
> of dirtying an inode and/or need to store the data in another place
> for example for a buffer to be logged.
> 
> This patch changes touch_atime and file_update_time to not update the
> inode directly but rather call through ->setattr into the filessystem.

Do we know what effect this will have on read/write performance?  I
can imagine that some ->setattr() implementations are orders of
magnitude slower than just dirtying the inode.

> There is a new ATTR_UPDTIMES flag for these two calls so filesystems
> know it's just a timestampts update.  This allows some optimizations
> and also allow to kill the IS_NOCMTIME we curretly have for networked
> filesystem by letting them simpliy ignore these kind of updates.
> 
> There is also a new ATTR_VERSION flag sent from file_update_time
> that tells the filesystem to update i_version because this update
> has the same issues as the timestamp updates.
> 
> As a side-effect of the optimiation to not perfrom redundant timestamp
> updates has been moved from touch_atime and file_update_time to
> notify_change and thus applies to explicit utimes calls, too.

This optimization is fishy.  Remember, inode->i_*time are just cached
values, and the actual times on the (remote) filesystem itself can
differ.  Which means that we will now optimize out a "touch" because
we happened to have the current time cached in the inode.  Not that
this would be a likely event, but still...

So at least this check should be made dependent on ATTR_UPDTIMES, and
explicit time updates left alone.

Miklos


<Prev in Thread] Current Thread [Next in Thread>