Message ID | 20190521074358.17186-3-jack@suse.cz |
---|---|
State | Superseded |
Headers | show |
Series | ext4: Fix issues in ext4 truncate handling | expand |
On Tue, May 21, 2019 at 09:43:57AM +0200, Jan Kara wrote: > It is possible that unlinked inode enters ext4_setattr() (e.g. if > somebody calls ftruncate(2) on unlinked but still open file). In such > case we should not delete the inode from the orphan list if truncate > fails. Note that this is mostly a theoretical concern as filesystem is > corrupted if we reach this path anyway but let's be consistent in our > orphan handling. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ext4/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 9bcb7f2b86dd..c7f77c643008 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -5625,7 +5625,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) > up_write(&EXT4_I(inode)->i_data_sem); > ext4_journal_stop(handle); > if (error) { > - if (orphan) > + if (orphan && inode->i_nlink) > ext4_orphan_del(NULL, inode); NIT: While ext4_orphan_del() can be called even if the inode was not on the orphan list it kind of tripped me up to see this called even if ext4_orphan_add() fails... But considering how ext4_orphan_del() works: Reviewed-by: Ira Weiny <ira.weiny@intel.com> > goto err_out; > } > -- > 2.16.4 >
On Tue 21-05-19 11:13:49, Ira Weiny wrote: > On Tue, May 21, 2019 at 09:43:57AM +0200, Jan Kara wrote: > > It is possible that unlinked inode enters ext4_setattr() (e.g. if > > somebody calls ftruncate(2) on unlinked but still open file). In such > > case we should not delete the inode from the orphan list if truncate > > fails. Note that this is mostly a theoretical concern as filesystem is > > corrupted if we reach this path anyway but let's be consistent in our > > orphan handling. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/ext4/inode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index 9bcb7f2b86dd..c7f77c643008 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -5625,7 +5625,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) > > up_write(&EXT4_I(inode)->i_data_sem); > > ext4_journal_stop(handle); > > if (error) { > > - if (orphan) > > + if (orphan && inode->i_nlink) > > ext4_orphan_del(NULL, inode); > > > NIT: While ext4_orphan_del() can be called even if the inode was not on the > orphan list it kind of tripped me up to see this called even if > ext4_orphan_add() fails... > > But considering how ext4_orphan_del() works: > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> Yes, calling ext4_orphan_del() twice is harmless. You're right we just shouldn't set 'orphan = 1' if ext4_orphan_add() fails but that's independent cleanup we could do. Thanks for your review! Honza
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 9bcb7f2b86dd..c7f77c643008 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -5625,7 +5625,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) up_write(&EXT4_I(inode)->i_data_sem); ext4_journal_stop(handle); if (error) { - if (orphan) + if (orphan && inode->i_nlink) ext4_orphan_del(NULL, inode); goto err_out; }
It is possible that unlinked inode enters ext4_setattr() (e.g. if somebody calls ftruncate(2) on unlinked but still open file). In such case we should not delete the inode from the orphan list if truncate fails. Note that this is mostly a theoretical concern as filesystem is corrupted if we reach this path anyway but let's be consistent in our orphan handling. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)