| Message ID | 1319144939-28801-3-git-send-email-dmonakhov@openvz.org |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 21, 2011 at 01:08:55AM +0400, Dmitry Monakhov wrote: > - add ext4_ext_try_shrink helper > - ext4_mark_inode_dirty() called externally in order to allow > caller to butch several inode updates in to one mark_dirty call. > > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> This patch is broken in two ways: 1) It drops the absolutely necessary calls to ext4_ext_get_access() and ext4_ext_dirty(). If you don't do this you will get file system corruptions. 2) Some of the callers to the new ext4_ext_try_shrink() helper depends on it return 0 or 1 depending on whether the tree was shrunk, but others assumed that it would return an error code. Which is OK, since the error codes should be negative, but that means it's critical that the callers check to see whether return code is really an error before returning it. Since this is just a cleanup, I'm going to skip this for now. Dmitry, could you fix this up and resubmit? Thanks!! - 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 Tue, 25 Oct 2011 04:01:35 -0400, "Ted Ts'o" <tytso@mit.edu> wrote: > On Fri, Oct 21, 2011 at 01:08:55AM +0400, Dmitry Monakhov wrote: > > - add ext4_ext_try_shrink helper > > - ext4_mark_inode_dirty() called externally in order to allow > > caller to butch several inode updates in to one mark_dirty call. > > > > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> > > This patch is broken in two ways: > > 1) It drops the absolutely necessary calls to ext4_ext_get_access() > and ext4_ext_dirty(). If you don't do this you will get file > system corruptions. You almost caught me.. When i read this comment for the first time, i've thought that you right, and this patch is crap. Patch was written two month ago so i almost forget exact details. But still it was strange because whole series was tested very hard, and never saw any corruption, or complains. From other point of view usually meta data modification w/o prior get_write_access call result in complain from JBD thread about dirty bufs in transaction.c:warn_dirty_buffer(). This never happen in my case. Answer is simple: ext4_ext_get_access(path[0]) is noop in case of inode body because it has no bh attached. And ext4_ext_dirty() turns in to ext4_mark_inode_dirty(). That's why patch is correct.. > > 2) Some of the callers to the new ext4_ext_try_shrink() helper depends > on it return 0 or 1 depending on whether the tree was shrunk, but > others assumed that it would return an error code. Which is OK, > since the error codes should be negative, but that means it's > critical that the callers check to see whether return code is > really an error before returning it. and {0:1} retcode is sufficient. Situation will change after flexible tree reduction appear, but this happens in perfect future. > > Since this is just a cleanup, I'm going to skip this for now. Dmitry, > could you fix this up and resubmit? Thanks!! > > - 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
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 8eb3656..315775e 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -2570,6 +2570,22 @@ ext4_ext_more_to_rm(struct ext4_ext_path *path) return 1; } +static int ext4_ext_try_shrink(handle_t *handle, struct inode *inode) +{ + /* TODO: flexible tree reduction should be here */ + if (ext_depth(inode) && ext_inode_hdr(inode)->eh_entries == 0) { + /* + * truncate to zero freed all the tree, + * so we need to correct eh_depth + */ + ext_inode_hdr(inode)->eh_depth = 0; + ext_inode_hdr(inode)->eh_max = + cpu_to_le16(ext4_ext_space_root(inode, 0)); + return 1; + } + return 0; +} + static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start) { struct super_block *sb = inode->i_sb; @@ -2577,7 +2593,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start) struct ext4_ext_path *path; ext4_fsblk_t partial_cluster = 0; handle_t *handle; - int i, err; + int i, err, err2; ext_debug("truncate since %u\n", start); @@ -2703,29 +2719,18 @@ again: EXT4_SB(sb)->s_cluster_ratio, flags); partial_cluster = 0; } - - /* TODO: flexible tree reduction should be here */ - if (path->p_hdr->eh_entries == 0) { - /* - * truncate to zero freed all the tree, - * so we need to correct eh_depth - */ - err = ext4_ext_get_access(handle, inode, path); - if (err == 0) { - ext_inode_hdr(inode)->eh_depth = 0; - ext_inode_hdr(inode)->eh_max = - cpu_to_le16(ext4_ext_space_root(inode, 0)); - err = ext4_ext_dirty(handle, inode, path); - } - } + if(ext4_ext_try_shrink(handle, inode)) + err2 = ext4_mark_inode_dirty(handle, inode); + if (!err) + err = err2; out: ext4_ext_drop_refs(path); kfree(path); if (err == -EAGAIN) goto again; - ext4_journal_stop(handle); + err2 = ext4_journal_stop(handle); - return err; + return err ? err : err2; } /* @@ -3965,22 +3970,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode, &partial_cluster, map->m_lblk, map->m_lblk + punched_out); - if (!err && path->p_hdr->eh_entries == 0) { - /* - * Punch hole freed all of this sub tree, - * so we need to correct eh_depth - */ - err = ext4_ext_get_access(handle, inode, path); - if (err == 0) { - ext_inode_hdr(inode)->eh_depth = 0; - ext_inode_hdr(inode)->eh_max = - cpu_to_le16(ext4_ext_space_root( - inode, 0)); - - err = ext4_ext_dirty( - handle, inode, path); - } - } + if (!err && ext4_ext_try_shrink(handle, inode)) + err = ext4_mark_inode_dirty(handle, inode); goto out2; }
- add ext4_ext_try_shrink helper - ext4_mark_inode_dirty() called externally in order to allow caller to butch several inode updates in to one mark_dirty call. Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> --- fs/ext4/extents.c | 59 ++++++++++++++++++++++------------------------------ 1 files changed, 25 insertions(+), 34 deletions(-)