| Message ID | 1241692770-22547-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Aneesh Kumar K.V wrote: > These buffer_heads are allocated on stack and are > used only to make get_blocks calls. So we can set the > b_state to 0 > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> I'd noticed this too, thanks for fixing up. > --- > fs/ext4/extents.c | 1 + > fs/ext4/inode.c | 2 +- > fs/mpage.c | 2 +- > 3 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index e963870..10b3028 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -3141,6 +3141,7 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len) > ret = PTR_ERR(handle); > break; > } > + map_bh.b_state = 0; > ret = ext4_get_blocks_wrap(handle, inode, block, > max_blocks, &map_bh, > EXT4_CREATE_UNINITIALIZED_EXT, 0, 0); > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 43884e3..c3cd00f 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -2104,7 +2104,7 @@ static int mpage_da_map_blocks(struct mpage_da_data *mpd) > if ((mpd->b_state & (1 << BH_Mapped)) && > !(mpd->b_state & (1 << BH_Delay))) > return 0; > - new.b_state = mpd->b_state; > + new.b_state = 0; hm can you explain why we want 0 rather than mpd->b_state? The others are obvious, b_state was largely uninitialized, but this is changing what looked like a different intentional initialization. Can you update the changelog to say why it's wrong? While we're at it could we name this something other than "new?" If it's a mapping bh, maybe "map_bh" like normal? :) > new.b_blocknr = 0; > new.b_size = mpd->b_size; > next = mpd->b_blocknr; > diff --git a/fs/mpage.c b/fs/mpage.c > index 680ba60..cd98409 100644 > --- a/fs/mpage.c > +++ b/fs/mpage.c > @@ -412,7 +412,7 @@ int mpage_readpage(struct page *page, get_block_t get_block) > struct buffer_head map_bh; > unsigned long first_logical_block = 0; > > - clear_buffer_mapped(&map_bh); > + map_bh.b_state = 0; > bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio, > &map_bh, &first_logical_block, get_block); > if (bio) the rest looks good to me; there are places in the core kernel that don't initialize state and just clear flags they "know" they'll care about... it always struck me as messy, clearing state is much much better. -Eric -- 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 Thu, May 07, 2009 at 10:20:26AM -0500, Eric Sandeen wrote: > Aneesh Kumar K.V wrote: > > These buffer_heads are allocated on stack and are > > used only to make get_blocks calls. So we can set the > > b_state to 0 > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > > I'd noticed this too, thanks for fixing up. Is this just a clean-up, or does this fix a bug? It wasn't obvious the patch description. (I'm not a big fan of Ingo's 'Impact: ' header, but it is good to make sure the patch description explains the impact of a patch.) In the long run, we should really look at cleaning up the get_blocks* interfaces so they don't use buffer_head when all they're really doing is passing back a block number. All aside from the confusion it causes, it also bloats our stack usage. - 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 Sun, May 10, 2009 at 07:57:41PM -0400, Theodore Tso wrote: > On Thu, May 07, 2009 at 10:20:26AM -0500, Eric Sandeen wrote: > > Aneesh Kumar K.V wrote: > > > These buffer_heads are allocated on stack and are > > > used only to make get_blocks calls. So we can set the > > > b_state to 0 > > > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > > > > I'd noticed this too, thanks for fixing up. > > Is this just a clean-up, or does this fix a bug? It wasn't obvious > the patch description. (I'm not a big fan of Ingo's 'Impact: ' > header, but it is good to make sure the patch description explains the > impact of a patch.) If you are taking patch 3/3 you would need this patch. But otherwise you can drop this. The fix is actually in patch 2/3. -aneesh -- 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 Mon, May 11, 2009 at 02:54:43PM +0530, Aneesh Kumar K.V wrote: > On Sun, May 10, 2009 at 07:57:41PM -0400, Theodore Tso wrote: > > On Thu, May 07, 2009 at 10:20:26AM -0500, Eric Sandeen wrote: > > > Aneesh Kumar K.V wrote: > > > > These buffer_heads are allocated on stack and are > > > > used only to make get_blocks calls. So we can set the > > > > b_state to 0 > > > > > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > > > > > > I'd noticed this too, thanks for fixing up. > > > > Is this just a clean-up, or does this fix a bug? It wasn't obvious > > the patch description. (I'm not a big fan of Ingo's 'Impact: ' > > header, but it is good to make sure the patch description explains the > > impact of a patch.) > > If you are taking patch 3/3 you would need this patch. But otherwise you > can drop this. The fix is actually in patch 2/3. OK, thanks. And these patches are orthogonal to these patches in the patch queue, right? fix-sub-block-zeroing-for-unwritten-extents use-a-fake-block-number-for-delalloc-bh So it looks like we probably want to push these two, plus patch 2/3. I'm also very much concerned about your for-2.6.31 patch. It is complex, so we probably want wait, but it looks like we have a real bug which these patches will just expose. And your 2/3 patch isn't going to fix that, right, since these are orthogonal problems. (Again, if you expect patches to supercede existing ones in the ext4 patchwork or which are in the patch queue, please tell me. Sometimes it really isn't obvious....) - 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
Theodore Tso wrote: > On Thu, May 07, 2009 at 10:20:26AM -0500, Eric Sandeen wrote: >> Aneesh Kumar K.V wrote: >>> These buffer_heads are allocated on stack and are used only to >>> make get_blocks calls. So we can set the b_state to 0 >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>> >> I'd noticed this too, thanks for fixing up. > > Is this just a clean-up, or does this fix a bug? It wasn't obvious > the patch description. (I'm not a big fan of Ingo's 'Impact: ' > header, but it is good to make sure the patch description explains > the impact of a patch.) Aneesh responded, but AFAICT it doesn't actually fix a bug, but letting buffer heads float around with indeterminate state can't be good in the long run. > In the long run, we should really look at cleaning up the get_blocks* > interfaces so they don't use buffer_head when all they're really > doing is passing back a block number. All aside from the confusion > it causes, it also bloats our stack usage. Overall, the kernel in general could use something in place of these buffer-heads-that-aren't-buffer-heads, imho. Pretty sure we use it for more than just a block nr, but it's not really a buffer head either, it's one of these "map_bh's" - we should probably at least try to consistently label them as such in ext* -Eric > - 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 Thu, May 07, 2009 at 04:09:28PM +0530, Aneesh Kumar K.V wrote: > These buffer_heads are allocated on stack and are > used only to make get_blocks calls. So we can set the > b_state to 0 > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> I've rewritten the commit description to this: ext4: Properly initialize the buffer_head state From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> These struct buffer_heads are allocated on the stack (and hence are initialized with stack garbage). They are only used to call a get_blocks() function, so that's mostly OK, but b_state must be initialized to be 0. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> Comments? - 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 Mon, May 11, 2009 at 11:17:56PM -0400, Theodore Tso wrote: > On Thu, May 07, 2009 at 04:09:28PM +0530, Aneesh Kumar K.V wrote: > > These buffer_heads are allocated on stack and are > > used only to make get_blocks calls. So we can set the > > b_state to 0 > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > > I've rewritten the commit description to this: > > ext4: Properly initialize the buffer_head state > > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> > > These struct buffer_heads are allocated on the stack (and hence are > initialized with stack garbage). They are only used to call a > get_blocks() function, so that's mostly OK, but b_state must be > initialized to be 0. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > > Comments? > good. -aneesh -- 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 e963870..10b3028 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -3141,6 +3141,7 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len) ret = PTR_ERR(handle); break; } + map_bh.b_state = 0; ret = ext4_get_blocks_wrap(handle, inode, block, max_blocks, &map_bh, EXT4_CREATE_UNINITIALIZED_EXT, 0, 0); diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 43884e3..c3cd00f 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -2104,7 +2104,7 @@ static int mpage_da_map_blocks(struct mpage_da_data *mpd) if ((mpd->b_state & (1 << BH_Mapped)) && !(mpd->b_state & (1 << BH_Delay))) return 0; - new.b_state = mpd->b_state; + new.b_state = 0; new.b_blocknr = 0; new.b_size = mpd->b_size; next = mpd->b_blocknr; diff --git a/fs/mpage.c b/fs/mpage.c index 680ba60..cd98409 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -412,7 +412,7 @@ int mpage_readpage(struct page *page, get_block_t get_block) struct buffer_head map_bh; unsigned long first_logical_block = 0; - clear_buffer_mapped(&map_bh); + map_bh.b_state = 0; bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio, &map_bh, &first_logical_block, get_block); if (bio)
These buffer_heads are allocated on stack and are used only to make get_blocks calls. So we can set the b_state to 0 Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> --- fs/ext4/extents.c | 1 + fs/ext4/inode.c | 2 +- fs/mpage.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)