| Message ID | 1306320216-19368-1-git-send-email-jack@suse.cz |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, May 25, 2011 at 12:43:36PM +0200, Jan Kara wrote: > Mostly trivial conversion. We fix a bug that IS_IMMUTABLE and IS_APPEND > files could not be truncated during failed writes as we change the code. > In fact we remove the test altogether because do_sys_[f]truncate() and > may_open() do necessary checks anyway. This doesn't look quite correct me. One of the major points of the new truncate sequence is to to *_truncate_page before updating i_size, so that we can properly handle an error there. With your patch it's still called too late. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2011 at 12:43:36PM +0200, Jan Kara wrote: > Mostly trivial conversion. We fix a bug that IS_IMMUTABLE and IS_APPEND > files could not be truncated during failed writes as we change the code. > In fact we remove the test altogether because do_sys_[f]truncate() and > may_open() do necessary checks anyway. > > Signed-off-by: Jan Kara <jack@suse.cz> Thanks, added to the ext4 tree. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2011 at 05:41:10PM -0400, Ted Ts'o wrote: > On Wed, May 25, 2011 at 12:43:36PM +0200, Jan Kara wrote: > > Mostly trivial conversion. We fix a bug that IS_IMMUTABLE and IS_APPEND > > files could not be truncated during failed writes as we change the code. > > In fact we remove the test altogether because do_sys_[f]truncate() and > > may_open() do necessary checks anyway. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > Thanks, added to the ext4 tree. Oops, replied to the wrong message. I pulled in the ext4 v2 version of your patch, obviously... - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 25-05-11 07:48:23, Christoph Hellwig wrote: > On Wed, May 25, 2011 at 12:43:36PM +0200, Jan Kara wrote: > > Mostly trivial conversion. We fix a bug that IS_IMMUTABLE and IS_APPEND > > files could not be truncated during failed writes as we change the code. > > In fact we remove the test altogether because do_sys_[f]truncate() and > > may_open() do necessary checks anyway. > > This doesn't look quite correct me. One of the major points of the > new truncate sequence is to to *_truncate_page before updating i_size, > so that we can properly handle an error there. With your patch it's > still called too late. OK, I missed this point (frankly, handling of error from ext3_block_truncate_page() does not seem like a huge win when we cannot handle error anywhere else during truncate but it's an improvement I agree). I'll move ext3_block_truncate_page() before the setting of i_size. Thanks for having a look at the patch. Honza
On Thu, May 26, 2011 at 04:23:37PM +0200, Jan Kara wrote: > > This doesn't look quite correct me. One of the major points of the > > new truncate sequence is to to *_truncate_page before updating i_size, > > so that we can properly handle an error there. With your patch it's > > still called too late. > OK, I missed this point (frankly, handling of error from > ext3_block_truncate_page() does not seem like a huge win when we cannot > handle error anywhere else during truncate but it's an improvement I > agree). I'll move ext3_block_truncate_page() before the setting of i_size. > Thanks for having a look at the patch. Btw, I think ext4 has the same issue. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext3/file.c b/fs/ext3/file.c index f55df0e..86c8ab3 100644 --- a/fs/ext3/file.c +++ b/fs/ext3/file.c @@ -71,7 +71,6 @@ const struct file_operations ext3_file_operations = { }; const struct inode_operations ext3_file_inode_operations = { - .truncate = ext3_truncate, .setattr = ext3_setattr, #ifdef CONFIG_EXT3_FS_XATTR .setxattr = generic_setxattr, diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c index 4d327a5..d78da39 100644 --- a/fs/ext3/inode.c +++ b/fs/ext3/inode.c @@ -234,12 +234,10 @@ void ext3_evict_inode (struct inode *inode) if (inode->i_blocks) ext3_truncate(inode); /* - * Kill off the orphan record which ext3_truncate created. - * AKPM: I think this can be inside the above `if'. - * Note that ext3_orphan_del() has to be able to cope with the - * deletion of a non-existent orphan - this is because we don't - * know if ext3_truncate() actually created an orphan record. - * (Well, we could do this if we need to, but heck - it works) + * Kill off the orphan record created when the inode lost the last + * link. Note that ext3_orphan_del() has to be able to cope with the + * deletion of a non-existent orphan - ext3_truncate() could + * have removed the record. */ ext3_orphan_del(handle, inode); EXT3_I(inode)->i_dtime = get_seconds(); @@ -890,6 +888,9 @@ int ext3_get_blocks_handle(handle_t *handle, struct inode *inode, if (!create || err == -EIO) goto cleanup; + /* + * Block out ext3_truncate while we alter the tree + */ mutex_lock(&ei->truncate_mutex); /* @@ -938,9 +939,6 @@ int ext3_get_blocks_handle(handle_t *handle, struct inode *inode, */ count = ext3_blks_to_allocate(partial, indirect_blks, maxblocks, blocks_to_boundary); - /* - * Block out ext3_truncate while we alter the tree - */ err = ext3_alloc_branch(handle, inode, indirect_blks, &count, goal, offsets + (partial - chain), partial); @@ -1863,7 +1861,7 @@ retry: /* This is really bad luck. We've written the data * but cannot extend i_size. Truncate allocated blocks * and pretend the write failed... */ - ext3_truncate(inode); + ext3_truncate_failed_write(inode); ret = PTR_ERR(handle); goto out; } @@ -2414,8 +2412,6 @@ static void ext3_free_branches(handle_t *handle, struct inode *inode, int ext3_can_truncate(struct inode *inode) { - if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) - return 0; if (S_ISREG(inode->i_mode)) return 1; if (S_ISDIR(inode->i_mode)) @@ -3264,9 +3260,8 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr) if ((attr->ia_valid & ATTR_SIZE) && attr->ia_size != i_size_read(inode)) { - rc = vmtruncate(inode, attr->ia_size); - if (rc) - goto err_out; + truncate_setsize(inode, attr->ia_size); + ext3_truncate(inode); } setattr_copy(inode, attr); diff --git a/include/linux/ext3_fs.h b/include/linux/ext3_fs.h index 85c1d30..d7511b4 100644 --- a/include/linux/ext3_fs.h +++ b/include/linux/ext3_fs.h @@ -913,7 +913,7 @@ extern void ext3_dirty_inode(struct inode *); extern int ext3_change_inode_journal_flag(struct inode *, int); extern int ext3_get_inode_loc(struct inode *, struct ext3_iloc *); extern int ext3_can_truncate(struct inode *inode); -extern void ext3_truncate (struct inode *); +extern void ext3_truncate(struct inode *inode); extern void ext3_set_inode_flags(struct inode *); extern void ext3_get_inode_flags(struct ext3_inode_info *); extern void ext3_set_aops(struct inode *inode);
Mostly trivial conversion. We fix a bug that IS_IMMUTABLE and IS_APPEND files could not be truncated during failed writes as we change the code. In fact we remove the test altogether because do_sys_[f]truncate() and may_open() do necessary checks anyway. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext3/file.c | 1 - fs/ext3/inode.c | 25 ++++++++++--------------- include/linux/ext3_fs.h | 2 +- 3 files changed, 11 insertions(+), 17 deletions(-) - updated patch to match ext4 version: - use truncate_setsize() instead of opencoding it - remove IS_APPEND & IS_IMMUTABLE check altogether - remove duplicate inode_newsize_ok() check