| Message ID | 20110509144201.GP4122@quack.suse.cz |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 5/9/2011 7:42 AM, Jan Kara wrote: > On Mon 09-05-11 10:22:37, Ted Tso wrote: >> On Mon, May 09, 2011 at 04:05:37PM +0200, Jan Kara wrote: >>> Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL >>> without being marked dirty. >> >> Ah, so the right fix then is to add to make the cleanup code like this: >> >> ext4_mark_inode_dirty(handle, dir); >> ext4_handle_dirty_metadata(handle, dir, frame->bh); >> + ext4_handle_dirty_metadata(handle, dir, bh2); >> + if (bh) >> + ext4_handle_dirty_metadata(handle, dir, bh); >> dx_release(frames); >> return retval; >> >> Agreed? > Not quite. make_indexed_dir() does frame->bh = bh and bh = bh2 before > calling do_split(). So bh2 is not really carrying a valid buffer reference > at this point - even more so because do_split() does brelse() on the passed > bh so it need not be around when are at this point. The code is a real > mess. But for example attached patch will work because both callers of > do_split() do brelse() anyway. > > Honza Hi all, Oh, I understand the problem now. Sorry for the late response, I had to stop and dig around with this one for a bit. Would people prefer to add the new code before the do_split like this: + ext4_handle_dirty_metadata(handle, dir, frame->bh); + ext4_handle_dirty_metadata(handle, dir, bh); + de = do_split(handle,dir, &bh, frame, &hinfo, &retval); if (!de) { /* @@ -1421,8 +1425,6 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry, * with corrupted filesystem. */ ext4_mark_inode_dirty(handle, dir); - ext4_handle_dirty_metadata(handle, dir, frame->bh); - ext4_handle_dirty_metadata(handle, dir, bh); dx_release(frames); return retval; } I've tested both patches and they both seem to resolve the null pointer. The only other solution that comes to mind would be to add flags to the do_split to skip the brelse or to do the mark dirty before the brelse as you suggest. Allison Henderson -- 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 09-05-11 13:39:07, Allison Henderson wrote: > On 5/9/2011 7:42 AM, Jan Kara wrote: > >On Mon 09-05-11 10:22:37, Ted Tso wrote: > >>On Mon, May 09, 2011 at 04:05:37PM +0200, Jan Kara wrote: > >>> Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL > >>>without being marked dirty. > >> > >>Ah, so the right fix then is to add to make the cleanup code like this: > >> > >> ext4_mark_inode_dirty(handle, dir); > >> ext4_handle_dirty_metadata(handle, dir, frame->bh); > >>+ ext4_handle_dirty_metadata(handle, dir, bh2); > >>+ if (bh) > >>+ ext4_handle_dirty_metadata(handle, dir, bh); > >> dx_release(frames); > >> return retval; > >> > >>Agreed? > > Not quite. make_indexed_dir() does frame->bh = bh and bh = bh2 before > >calling do_split(). So bh2 is not really carrying a valid buffer reference > >at this point - even more so because do_split() does brelse() on the passed > >bh so it need not be around when are at this point. The code is a real > >mess. But for example attached patch will work because both callers of > >do_split() do brelse() anyway. > > > > Honza > > Hi all, > > Oh, I understand the problem now. Sorry for the late response, I > had to stop and dig around with this one for a bit. Would people > prefer to add the new code before the do_split like this: > > + ext4_handle_dirty_metadata(handle, dir, frame->bh); > + ext4_handle_dirty_metadata(handle, dir, bh); > + > de = do_split(handle,dir, &bh, frame, &hinfo, &retval); > if (!de) { > /* > @@ -1421,8 +1425,6 @@ static int make_indexed_dir(handle_t *handle, > struct dentry *dentry, > * with corrupted filesystem. > */ > ext4_mark_inode_dirty(handle, dir); > - ext4_handle_dirty_metadata(handle, dir, frame->bh); > - ext4_handle_dirty_metadata(handle, dir, bh); > dx_release(frames); > return retval; > } This would be fine with me as well. It has a slight overhead of marking buffer dirty twice but I don't think it really matters. > I've tested both patches and they both seem to resolve the null > pointer. The only other solution that comes to mind would be to add > flags to the do_split to skip the brelse or to do the mark dirty > before the brelse as you suggest. Yes, I don't mind which of them Ted chooses... Honza
From 59729e0ab18a763ba36616a1025ce606a8721f1c Mon Sep 17 00:00:00 2001 From: Jan Kara <jack@suse.cz> Date: Mon, 9 May 2011 16:39:34 +0200 Subject: [PATCH] ext4: Stop releasing passed bh in do_split() make_indexed_dir() needs to do error recovery on the passed bh when do_split() fails. So do not release it early in do_split(). Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/namei.c | 7 +------ 1 files changed, 1 insertions(+), 6 deletions(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 3c7a06e..1cddab9 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1165,11 +1165,8 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, int err = 0, i; bh2 = ext4_append (handle, dir, &newblock, &err); - if (!(bh2)) { - brelse(*bh); - *bh = NULL; + if (!bh2) goto errout; - } BUFFER_TRACE(*bh, "get_write_access"); err = ext4_journal_get_write_access(handle, *bh); @@ -1235,9 +1232,7 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, return de; journal_error: - brelse(*bh); brelse(bh2); - *bh = NULL; ext4_std_error(dir->i_sb, err); errout: *error = err; -- 1.7.1