Hi Andrew,
The patch below addresses the problem we were talking about earlier
where ext3_writepage ends up accessing the inode after the page lock has
been dropped (and hence at a point where it is possible for the inode to
have been reclaimed.) Tested minimally (it builds and boots.)
It makes ext3_journal_stop take an sb, not an inode, as its final
parameter. It also sets sb->s_need_sync_fs, not sb->s_dirt, as setting
s_dirt was only ever a workaround for the lack of a proper sync-fs
mechanism.
Btw, we clear s_need_sync_fs in sync_filesystems(). Don't we also need
to do the same in fsync_super()?
--Stephen
>From 2.5 proposed diff to fix illegal access to inode in
ext3_journal_stop during writepage:
===== fs/ext3/acl.c 1.6 vs edited =====
--- 1.6/fs/ext3/acl.c Tue Feb 25 16:21:08 2003
+++ edited/fs/ext3/acl.c Thu Mar 20 17:18:19 2003
@@ -420,7 +420,7 @@
return PTR_ERR(handle);
}
error = ext3_set_acl(handle, inode, ACL_TYPE_ACCESS, clone);
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
}
posix_acl_release(clone);
return error;
@@ -522,7 +522,7 @@
if (IS_ERR(handle))
return PTR_ERR(handle);
error = ext3_set_acl(handle, inode, type, acl);
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
release_and_out:
posix_acl_release(acl);
===== fs/ext3/inode.c 1.63 vs edited =====
--- 1.63/fs/ext3/inode.c Mon Mar 17 06:33:16 2003
+++ edited/fs/ext3/inode.c Thu Mar 20 17:19:55 2003
@@ -235,7 +235,7 @@
clear_inode(inode);
else
ext3_free_inode(handle, inode);
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
unlock_kernel();
return;
no_delete:
@@ -1107,7 +1107,7 @@
}
prepare_write_failed:
if (ret)
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
out:
unlock_kernel();
return ret;
@@ -1176,7 +1176,7 @@
if (!ret)
ret = ret2;
}
- ret2 = ext3_journal_stop(handle, inode);
+ ret2 = ext3_journal_stop(handle, inode->i_sb);
unlock_kernel();
if (!ret)
ret = ret2;
@@ -1299,6 +1299,7 @@
static int ext3_writepage(struct page *page, struct writeback_control *wbc)
{
struct inode *inode = page->mapping->host;
+ struct super_block *sb = inode->i_sb;
struct buffer_head *page_bufs;
handle_t *handle = NULL;
int ret = 0, err;
@@ -1374,7 +1375,7 @@
PAGE_CACHE_SIZE, NULL, bput_one);
}
- err = ext3_journal_stop(handle, inode);
+ err = ext3_journal_stop(handle, sb);
if (!ret)
ret = err;
unlock_kernel();
@@ -1479,7 +1480,7 @@
ret = err;
}
}
- err = ext3_journal_stop(handle, inode);
+ err = ext3_journal_stop(handle, inode->i_sb);
if (ret == 0)
ret = err;
unlock_kernel();
@@ -2140,7 +2141,7 @@
if (inode->i_nlink)
ext3_orphan_del(handle, inode);
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
unlock_kernel();
}
@@ -2560,7 +2561,7 @@
rc = ext3_mark_inode_dirty(handle, inode);
if (!error)
error = rc;
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
}
rc = inode_setattr(inode, attr);
@@ -2737,7 +2738,7 @@
current_handle);
ext3_mark_inode_dirty(handle, inode);
}
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
out:
unlock_kernel();
}
@@ -2818,7 +2819,7 @@
err = ext3_mark_inode_dirty(handle, inode);
handle->h_sync = 1;
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
ext3_std_error(inode->i_sb, err);
return err;
===== fs/ext3/ioctl.c 1.7 vs edited =====
--- 1.7/fs/ext3/ioctl.c Mon Mar 17 06:33:16 2003
+++ edited/fs/ext3/ioctl.c Thu Mar 20 17:18:18 2003
@@ -90,7 +90,7 @@
err = ext3_mark_iloc_dirty(handle, inode, &iloc);
flags_err:
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
if (err)
return err;
@@ -126,7 +126,7 @@
inode->i_generation = generation;
err = ext3_mark_iloc_dirty(handle, inode, &iloc);
- ext3_journal_stop(handle, inode);
+ ext3_journal_stop(handle, inode->i_sb);
return err;
}
#ifdef CONFIG_JBD_DEBUG
===== fs/ext3/namei.c 1.36 vs edited =====
--- 1.36/fs/ext3/namei.c Fri Feb 28 20:13:20 2003
+++ edited/fs/ext3/namei.c Thu Mar 20 17:18:39 2003
@@ -1615,7 +1615,7 @@
inode->i_mapping->a_ops = &ext3_aops;
err = ext3_add_nondir(handle, dentry, inode);
}
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
unlock_kernel();
return err;
}
@@ -1647,7 +1647,7 @@
#endif
err = ext3_add_nondir(handle, dentry, inode);
}
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
unlock_kernel();
return err;
}
@@ -1721,7 +1721,7 @@
ext3_mark_inode_dirty(handle, dir);
d_instantiate(dentry, inode);
out_stop:
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
unlock_kernel();
return err;
}
@@ -1994,7 +1994,7 @@
ext3_mark_inode_dirty(handle, dir);
end_rmdir:
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
brelse (bh);
unlock_kernel();
return retval;
@@ -2050,7 +2050,7 @@
retval = 0;
end_unlink:
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
unlock_kernel();
brelse (bh);
return retval;
@@ -2109,7 +2109,7 @@
EXT3_I(inode)->i_disksize = inode->i_size;
err = ext3_add_nondir(handle, dentry, inode);
out_stop:
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
unlock_kernel();
return err;
}
@@ -2142,7 +2142,7 @@
atomic_inc(&inode->i_count);
err = ext3_add_nondir(handle, dentry, inode);
- ext3_journal_stop(handle, dir);
+ ext3_journal_stop(handle, dir->i_sb);
unlock_kernel();
return err;
}
@@ -2299,7 +2299,7 @@
brelse (dir_bh);
brelse (old_bh);
brelse (new_bh);
- ext3_journal_stop(handle, old_dir);
+ ext3_journal_stop(handle, old_dir->i_sb);
unlock_kernel();
return retval;
}
===== fs/ext3/super.c 1.53 vs edited =====
--- 1.53/fs/ext3/super.c Tue Mar 11 06:34:32 2003
+++ edited/fs/ext3/super.c Thu Mar 20 11:01:35 2003
@@ -1735,7 +1735,7 @@
tid_t target;
lock_kernel();
- sb->s_dirt = 0;
+ sb->s_need_sync_fs = 0;
target = log_start_commit(EXT3_SB(sb)->s_journal, NULL);
if (wait)
log_wait_commit(EXT3_SB(sb)->s_journal, target);
===== fs/ext3/xattr.c 1.11 vs edited =====
--- 1.11/fs/ext3/xattr.c Sat Mar 8 22:50:42 2003
+++ edited/fs/ext3/xattr.c Thu Mar 20 17:18:38 2003
@@ -854,7 +854,7 @@
else
error = ext3_xattr_set_handle(handle, inode, name_index, name,
value, value_len, flags);
- error2 = ext3_journal_stop(handle, inode);
+ error2 = ext3_journal_stop(handle, inode->i_sb);
unlock_kernel();
return error ? error : error2;
===== include/linux/ext3_jbd.h 1.9 vs edited =====
--- 1.9/include/linux/ext3_jbd.h Tue Feb 11 03:20:37 2003
+++ edited/include/linux/ext3_jbd.h Thu Mar 20 17:13:18 2003
@@ -217,20 +217,21 @@
* appropriate.
*/
static inline int __ext3_journal_stop(const char *where,
- handle_t *handle, struct inode *inode)
+ handle_t *handle,
+ struct super_block *sb)
{
int err = handle->h_err;
int rc = journal_stop(handle);
- inode->i_sb->s_dirt = 1;
+ sb->s_need_sync_fs = 1;
if (!err)
err = rc;
if (err)
- __ext3_std_error(inode->i_sb, where, err);
+ __ext3_std_error(sb, where, err);
return err;
}
-#define ext3_journal_stop(handle, inode) \
- __ext3_journal_stop(__FUNCTION__, (handle), (inode))
+#define ext3_journal_stop(handle, sb) \
+ __ext3_journal_stop(__FUNCTION__, (handle), (sb))
static inline handle_t *ext3_journal_current_handle(void)
{
