| Message ID | 1321344474-14707-2-git-send-email-xiaoqiangnk@gmail.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tue, Nov 15, 2011 at 04:07:51PM +0800, Yongqiang Yang wrote: > This patch lets ext4 journal deletion of data blocks. Besides this, > a unnecessary variable is removed. > > Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> I don't see the point of this patch; it seems to be a code simplification, but in fact it introduces a bug which has to get fixed in patch 3/5 of this series. The code here is a little arcane, because if bh is non-null, then count must be 1. This is expressed in the BUG_ON found in the function: > BUG_ON(bh && (count > 1)); The reason for this bit of complexity is to avoid needing to call sb_find_get_block() in those places where we have the buffer_head already. This happens in exactly two locations: in an error cleanup path in fs/ext4/indirect.c, and when releasing an xattr block in ext4_xattr_release_block(). The better way of dealing with this is to drop the bh argument from ext4_free_blocks() completely, and explicitly call ext4_forget() on the bh in those two functions. This will require changing all of the call sites of ext4_free_blocks(), but it simplifies the function signature as well as simplifying the code. - Ted > fs/ext4/mballoc.c | 7 ++----- > 1 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index e2d8be8..2529efc 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -4562,19 +4562,16 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, > trace_ext4_free_blocks(inode, block, count, flags); > > if (flags & EXT4_FREE_BLOCKS_FORGET) { > - struct buffer_head *tbh = bh; > int i; > > BUG_ON(bh && (count > 1)); > > for (i = 0; i < count; i++) { > if (!bh) > - tbh = sb_find_get_block(inode->i_sb, > + bh = sb_find_get_block(inode->i_sb, > block + i); > - if (unlikely(!tbh)) > - continue; > ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA, > - inode, tbh, block + i); > + inode, bh, block + i); > } > } > > -- > 1.7.5.1 > -- 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
Hi Ted, The 2nd and 3rd patch aim to let ext4_free_blocks work with journal mode. Consider that journal mode of a file is changed from ordered mode to journal mode and several data blocks are deleted, then bh passed in is NULL and sb_find_get_block returns NULL, but we need ext4_forget to handle the data blocks to record them in revoke table. I am not sure status of ext4 with journal mode, according code here it seems that ext4 with journal mode does not work. Yongqiang. On Thu, Dec 29, 2011 at 1:23 AM, Ted Ts'o <tytso@mit.edu> wrote: > On Tue, Nov 15, 2011 at 04:07:51PM +0800, Yongqiang Yang wrote: >> This patch lets ext4 journal deletion of data blocks. Besides this, >> a unnecessary variable is removed. >> >> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> > > I don't see the point of this patch; it seems to be a code > simplification, but in fact it introduces a bug which has to get fixed > in patch 3/5 of this series. > > The code here is a little arcane, because if bh is non-null, then > count must be 1. This is expressed in the BUG_ON found in the > function: > >> BUG_ON(bh && (count > 1)); > > The reason for this bit of complexity is to avoid needing to call > sb_find_get_block() in those places where we have the buffer_head > already. This happens in exactly two locations: in an error cleanup > path in fs/ext4/indirect.c, and when releasing an xattr block in > ext4_xattr_release_block(). > > The better way of dealing with this is to drop the bh argument from > ext4_free_blocks() completely, and explicitly call ext4_forget() on > the bh in those two functions. > > This will require changing all of the call sites of > ext4_free_blocks(), but it simplifies the function signature as well > as simplifying the code. > > - Ted > >> fs/ext4/mballoc.c | 7 ++----- >> 1 files changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >> index e2d8be8..2529efc 100644 >> --- a/fs/ext4/mballoc.c >> +++ b/fs/ext4/mballoc.c >> @@ -4562,19 +4562,16 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, >> trace_ext4_free_blocks(inode, block, count, flags); >> >> if (flags & EXT4_FREE_BLOCKS_FORGET) { >> - struct buffer_head *tbh = bh; >> int i; >> >> BUG_ON(bh && (count > 1)); >> >> for (i = 0; i < count; i++) { >> if (!bh) >> - tbh = sb_find_get_block(inode->i_sb, >> + bh = sb_find_get_block(inode->i_sb, >> block + i); >> - if (unlikely(!tbh)) >> - continue; >> ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA, >> - inode, tbh, block + i); >> + inode, bh, block + i); >> } >> } >> >> -- >> 1.7.5.1 >>
On Fri, Dec 30, 2011 at 10:59:48PM +0800, Yongqiang Yang wrote: > Hi Ted, > > The 2nd and 3rd patch aim to let ext4_free_blocks work with journal > mode. Consider that journal mode of a file is changed from ordered > mode to journal mode and several data blocks are deleted, then bh > passed in is NULL and sb_find_get_block returns NULL, but we need > ext4_forget to handle the data blocks to record them in revoke table. Ah, I see. This wasn't obvious from the commit description. Could you combine patches #2 and #3, and add the above detail in the commit description? Many 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/mballoc.c b/fs/ext4/mballoc.c index e2d8be8..2529efc 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -4562,19 +4562,16 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, trace_ext4_free_blocks(inode, block, count, flags); if (flags & EXT4_FREE_BLOCKS_FORGET) { - struct buffer_head *tbh = bh; int i; BUG_ON(bh && (count > 1)); for (i = 0; i < count; i++) { if (!bh) - tbh = sb_find_get_block(inode->i_sb, + bh = sb_find_get_block(inode->i_sb, block + i); - if (unlikely(!tbh)) - continue; ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA, - inode, tbh, block + i); + inode, bh, block + i); } }
This patch lets ext4 journal deletion of data blocks. Besides this, a unnecessary variable is removed. Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> --- fs/ext4/mballoc.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)