| Message ID | 20171222120556.7435-17-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 22-12-17 07:05:53, Jeff Layton wrote: > From: Jeff Layton <jlayton@redhat.com> > > We only really need to update i_version if someone has queried for it > since we last incremented it. By doing that, we can avoid having to > update the inode if the times haven't changed. > > If the times have changed, then we go ahead and forcibly increment the > counter, under the assumption that we'll be going to the storage > anyway, and the increment itself is relatively cheap. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/inode.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 19e72f500f71..2fa920188759 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1635,17 +1635,21 @@ static int relatime_need_update(const struct path *path, struct inode *inode, > int generic_update_time(struct inode *inode, struct timespec *time, int flags) > { > int iflags = I_DIRTY_TIME; > + bool dirty = false; > > if (flags & S_ATIME) > inode->i_atime = *time; > if (flags & S_VERSION) > - inode_inc_iversion(inode); > + dirty |= inode_maybe_inc_iversion(inode, dirty); > if (flags & S_CTIME) > inode->i_ctime = *time; > if (flags & S_MTIME) > inode->i_mtime = *time; > + if ((flags & (S_ATIME | S_CTIME | S_MTIME)) && > + !(inode->i_sb->s_flags & SB_LAZYTIME)) > + dirty = true; When you pass 'dirty' to inode_maybe_inc_iversion(), it is always false. Maybe this condition should be at the beginning of the function? Once you fix that the patch looks good so you can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On Tue, 2018-01-02 at 17:50 +0100, Jan Kara wrote: > On Fri 22-12-17 07:05:53, Jeff Layton wrote: > > From: Jeff Layton <jlayton@redhat.com> > > > > We only really need to update i_version if someone has queried for it > > since we last incremented it. By doing that, we can avoid having to > > update the inode if the times haven't changed. > > > > If the times have changed, then we go ahead and forcibly increment the > > counter, under the assumption that we'll be going to the storage > > anyway, and the increment itself is relatively cheap. > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/inode.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 19e72f500f71..2fa920188759 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -1635,17 +1635,21 @@ static int relatime_need_update(const struct path *path, struct inode *inode, > > int generic_update_time(struct inode *inode, struct timespec *time, int flags) > > { > > int iflags = I_DIRTY_TIME; > > + bool dirty = false; > > > > if (flags & S_ATIME) > > inode->i_atime = *time; > > if (flags & S_VERSION) > > - inode_inc_iversion(inode); > > + dirty |= inode_maybe_inc_iversion(inode, dirty); > > if (flags & S_CTIME) > > inode->i_ctime = *time; > > if (flags & S_MTIME) > > inode->i_mtime = *time; > > + if ((flags & (S_ATIME | S_CTIME | S_MTIME)) && > > + !(inode->i_sb->s_flags & SB_LAZYTIME)) > > + dirty = true; > > When you pass 'dirty' to inode_maybe_inc_iversion(), it is always false. > Maybe this condition should be at the beginning of the function? Once you > fix that the patch looks good so you can add: > > Reviewed-by: Jan Kara <jack@suse.cz> > Thanks for the review! I've fixed it in my tree. I'll not re-post the set unless I have to make another significant change or someone requests it. I did make one other change, and that was to drop the "const" qualifiers on the integer arguments in the new API. David Howells pointed out that they don't really help anything, and the prototypes look cleaner without them. This set is now in linux-next as well, so I'm going to try to get this merged into v4.16, assuming no problems between now and the merge window.
diff --git a/fs/inode.c b/fs/inode.c index 19e72f500f71..2fa920188759 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1635,17 +1635,21 @@ static int relatime_need_update(const struct path *path, struct inode *inode, int generic_update_time(struct inode *inode, struct timespec *time, int flags) { int iflags = I_DIRTY_TIME; + bool dirty = false; if (flags & S_ATIME) inode->i_atime = *time; if (flags & S_VERSION) - inode_inc_iversion(inode); + dirty |= inode_maybe_inc_iversion(inode, dirty); if (flags & S_CTIME) inode->i_ctime = *time; if (flags & S_MTIME) inode->i_mtime = *time; + if ((flags & (S_ATIME | S_CTIME | S_MTIME)) && + !(inode->i_sb->s_flags & SB_LAZYTIME)) + dirty = true; - if (!(inode->i_sb->s_flags & SB_LAZYTIME) || (flags & S_VERSION)) + if (dirty) iflags |= I_DIRTY_SYNC; __mark_inode_dirty(inode, iflags); return 0; @@ -1864,7 +1868,7 @@ int file_update_time(struct file *file) if (!timespec_equal(&inode->i_ctime, &now)) sync_it |= S_CTIME; - if (IS_I_VERSION(inode)) + if (IS_I_VERSION(inode) && inode_iversion_need_inc(inode)) sync_it |= S_VERSION; if (!sync_it)