| Message ID | 20220816131522.42467-1-jlayton@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: fix i_version handling in ext4 | expand |
On Tue, Aug 16, 2022 at 09:15:22AM -0400, Jeff Layton wrote: > ext4 currently updates the i_version counter when the atime is updated > during a read. This is less than ideal as it can cause unnecessary cache > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > problematic since it can also corrupt the i_version counter for > ea_inodes. > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > the i_version there seems wrong, and is the cause of both problems. > Remove that callsite and add increments to the setattr and setxattr > codepaths (at the same time that we update the ctime). The i_version > bump that already happens during timestamp updates should take care of > the rest. > > Cc: Lukas Czerner <lczerner@redhat.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Christian Brauner <brauner@kernel.org> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- Seems good to me. But it seems that the xfs patch you sent does have inode_inc_version() right after setattr_copy() as well. So I wonder if we couldn't just try and move inode_inc_version() into setattr_copy() itself. > fs/ext4/inode.c | 10 +++++----- > fs/ext4/xattr.c | 2 ++ > 2 files changed, 7 insertions(+), 5 deletions(-) > > I think this patch should probably supersede Lukas' patch entitled: > > ext4: don't increase iversion counter for ea_inodes > > This will also mean that we'll need to respin the patch to turn on the > i_version counter unconditionally in ext4 (though that should be > trivial). > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 601214453c3a..a70921df89a5 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > int error, rc = 0; > int orphan = 0; > const unsigned int ia_valid = attr->ia_valid; > + bool inc_ivers = IS_IVERSION(inode); > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > return -EIO; > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > return -EINVAL; > } > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > - inode_inc_iversion(inode); > + if (attr->ia_size == inode->i_size) > + inc_ivers = false; > > if (shrink) { > if (ext4_should_order_data(inode)) { > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > } > > if (!error) { > + if (inc_ivers) > + inode_inc_iversion(inode); > setattr_copy(mnt_userns, inode, attr); > mark_inode_dirty(inode); > } > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > } > ext4_fc_track_inode(handle, inode); > > - if (IS_I_VERSION(inode)) > - inode_inc_iversion(inode); > - > /* the do_update_inode consumes one bh->b_count */ > get_bh(iloc->bh); > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 533216e80fa2..4d84919d1c9c 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > if (!error) { > ext4_xattr_update_super_block(handle, inode->i_sb); > inode->i_ctime = current_time(inode); > + if (IS_IVERSION(inode)) > + inode_inc_iversion(inode); > if (!value) > no_expand = 0; > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > -- > 2.37.2 >
On Tue, 2022-08-16 at 15:33 +0200, Christian Brauner wrote: > On Tue, Aug 16, 2022 at 09:15:22AM -0400, Jeff Layton wrote: > > ext4 currently updates the i_version counter when the atime is updated > > during a read. This is less than ideal as it can cause unnecessary cache > > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > > problematic since it can also corrupt the i_version counter for > > ea_inodes. > > > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > > the i_version there seems wrong, and is the cause of both problems. > > Remove that callsite and add increments to the setattr and setxattr > > codepaths (at the same time that we update the ctime). The i_version > > bump that already happens during timestamp updates should take care of > > the rest. > > > > Cc: Lukas Czerner <lczerner@redhat.com> > > Cc: Jan Kara <jack@suse.cz> > > Cc: Christian Brauner <brauner@kernel.org> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > Seems good to me. But it seems that the xfs patch you sent does have > inode_inc_version() right after setattr_copy() as well. So I wonder if > we couldn't just try and move inode_inc_version() into setattr_copy() > itself. > We probably could, but setattr_copy has a lot of callers and most filesystems don't need this. Also, there are some cases where we don't want to update the i_version after a setattr. In particular, if you do a truncate and the size doesn't change, then you really don't want to update the timestamps (and therefore the i_version shouldn't change either). > > fs/ext4/inode.c | 10 +++++----- > > fs/ext4/xattr.c | 2 ++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > I think this patch should probably supersede Lukas' patch entitled: > > > > ext4: don't increase iversion counter for ea_inodes > > > > This will also mean that we'll need to respin the patch to turn on the > > i_version counter unconditionally in ext4 (though that should be > > trivial). > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index 601214453c3a..a70921df89a5 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > int error, rc = 0; > > int orphan = 0; > > const unsigned int ia_valid = attr->ia_valid; > > + bool inc_ivers = IS_IVERSION(inode); > > > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > return -EIO; > > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > return -EINVAL; > > } > > > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > > - inode_inc_iversion(inode); > > + if (attr->ia_size == inode->i_size) > > + inc_ivers = false; > > > > if (shrink) { > > if (ext4_should_order_data(inode)) { > > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > } > > > > if (!error) { > > + if (inc_ivers) > > + inode_inc_iversion(inode); > > setattr_copy(mnt_userns, inode, attr); > > mark_inode_dirty(inode); > > } > > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > > } > > ext4_fc_track_inode(handle, inode); > > > > - if (IS_I_VERSION(inode)) > > - inode_inc_iversion(inode); > > - > > /* the do_update_inode consumes one bh->b_count */ > > get_bh(iloc->bh); > > > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > > index 533216e80fa2..4d84919d1c9c 100644 > > --- a/fs/ext4/xattr.c > > +++ b/fs/ext4/xattr.c > > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > > if (!error) { > > ext4_xattr_update_super_block(handle, inode->i_sb); > > inode->i_ctime = current_time(inode); > > + if (IS_IVERSION(inode)) > > + inode_inc_iversion(inode); > > if (!value) > > no_expand = 0; > > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > > -- > > 2.37.2 > >
On Tue, Aug 16, 2022 at 09:43:16AM -0400, Jeff Layton wrote: > On Tue, 2022-08-16 at 15:33 +0200, Christian Brauner wrote: > > On Tue, Aug 16, 2022 at 09:15:22AM -0400, Jeff Layton wrote: > > > ext4 currently updates the i_version counter when the atime is updated > > > during a read. This is less than ideal as it can cause unnecessary cache > > > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > > > problematic since it can also corrupt the i_version counter for > > > ea_inodes. > > > > > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > > > the i_version there seems wrong, and is the cause of both problems. > > > Remove that callsite and add increments to the setattr and setxattr > > > codepaths (at the same time that we update the ctime). The i_version > > > bump that already happens during timestamp updates should take care of > > > the rest. > > > > > > Cc: Lukas Czerner <lczerner@redhat.com> > > > Cc: Jan Kara <jack@suse.cz> > > > Cc: Christian Brauner <brauner@kernel.org> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > > Seems good to me. But it seems that the xfs patch you sent does have > > inode_inc_version() right after setattr_copy() as well. So I wonder if > > we couldn't just try and move inode_inc_version() into setattr_copy() > > itself. > > > > We probably could, but setattr_copy has a lot of callers and most > filesystems don't need this. Also, there are some cases where we don't > want to update the i_version after a setattr. > > In particular, if you do a truncate and the size doesn't change, then > you really don't want to update the timestamps (and therefore the > i_version shouldn't change either). We could probably all handle that with some massaging but I'm also fine with doing it just for ext4 and xfs if these are the only ones where this is relevant: Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
On Tue 16-08-22 09:15:22, Jeff Layton wrote: > ext4 currently updates the i_version counter when the atime is updated > during a read. This is less than ideal as it can cause unnecessary cache > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > problematic since it can also corrupt the i_version counter for > ea_inodes. > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > the i_version there seems wrong, and is the cause of both problems. > Remove that callsite and add increments to the setattr and setxattr > codepaths (at the same time that we update the ctime). The i_version > bump that already happens during timestamp updates should take care of > the rest. > > Cc: Lukas Czerner <lczerner@redhat.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Christian Brauner <brauner@kernel.org> > Signed-off-by: Jeff Layton <jlayton@kernel.org> After some verification (which was not completely trivial e.g. for directories) I agree all cases should be covered. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/inode.c | 10 +++++----- > fs/ext4/xattr.c | 2 ++ > 2 files changed, 7 insertions(+), 5 deletions(-) > > I think this patch should probably supersede Lukas' patch entitled: > > ext4: don't increase iversion counter for ea_inodes > > This will also mean that we'll need to respin the patch to turn on the > i_version counter unconditionally in ext4 (though that should be > trivial). > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 601214453c3a..a70921df89a5 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > int error, rc = 0; > int orphan = 0; > const unsigned int ia_valid = attr->ia_valid; > + bool inc_ivers = IS_IVERSION(inode); > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > return -EIO; > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > return -EINVAL; > } > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > - inode_inc_iversion(inode); > + if (attr->ia_size == inode->i_size) > + inc_ivers = false; > > if (shrink) { > if (ext4_should_order_data(inode)) { > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > } > > if (!error) { > + if (inc_ivers) > + inode_inc_iversion(inode); > setattr_copy(mnt_userns, inode, attr); > mark_inode_dirty(inode); > } > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > } > ext4_fc_track_inode(handle, inode); > > - if (IS_I_VERSION(inode)) > - inode_inc_iversion(inode); > - > /* the do_update_inode consumes one bh->b_count */ > get_bh(iloc->bh); > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 533216e80fa2..4d84919d1c9c 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > if (!error) { > ext4_xattr_update_super_block(handle, inode->i_sb); > inode->i_ctime = current_time(inode); > + if (IS_IVERSION(inode)) > + inode_inc_iversion(inode); > if (!value) > no_expand = 0; > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > -- > 2.37.2 >
On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote: > On Tue 16-08-22 09:15:22, Jeff Layton wrote: > > ext4 currently updates the i_version counter when the atime is updated > > during a read. This is less than ideal as it can cause unnecessary cache > > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > > problematic since it can also corrupt the i_version counter for > > ea_inodes. > > > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > > the i_version there seems wrong, and is the cause of both problems. > > Remove that callsite and add increments to the setattr and setxattr > > codepaths (at the same time that we update the ctime). The i_version > > bump that already happens during timestamp updates should take care of > > the rest. > > > > Cc: Lukas Czerner <lczerner@redhat.com> > > Cc: Jan Kara <jack@suse.cz> > > Cc: Christian Brauner <brauner@kernel.org> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > After some verification (which was not completely trivial e.g. for > directories) I agree all cases should be covered. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Honza > Thanks. I think this covers the typical cases, but there are some places I missed: The setacl codepath, for one, and there are a number of places that set the ctime explicitly for hole punching and the like. I'm planning to send a v2 once I do a bit more testing. I'll hold off on adding your Reviewed-by just yet, since the final patch may be quite a bit different. > > --- > > fs/ext4/inode.c | 10 +++++----- > > fs/ext4/xattr.c | 2 ++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > I think this patch should probably supersede Lukas' patch entitled: > > > > ext4: don't increase iversion counter for ea_inodes > > > > This will also mean that we'll need to respin the patch to turn on the > > i_version counter unconditionally in ext4 (though that should be > > trivial). > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index 601214453c3a..a70921df89a5 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > int error, rc = 0; > > int orphan = 0; > > const unsigned int ia_valid = attr->ia_valid; > > + bool inc_ivers = IS_IVERSION(inode); > > > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > return -EIO; > > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > return -EINVAL; > > } > > > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > > - inode_inc_iversion(inode); > > + if (attr->ia_size == inode->i_size) > > + inc_ivers = false; > > > > if (shrink) { > > if (ext4_should_order_data(inode)) { > > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > } > > > > if (!error) { > > + if (inc_ivers) > > + inode_inc_iversion(inode); > > setattr_copy(mnt_userns, inode, attr); > > mark_inode_dirty(inode); > > } > > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > > } > > ext4_fc_track_inode(handle, inode); > > > > - if (IS_I_VERSION(inode)) > > - inode_inc_iversion(inode); > > - > > /* the do_update_inode consumes one bh->b_count */ > > get_bh(iloc->bh); > > > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > > index 533216e80fa2..4d84919d1c9c 100644 > > --- a/fs/ext4/xattr.c > > +++ b/fs/ext4/xattr.c > > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > > if (!error) { > > ext4_xattr_update_super_block(handle, inode->i_sb); > > inode->i_ctime = current_time(inode); > > + if (IS_IVERSION(inode)) > > + inode_inc_iversion(inode); > > if (!value) > > no_expand = 0; > > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > > -- > > 2.37.2 > >
On Wed 17-08-22 09:09:58, Jeff Layton wrote: > On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote: > > On Tue 16-08-22 09:15:22, Jeff Layton wrote: > > > ext4 currently updates the i_version counter when the atime is updated > > > during a read. This is less than ideal as it can cause unnecessary cache > > > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > > > problematic since it can also corrupt the i_version counter for > > > ea_inodes. > > > > > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > > > the i_version there seems wrong, and is the cause of both problems. > > > Remove that callsite and add increments to the setattr and setxattr > > > codepaths (at the same time that we update the ctime). The i_version > > > bump that already happens during timestamp updates should take care of > > > the rest. > > > > > > Cc: Lukas Czerner <lczerner@redhat.com> > > > Cc: Jan Kara <jack@suse.cz> > > > Cc: Christian Brauner <brauner@kernel.org> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > After some verification (which was not completely trivial e.g. for > > directories) I agree all cases should be covered. Feel free to add: > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > Honza > > > > Thanks. > > I think this covers the typical cases, but there are some places I > missed: > > The setacl codepath, for one, and there are a number of places that set > the ctime explicitly for hole punching and the like. Hum, why is setacl() not covered by your change to ext4_xattr_set_handle()? ext4_set_acl() ends up calling it... I have checked hole punching (whole ext4_fallocate()) and it seems to be incrementing iversion where needed. > I'm planning to > send a v2 once I do a bit more testing. I'll hold off on adding your > Reviewed-by just yet, since the final patch may be quite a bit > different. OK, thanks! Honza > > > > > --- > > > fs/ext4/inode.c | 10 +++++----- > > > fs/ext4/xattr.c | 2 ++ > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > I think this patch should probably supersede Lukas' patch entitled: > > > > > > ext4: don't increase iversion counter for ea_inodes > > > > > > This will also mean that we'll need to respin the patch to turn on the > > > i_version counter unconditionally in ext4 (though that should be > > > trivial). > > > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > > index 601214453c3a..a70921df89a5 100644 > > > --- a/fs/ext4/inode.c > > > +++ b/fs/ext4/inode.c > > > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > int error, rc = 0; > > > int orphan = 0; > > > const unsigned int ia_valid = attr->ia_valid; > > > + bool inc_ivers = IS_IVERSION(inode); > > > > > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > > return -EIO; > > > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > return -EINVAL; > > > } > > > > > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > > > - inode_inc_iversion(inode); > > > + if (attr->ia_size == inode->i_size) > > > + inc_ivers = false; > > > > > > if (shrink) { > > > if (ext4_should_order_data(inode)) { > > > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > } > > > > > > if (!error) { > > > + if (inc_ivers) > > > + inode_inc_iversion(inode); > > > setattr_copy(mnt_userns, inode, attr); > > > mark_inode_dirty(inode); > > > } > > > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > > > } > > > ext4_fc_track_inode(handle, inode); > > > > > > - if (IS_I_VERSION(inode)) > > > - inode_inc_iversion(inode); > > > - > > > /* the do_update_inode consumes one bh->b_count */ > > > get_bh(iloc->bh); > > > > > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > > > index 533216e80fa2..4d84919d1c9c 100644 > > > --- a/fs/ext4/xattr.c > > > +++ b/fs/ext4/xattr.c > > > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > > > if (!error) { > > > ext4_xattr_update_super_block(handle, inode->i_sb); > > > inode->i_ctime = current_time(inode); > > > + if (IS_IVERSION(inode)) > > > + inode_inc_iversion(inode); > > > if (!value) > > > no_expand = 0; > > > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > > > -- > > > 2.37.2 > > > > > -- > Jeff Layton <jlayton@kernel.org>
On Wed, 2022-08-17 at 15:25 +0200, Jan Kara wrote: > On Wed 17-08-22 09:09:58, Jeff Layton wrote: > > On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote: > > > On Tue 16-08-22 09:15:22, Jeff Layton wrote: > > > > ext4 currently updates the i_version counter when the atime is updated > > > > during a read. This is less than ideal as it can cause unnecessary cache > > > > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > > > > problematic since it can also corrupt the i_version counter for > > > > ea_inodes. > > > > > > > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > > > > the i_version there seems wrong, and is the cause of both problems. > > > > Remove that callsite and add increments to the setattr and setxattr > > > > codepaths (at the same time that we update the ctime). The i_version > > > > bump that already happens during timestamp updates should take care of > > > > the rest. > > > > > > > > Cc: Lukas Czerner <lczerner@redhat.com> > > > > Cc: Jan Kara <jack@suse.cz> > > > > Cc: Christian Brauner <brauner@kernel.org> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > After some verification (which was not completely trivial e.g. for > > > directories) I agree all cases should be covered. Feel free to add: > > > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > > > Honza > > > > > > > Thanks. > > > > I think this covers the typical cases, but there are some places I > > missed: > > > > The setacl codepath, for one, and there are a number of places that set > > the ctime explicitly for hole punching and the like. > > Hum, why is setacl() not covered by your change to ext4_xattr_set_handle()? > ext4_set_acl() ends up calling it... I have checked hole punching (whole > ext4_fallocate()) and it seems to be incrementing iversion where needed. > Oh, ok! I mostly noticed the places I was "missing" by inspection. It's possible that I don't need those changes after all. If you think this patch is sufficient, then I'll plan to just go with this one. > > I'm planning to > > send a v2 once I do a bit more testing. I'll hold off on adding your > > Reviewed-by just yet, since the final patch may be quite a bit > > different. > > OK, thanks! > > Honza > > > > > > > > > --- > > > > fs/ext4/inode.c | 10 +++++----- > > > > fs/ext4/xattr.c | 2 ++ > > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > I think this patch should probably supersede Lukas' patch entitled: > > > > > > > > ext4: don't increase iversion counter for ea_inodes > > > > > > > > This will also mean that we'll need to respin the patch to turn on the > > > > i_version counter unconditionally in ext4 (though that should be > > > > trivial). > > > > > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > > > index 601214453c3a..a70921df89a5 100644 > > > > --- a/fs/ext4/inode.c > > > > +++ b/fs/ext4/inode.c > > > > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > int error, rc = 0; > > > > int orphan = 0; > > > > const unsigned int ia_valid = attr->ia_valid; > > > > + bool inc_ivers = IS_IVERSION(inode); > > > > > > > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > > > return -EIO; > > > > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > return -EINVAL; > > > > } > > > > > > > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > > > > - inode_inc_iversion(inode); > > > > + if (attr->ia_size == inode->i_size) > > > > + inc_ivers = false; > > > > > > > > if (shrink) { > > > > if (ext4_should_order_data(inode)) { > > > > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > } > > > > > > > > if (!error) { > > > > + if (inc_ivers) > > > > + inode_inc_iversion(inode); > > > > setattr_copy(mnt_userns, inode, attr); > > > > mark_inode_dirty(inode); > > > > } > > > > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > > > > } > > > > ext4_fc_track_inode(handle, inode); > > > > > > > > - if (IS_I_VERSION(inode)) > > > > - inode_inc_iversion(inode); > > > > - > > > > /* the do_update_inode consumes one bh->b_count */ > > > > get_bh(iloc->bh); > > > > > > > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > > > > index 533216e80fa2..4d84919d1c9c 100644 > > > > --- a/fs/ext4/xattr.c > > > > +++ b/fs/ext4/xattr.c > > > > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > > > > if (!error) { > > > > ext4_xattr_update_super_block(handle, inode->i_sb); > > > > inode->i_ctime = current_time(inode); > > > > + if (IS_IVERSION(inode)) > > > > + inode_inc_iversion(inode); > > > > if (!value) > > > > no_expand = 0; > > > > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > > > > -- > > > > 2.37.2 > > > > > > > > -- > > Jeff Layton <jlayton@kernel.org>
On Wed 17-08-22 09:28:52, Jeff Layton wrote: > On Wed, 2022-08-17 at 15:25 +0200, Jan Kara wrote: > > On Wed 17-08-22 09:09:58, Jeff Layton wrote: > > > On Wed, 2022-08-17 at 15:04 +0200, Jan Kara wrote: > > > > On Tue 16-08-22 09:15:22, Jeff Layton wrote: > > > > > ext4 currently updates the i_version counter when the atime is updated > > > > > during a read. This is less than ideal as it can cause unnecessary cache > > > > > invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also > > > > > problematic since it can also corrupt the i_version counter for > > > > > ea_inodes. > > > > > > > > > > We aren't bumping the file times in ext4_mark_iloc_dirty, so changing > > > > > the i_version there seems wrong, and is the cause of both problems. > > > > > Remove that callsite and add increments to the setattr and setxattr > > > > > codepaths (at the same time that we update the ctime). The i_version > > > > > bump that already happens during timestamp updates should take care of > > > > > the rest. > > > > > > > > > > Cc: Lukas Czerner <lczerner@redhat.com> > > > > > Cc: Jan Kara <jack@suse.cz> > > > > > Cc: Christian Brauner <brauner@kernel.org> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > > > After some verification (which was not completely trivial e.g. for > > > > directories) I agree all cases should be covered. Feel free to add: > > > > > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > > > > > Honza > > > > > > > > > > Thanks. > > > > > > I think this covers the typical cases, but there are some places I > > > missed: > > > > > > The setacl codepath, for one, and there are a number of places that set > > > the ctime explicitly for hole punching and the like. > > > > Hum, why is setacl() not covered by your change to ext4_xattr_set_handle()? > > ext4_set_acl() ends up calling it... I have checked hole punching (whole > > ext4_fallocate()) and it seems to be incrementing iversion where needed. > > > > Oh, ok! I mostly noticed the places I was "missing" by inspection. It's > possible that I don't need those changes after all. If you think this > patch is sufficient, then I'll plan to just go with this one. So I did some more grepping and I think we are missing i_version increment in ext4 ioctl handling. In particular stuff like swap_inode_boot_loader(), ext4_ioctl_setflags(), ext4_ioctl_setproject(), and EXT4_IOC_SETVERSION handling in __ext4_ioctl() need i_version increment. Similarly for defrag ioctl implemented in ext4_move_extents() (that even seems to miss a ctime update). Honza > > > > > --- > > > > > fs/ext4/inode.c | 10 +++++----- > > > > > fs/ext4/xattr.c | 2 ++ > > > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > > > I think this patch should probably supersede Lukas' patch entitled: > > > > > > > > > > ext4: don't increase iversion counter for ea_inodes > > > > > > > > > > This will also mean that we'll need to respin the patch to turn on the > > > > > i_version counter unconditionally in ext4 (though that should be > > > > > trivial). > > > > > > > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > > > > index 601214453c3a..a70921df89a5 100644 > > > > > --- a/fs/ext4/inode.c > > > > > +++ b/fs/ext4/inode.c > > > > > @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > > int error, rc = 0; > > > > > int orphan = 0; > > > > > const unsigned int ia_valid = attr->ia_valid; > > > > > + bool inc_ivers = IS_IVERSION(inode); > > > > > > > > > > if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > > > > return -EIO; > > > > > @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > > return -EINVAL; > > > > > } > > > > > > > > > > - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) > > > > > - inode_inc_iversion(inode); > > > > > + if (attr->ia_size == inode->i_size) > > > > > + inc_ivers = false; > > > > > > > > > > if (shrink) { > > > > > if (ext4_should_order_data(inode)) { > > > > > @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > > } > > > > > > > > > > if (!error) { > > > > > + if (inc_ivers) > > > > > + inode_inc_iversion(inode); > > > > > setattr_copy(mnt_userns, inode, attr); > > > > > mark_inode_dirty(inode); > > > > > } > > > > > @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, > > > > > } > > > > > ext4_fc_track_inode(handle, inode); > > > > > > > > > > - if (IS_I_VERSION(inode)) > > > > > - inode_inc_iversion(inode); > > > > > - > > > > > /* the do_update_inode consumes one bh->b_count */ > > > > > get_bh(iloc->bh); > > > > > > > > > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > > > > > index 533216e80fa2..4d84919d1c9c 100644 > > > > > --- a/fs/ext4/xattr.c > > > > > +++ b/fs/ext4/xattr.c > > > > > @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > > > > > if (!error) { > > > > > ext4_xattr_update_super_block(handle, inode->i_sb); > > > > > inode->i_ctime = current_time(inode); > > > > > + if (IS_IVERSION(inode)) > > > > > + inode_inc_iversion(inode); > > > > > if (!value) > > > > > no_expand = 0; > > > > > error = ext4_mark_iloc_dirty(handle, inode, &is.iloc); > > > > > -- > > > > > 2.37.2 > > > > > > > > > > > -- > > > Jeff Layton <jlayton@kernel.org> > > -- > Jeff Layton <jlayton@kernel.org>
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 601214453c3a..a70921df89a5 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -5342,6 +5342,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, int error, rc = 0; int orphan = 0; const unsigned int ia_valid = attr->ia_valid; + bool inc_ivers = IS_IVERSION(inode); if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) return -EIO; @@ -5425,8 +5426,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, return -EINVAL; } - if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size) - inode_inc_iversion(inode); + if (attr->ia_size == inode->i_size) + inc_ivers = false; if (shrink) { if (ext4_should_order_data(inode)) { @@ -5528,6 +5529,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, } if (!error) { + if (inc_ivers) + inode_inc_iversion(inode); setattr_copy(mnt_userns, inode, attr); mark_inode_dirty(inode); } @@ -5731,9 +5734,6 @@ int ext4_mark_iloc_dirty(handle_t *handle, } ext4_fc_track_inode(handle, inode); - if (IS_I_VERSION(inode)) - inode_inc_iversion(inode); - /* the do_update_inode consumes one bh->b_count */ get_bh(iloc->bh); diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 533216e80fa2..4d84919d1c9c 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -2412,6 +2412,8 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, if (!error) { ext4_xattr_update_super_block(handle, inode->i_sb); inode->i_ctime = current_time(inode); + if (IS_IVERSION(inode)) + inode_inc_iversion(inode); if (!value) no_expand = 0; error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
ext4 currently updates the i_version counter when the atime is updated during a read. This is less than ideal as it can cause unnecessary cache invalidations with NFSv4. The increment in ext4_mark_iloc_dirty is also problematic since it can also corrupt the i_version counter for ea_inodes. We aren't bumping the file times in ext4_mark_iloc_dirty, so changing the i_version there seems wrong, and is the cause of both problems. Remove that callsite and add increments to the setattr and setxattr codepaths (at the same time that we update the ctime). The i_version bump that already happens during timestamp updates should take care of the rest. Cc: Lukas Czerner <lczerner@redhat.com> Cc: Jan Kara <jack@suse.cz> Cc: Christian Brauner <brauner@kernel.org> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ext4/inode.c | 10 +++++----- fs/ext4/xattr.c | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) I think this patch should probably supersede Lukas' patch entitled: ext4: don't increase iversion counter for ea_inodes This will also mean that we'll need to respin the patch to turn on the i_version counter unconditionally in ext4 (though that should be trivial).