There are lots of places where we do dentry->d_parent->d_inode without
holding
the dentry->d_lock. This could cause problems with rename. So instead use
dget_parent where we can, or in some cases we don''t even need to use
dentry->d_parent->d_inode since we get the inode of the dir passed to us
from
VFS. I tested this with xfstests and my no space tests and everything turned
out fine. Thanks,
Signed-off-by: Josef Bacik <josef@redhat.com>
---
fs/btrfs/file.c | 2 ++
fs/btrfs/inode.c | 48 ++++++++++++++++++++++++------------------------
fs/btrfs/ioctl.c | 11 +++++++++--
fs/btrfs/transaction.c | 5 ++++-
fs/btrfs/tree-log.c | 22 ++++++++++++++++++----
5 files changed, 57 insertions(+), 31 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index e354c33..6a4daa0 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1047,8 +1047,10 @@ out:
if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) {
trans = btrfs_start_transaction(root, 0);
+ mutex_lock(&inode->i_mutex);
ret = btrfs_log_dentry_safe(trans, root,
file->f_dentry);
+ mutex_unlock(&inode->i_mutex);
if (ret == 0) {
ret = btrfs_sync_log(trans, root);
if (ret == 0)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7146971..e77ee56 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4144,6 +4144,7 @@ static int btrfs_dentry_delete(struct dentry *dentry)
if (btrfs_root_refs(&root->root_item) == 0)
return 1;
}
+
return 0;
}
@@ -4627,12 +4628,12 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
}
static int btrfs_add_nondir(struct btrfs_trans_handle *trans,
- struct dentry *dentry, struct inode *inode,
- int backref, u64 index)
+ struct inode *dir, struct dentry *dentry,
+ struct inode *inode, int backref, u64 index)
{
- int err = btrfs_add_link(trans, dentry->d_parent->d_inode,
- inode, dentry->d_name.name,
- dentry->d_name.len, backref, index);
+ int err = btrfs_add_link(trans, dir, inode,
+ dentry->d_name.name, dentry->d_name.len,
+ backref, index);
if (!err) {
d_instantiate(dentry, inode);
return 0;
@@ -4673,8 +4674,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry
*dentry,
btrfs_set_trans_block_group(trans, dir);
inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name,
- dentry->d_name.len,
- dentry->d_parent->d_inode->i_ino, objectid,
+ dentry->d_name.len, dir->i_ino, objectid,
BTRFS_I(dir)->block_group, mode, &index);
err = PTR_ERR(inode);
if (IS_ERR(inode))
@@ -4687,7 +4687,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry
*dentry,
}
btrfs_set_trans_block_group(trans, inode);
- err = btrfs_add_nondir(trans, dentry, inode, 0, index);
+ err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index);
if (err)
drop_inode = 1;
else {
@@ -4735,10 +4735,8 @@ static int btrfs_create(struct inode *dir, struct dentry
*dentry,
btrfs_set_trans_block_group(trans, dir);
inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name,
- dentry->d_name.len,
- dentry->d_parent->d_inode->i_ino,
- objectid, BTRFS_I(dir)->block_group, mode,
- &index);
+ dentry->d_name.len, dir->i_ino, objectid,
+ BTRFS_I(dir)->block_group, mode, &index);
err = PTR_ERR(inode);
if (IS_ERR(inode))
goto out_unlock;
@@ -4750,7 +4748,7 @@ static int btrfs_create(struct inode *dir, struct dentry
*dentry,
}
btrfs_set_trans_block_group(trans, inode);
- err = btrfs_add_nondir(trans, dentry, inode, 0, index);
+ err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index);
if (err)
drop_inode = 1;
else {
@@ -4810,15 +4808,17 @@ static int btrfs_link(struct dentry *old_dentry, struct
inode *dir,
btrfs_set_trans_block_group(trans, dir);
atomic_inc(&inode->i_count);
- err = btrfs_add_nondir(trans, dentry, inode, 1, index);
+ err = btrfs_add_nondir(trans, dir, dentry, inode, 1, index);
if (err) {
drop_inode = 1;
} else {
+ struct dentry *parent = dget_parent(dentry);
btrfs_update_inode_block_group(trans, dir);
err = btrfs_update_inode(trans, root, inode);
BUG_ON(err);
- btrfs_log_new_name(trans, inode, NULL, dentry->d_parent);
+ btrfs_log_new_name(trans, inode, NULL, parent);
+ dput(parent);
}
nr = trans->blocks_used;
@@ -4858,8 +4858,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry
*dentry, int mode)
btrfs_set_trans_block_group(trans, dir);
inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name,
- dentry->d_name.len,
- dentry->d_parent->d_inode->i_ino, objectid,
+ dentry->d_name.len, dir->i_ino, objectid,
BTRFS_I(dir)->block_group, S_IFDIR | mode,
&index);
if (IS_ERR(inode)) {
@@ -4882,9 +4881,8 @@ static int btrfs_mkdir(struct inode *dir, struct dentry
*dentry, int mode)
if (err)
goto out_fail;
- err = btrfs_add_link(trans, dentry->d_parent->d_inode,
- inode, dentry->d_name.name,
- dentry->d_name.len, 0, index);
+ err = btrfs_add_link(trans, dir, inode, dentry->d_name.name,
+ dentry->d_name.len, 0, index);
if (err)
goto out_fail;
@@ -6613,8 +6611,11 @@ static int btrfs_rename(struct inode *old_dir, struct
dentry *old_dentry,
BUG_ON(ret);
if (old_inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) {
+ struct dentry *parent = dget_parent(new_dentry);
+
btrfs_log_new_name(trans, old_inode, old_dir,
- new_dentry->d_parent);
+ parent);
+ dput(parent);
btrfs_end_log_trans(root);
}
out_fail:
@@ -6764,8 +6765,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry
*dentry,
btrfs_set_trans_block_group(trans, dir);
inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name,
- dentry->d_name.len,
- dentry->d_parent->d_inode->i_ino, objectid,
+ dentry->d_name.len, dir->i_ino, objectid,
BTRFS_I(dir)->block_group, S_IFLNK|S_IRWXUGO,
&index);
err = PTR_ERR(inode);
@@ -6779,7 +6779,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry
*dentry,
}
btrfs_set_trans_block_group(trans, inode);
- err = btrfs_add_nondir(trans, dentry, inode, 0, index);
+ err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index);
if (err)
drop_inode = 1;
else {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e264072..396ccd1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -232,13 +232,17 @@ static noinline int create_subvol(struct btrfs_root *root,
struct btrfs_inode_item *inode_item;
struct extent_buffer *leaf;
struct btrfs_root *new_root;
- struct inode *dir = dentry->d_parent->d_inode;
+ struct dentry *parent = dget_parent(dentry);
+ struct inode *dir;
int ret;
int err;
u64 objectid;
u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
u64 index = 0;
+ dir = parent->d_inode;
+ dput(parent);
+
ret = btrfs_find_free_objectid(NULL, root->fs_info->tree_root,
0, &objectid);
if (ret)
@@ -347,6 +351,7 @@ fail:
static int create_snapshot(struct btrfs_root *root, struct dentry *dentry)
{
struct inode *inode;
+ struct dentry *parent;
struct btrfs_pending_snapshot *pending_snapshot;
struct btrfs_trans_handle *trans;
int ret;
@@ -382,7 +387,9 @@ static int create_snapshot(struct btrfs_root *root, struct
dentry *dentry)
btrfs_orphan_cleanup(pending_snapshot->snap);
- inode = btrfs_lookup_dentry(dentry->d_parent->d_inode, dentry);
+ parent = dget_parent(dentry);
+ inode = btrfs_lookup_dentry(parent->d_inode, dentry);
+ dput(parent);
if (IS_ERR(inode)) {
ret = PTR_ERR(inode);
goto fail;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 0af647c..076729e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -849,6 +849,7 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
struct btrfs_root *root = pending->root;
struct btrfs_root *parent_root;
struct inode *parent_inode;
+ struct dentry *parent_dentry;
struct dentry *dentry;
struct extent_buffer *tmp;
struct extent_buffer *old;
@@ -888,7 +889,9 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
trans->block_rsv = &pending->block_rsv;
dentry = pending->dentry;
- parent_inode = dentry->d_parent->d_inode;
+ parent_dentry = dget_parent(dentry);
+ parent_inode = parent_dentry->d_inode;
+ dput(parent_dentry);
parent_root = BTRFS_I(parent_inode)->root;
record_root_in_trans(trans, parent_root);
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index fb102a9..bf01bdb 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2884,6 +2884,7 @@ static noinline int check_parent_dirs_for_sync(struct
btrfs_trans_handle *trans,
{
int ret = 0;
struct btrfs_root *root;
+ struct dentry *old_parent = NULL;
/*
* for regular files, if its inode is already on disk, we don''t
@@ -2925,10 +2926,13 @@ static noinline int check_parent_dirs_for_sync(struct
btrfs_trans_handle *trans,
if (IS_ROOT(parent))
break;
- parent = parent->d_parent;
+ parent = dget_parent(parent);
+ dput(old_parent);
+ old_parent = parent;
inode = parent->d_inode;
}
+ dput(old_parent);
out:
return ret;
}
@@ -2960,6 +2964,7 @@ int btrfs_log_inode_parent(struct btrfs_trans_handle
*trans,
{
int inode_only = exists_only ? LOG_INODE_EXISTS : LOG_INODE_ALL;
struct super_block *sb;
+ struct dentry *old_parent = NULL;
int ret = 0;
u64 last_committed = root->fs_info->last_trans_committed;
@@ -3031,10 +3036,13 @@ int btrfs_log_inode_parent(struct btrfs_trans_handle
*trans,
if (IS_ROOT(parent))
break;
- parent = parent->d_parent;
+ parent = dget_parent(parent);
+ dput(old_parent);
+ old_parent = parent;
}
ret = 0;
end_trans:
+ dput(old_parent);
if (ret < 0) {
BUG_ON(ret != -ENOSPC);
root->fs_info->last_trans_log_full_commit = trans->transid;
@@ -3054,8 +3062,14 @@ end_no_trans:
int btrfs_log_dentry_safe(struct btrfs_trans_handle *trans,
struct btrfs_root *root, struct dentry *dentry)
{
- return btrfs_log_inode_parent(trans, root, dentry->d_inode,
- dentry->d_parent, 0);
+ struct dentry *parent = dget_parent(dentry);
+ int ret;
+
+ ret = btrfs_log_inode_parent(trans, root, dentry->d_inode, parent, 0);
+
+ dput(parent);
+
+ return ret;
}
/*
--
1.6.6.1
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I did another test with 2.6.36 same results (system instantly crash after mount, even in read only mode) Userland View (mount filesystem image) ------------------------------ snip ------------------------------ vbox portable # uname -a Linux vbox 2.6.36-gentoo-VBOX #1 SMP Tue Oct 26 12:56:53 CEST 2010 x86_64 Intel(R) Xeon(R) CPU W3520 @ 2.67GHz GenuineIntel GNU/Linux vbox portable # modprobe loop vbox portable # losetup /dev/loop0 sd.img vbox portable # mount -t btrfs -o ro /dev/loop0 /mnt/sdcard/ ------------------------------ snip ------------------------------ Kernel View ------------------------------ snip ------------------------------ Btrfs loaded device label home devid 1 transid 122306 /dev/loop0 ------------------------------ snip ------------------------------ btrfsck report (everything ok) ------------------------------ snip ------------------------------ vbox portable # btrfsck sd.img found 4694925312 bytes used err is 0 total csum bytes: 4518676 total tree bytes: 67801088 total fs tree bytes: 53567488 btree space waste bytes: 18724362 file data blocks allocated: 5819858944 referenced 5092511744 Btrfs Btrfs v0.19 ------------------------------ snip ------------------------------ any help appreciated regards Erik -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-10-27 at 10:39 -0400, Josef Bacik wrote:> There are lots of places where we do dentry->d_parent->d_inode without holding > the dentry->d_lock. This could cause problems with rename. So instead use > dget_parent where we can, or in some cases we don''t even need to use > dentry->d_parent->d_inode since we get the inode of the dir passed to us from > VFS. I tested this with xfstests and my no space tests and everything turned > out fine. Thanks, > > Signed-off-by: Josef Bacik <josef@redhat.com> > --- > fs/btrfs/file.c | 2 ++ > fs/btrfs/inode.c | 48 ++++++++++++++++++++++++------------------------ > fs/btrfs/ioctl.c | 11 +++++++++-- > fs/btrfs/transaction.c | 5 ++++- > fs/btrfs/tree-log.c | 22 ++++++++++++++++++---- > 5 files changed, 57 insertions(+), 31 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index e354c33..6a4daa0 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -1047,8 +1047,10 @@ out: > > if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) { > trans = btrfs_start_transaction(root, 0);If btrfs_start_transaction() fails now the inode mutex will be held as well. I guess the resulting oops makes this irrelevant, ;) Looking through the dead end BUG_ON()s in the transaction code, and callers, one thing I''ve found difficult is working out how to recover from IS_ERR() returns from btrfs_start_transaction() in situations like this. Any chance of a patch to handle this to give me an example (well one case anyway) of how to do it?> + mutex_lock(&inode->i_mutex); > ret = btrfs_log_dentry_safe(trans, root, > file->f_dentry); > + mutex_unlock(&inode->i_mutex); > if (ret == 0) { > ret = btrfs_sync_log(trans, root); > if (ret == 0) > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 7146971..e77ee56 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -4144,6 +4144,7 @@ static int btrfs_dentry_delete(struct dentry *dentry) > if (btrfs_root_refs(&root->root_item) == 0) > return 1; > } > + > return 0; > } > > @@ -4627,12 +4628,12 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, > } > > static int btrfs_add_nondir(struct btrfs_trans_handle *trans, > - struct dentry *dentry, struct inode *inode, > - int backref, u64 index) > + struct inode *dir, struct dentry *dentry, > + struct inode *inode, int backref, u64 index) > { > - int err = btrfs_add_link(trans, dentry->d_parent->d_inode, > - inode, dentry->d_name.name, > - dentry->d_name.len, backref, index); > + int err = btrfs_add_link(trans, dir, inode, > + dentry->d_name.name, dentry->d_name.len, > + backref, index); > if (!err) { > d_instantiate(dentry, inode); > return 0; > @@ -4673,8 +4674,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > btrfs_set_trans_block_group(trans, dir); > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > - dentry->d_name.len, > - dentry->d_parent->d_inode->i_ino, objectid, > + dentry->d_name.len, dir->i_ino, objectid, > BTRFS_I(dir)->block_group, mode, &index); > err = PTR_ERR(inode); > if (IS_ERR(inode)) > @@ -4687,7 +4687,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > } > > btrfs_set_trans_block_group(trans, inode); > - err = btrfs_add_nondir(trans, dentry, inode, 0, index); > + err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index); > if (err) > drop_inode = 1; > else { > @@ -4735,10 +4735,8 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > btrfs_set_trans_block_group(trans, dir); > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > - dentry->d_name.len, > - dentry->d_parent->d_inode->i_ino, > - objectid, BTRFS_I(dir)->block_group, mode, > - &index); > + dentry->d_name.len, dir->i_ino, objectid, > + BTRFS_I(dir)->block_group, mode, &index); > err = PTR_ERR(inode); > if (IS_ERR(inode)) > goto out_unlock; > @@ -4750,7 +4748,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > } > > btrfs_set_trans_block_group(trans, inode); > - err = btrfs_add_nondir(trans, dentry, inode, 0, index); > + err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index); > if (err) > drop_inode = 1; > else { > @@ -4810,15 +4808,17 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir, > btrfs_set_trans_block_group(trans, dir); > atomic_inc(&inode->i_count); > > - err = btrfs_add_nondir(trans, dentry, inode, 1, index); > + err = btrfs_add_nondir(trans, dir, dentry, inode, 1, index); > > if (err) { > drop_inode = 1; > } else { > + struct dentry *parent = dget_parent(dentry); > btrfs_update_inode_block_group(trans, dir); > err = btrfs_update_inode(trans, root, inode); > BUG_ON(err); > - btrfs_log_new_name(trans, inode, NULL, dentry->d_parent); > + btrfs_log_new_name(trans, inode, NULL, parent); > + dput(parent); > } > > nr = trans->blocks_used; > @@ -4858,8 +4858,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, int mode) > btrfs_set_trans_block_group(trans, dir); > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > - dentry->d_name.len, > - dentry->d_parent->d_inode->i_ino, objectid, > + dentry->d_name.len, dir->i_ino, objectid, > BTRFS_I(dir)->block_group, S_IFDIR | mode, > &index); > if (IS_ERR(inode)) { > @@ -4882,9 +4881,8 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, int mode) > if (err) > goto out_fail; > > - err = btrfs_add_link(trans, dentry->d_parent->d_inode, > - inode, dentry->d_name.name, > - dentry->d_name.len, 0, index); > + err = btrfs_add_link(trans, dir, inode, dentry->d_name.name, > + dentry->d_name.len, 0, index); > if (err) > goto out_fail; > > @@ -6613,8 +6611,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, > BUG_ON(ret); > > if (old_inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) { > + struct dentry *parent = dget_parent(new_dentry); > + > btrfs_log_new_name(trans, old_inode, old_dir, > - new_dentry->d_parent); > + parent); > + dput(parent); > btrfs_end_log_trans(root); > } > out_fail: > @@ -6764,8 +6765,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > btrfs_set_trans_block_group(trans, dir); > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > - dentry->d_name.len, > - dentry->d_parent->d_inode->i_ino, objectid, > + dentry->d_name.len, dir->i_ino, objectid, > BTRFS_I(dir)->block_group, S_IFLNK|S_IRWXUGO, > &index); > err = PTR_ERR(inode); > @@ -6779,7 +6779,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > } > > btrfs_set_trans_block_group(trans, inode); > - err = btrfs_add_nondir(trans, dentry, inode, 0, index); > + err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index); > if (err) > drop_inode = 1; > else { > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index e264072..396ccd1 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -232,13 +232,17 @@ static noinline int create_subvol(struct btrfs_root *root, > struct btrfs_inode_item *inode_item; > struct extent_buffer *leaf; > struct btrfs_root *new_root; > - struct inode *dir = dentry->d_parent->d_inode; > + struct dentry *parent = dget_parent(dentry); > + struct inode *dir; > int ret; > int err; > u64 objectid; > u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID; > u64 index = 0; > > + dir = parent->d_inode; > + dput(parent);I get that parent is, well, the parent so the child were using will hold a reference to it and so the dput() should be safe. But, since we have the reference why not hold it while we use the inode to make certain the inode cannot go away?> + > ret = btrfs_find_free_objectid(NULL, root->fs_info->tree_root, > 0, &objectid); > if (ret) > @@ -347,6 +351,7 @@ fail: > static int create_snapshot(struct btrfs_root *root, struct dentry *dentry) > { > struct inode *inode; > + struct dentry *parent; > struct btrfs_pending_snapshot *pending_snapshot; > struct btrfs_trans_handle *trans; > int ret; > @@ -382,7 +387,9 @@ static int create_snapshot(struct btrfs_root *root, struct dentry *dentry) > > btrfs_orphan_cleanup(pending_snapshot->snap); > > - inode = btrfs_lookup_dentry(dentry->d_parent->d_inode, dentry); > + parent = dget_parent(dentry); > + inode = btrfs_lookup_dentry(parent->d_inode, dentry); > + dput(parent); > if (IS_ERR(inode)) { > ret = PTR_ERR(inode); > goto fail; > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 0af647c..076729e 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -849,6 +849,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > struct btrfs_root *root = pending->root; > struct btrfs_root *parent_root; > struct inode *parent_inode; > + struct dentry *parent_dentry; > struct dentry *dentry; > struct extent_buffer *tmp; > struct extent_buffer *old; > @@ -888,7 +889,9 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > trans->block_rsv = &pending->block_rsv; > > dentry = pending->dentry; > - parent_inode = dentry->d_parent->d_inode; > + parent_dentry = dget_parent(dentry); > + parent_inode = parent_dentry->d_inode; > + dput(parent_dentry);And again.> parent_root = BTRFS_I(parent_inode)->root; > record_root_in_trans(trans, parent_root); > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index fb102a9..bf01bdb 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -2884,6 +2884,7 @@ static noinline int check_parent_dirs_for_sync(struct btrfs_trans_handle *trans, > { > int ret = 0; > struct btrfs_root *root; > + struct dentry *old_parent = NULL; > > /* > * for regular files, if its inode is already on disk, we don''t > @@ -2925,10 +2926,13 @@ static noinline int check_parent_dirs_for_sync(struct btrfs_trans_handle *trans, > if (IS_ROOT(parent)) > break; > > - parent = parent->d_parent; > + parent = dget_parent(parent); > + dput(old_parent); > + old_parent = parent; > inode = parent->d_inode; > > } > + dput(old_parent); > out: > return ret; > } > @@ -2960,6 +2964,7 @@ int btrfs_log_inode_parent(struct btrfs_trans_handle *trans, > { > int inode_only = exists_only ? LOG_INODE_EXISTS : LOG_INODE_ALL; > struct super_block *sb; > + struct dentry *old_parent = NULL; > int ret = 0; > u64 last_committed = root->fs_info->last_trans_committed; > > @@ -3031,10 +3036,13 @@ int btrfs_log_inode_parent(struct btrfs_trans_handle *trans, > if (IS_ROOT(parent)) > break; > > - parent = parent->d_parent; > + parent = dget_parent(parent); > + dput(old_parent); > + old_parent = parent; > } > ret = 0; > end_trans: > + dput(old_parent); > if (ret < 0) { > BUG_ON(ret != -ENOSPC); > root->fs_info->last_trans_log_full_commit = trans->transid; > @@ -3054,8 +3062,14 @@ end_no_trans: > int btrfs_log_dentry_safe(struct btrfs_trans_handle *trans, > struct btrfs_root *root, struct dentry *dentry) > { > - return btrfs_log_inode_parent(trans, root, dentry->d_inode, > - dentry->d_parent, 0); > + struct dentry *parent = dget_parent(dentry); > + int ret; > + > + ret = btrfs_log_inode_parent(trans, root, dentry->d_inode, parent, 0); > + > + dput(parent); > + > + return ret; > } > > /*-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 28, 2010 at 11:42:04AM +0800, Ian Kent wrote:> On Wed, 2010-10-27 at 10:39 -0400, Josef Bacik wrote: > > There are lots of places where we do dentry->d_parent->d_inode without holding > > the dentry->d_lock. This could cause problems with rename. So instead use > > dget_parent where we can, or in some cases we don''t even need to use > > dentry->d_parent->d_inode since we get the inode of the dir passed to us from > > VFS. I tested this with xfstests and my no space tests and everything turned > > out fine. Thanks, > > > > Signed-off-by: Josef Bacik <josef@redhat.com> > > --- > > fs/btrfs/file.c | 2 ++ > > fs/btrfs/inode.c | 48 ++++++++++++++++++++++++------------------------ > > fs/btrfs/ioctl.c | 11 +++++++++-- > > fs/btrfs/transaction.c | 5 ++++- > > fs/btrfs/tree-log.c | 22 ++++++++++++++++++---- > > 5 files changed, 57 insertions(+), 31 deletions(-) > > > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > > index e354c33..6a4daa0 100644 > > --- a/fs/btrfs/file.c > > +++ b/fs/btrfs/file.c > > @@ -1047,8 +1047,10 @@ out: > > > > if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) { > > trans = btrfs_start_transaction(root, 0); > > If btrfs_start_transaction() fails now the inode mutex will be held as > well. I guess the resulting oops makes this irrelevant, ;) > > Looking through the dead end BUG_ON()s in the transaction code, and > callers, one thing I''ve found difficult is working out how to recover > from IS_ERR() returns from btrfs_start_transaction() in situations like > this. > > Any chance of a patch to handle this to give me an example (well one > case anyway) of how to do it? >Yup I can do that. Recovering from some of these errors is going to be tricky, if you are going to work on that what I had in mind was doing alot of static foo = 0; foo++; if (foo % 10000) return (error where we used to BUG_ON()); so to be sure that all of a sudden returning an error doesn''t cause an panic farther up the callchain.> > + mutex_lock(&inode->i_mutex); > > ret = btrfs_log_dentry_safe(trans, root, > > file->f_dentry); > > + mutex_unlock(&inode->i_mutex); > > if (ret == 0) { > > ret = btrfs_sync_log(trans, root); > > if (ret == 0) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 7146971..e77ee56 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -4144,6 +4144,7 @@ static int btrfs_dentry_delete(struct dentry *dentry) > > if (btrfs_root_refs(&root->root_item) == 0) > > return 1; > > } > > + > > return 0; > > } > > > > @@ -4627,12 +4628,12 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, > > } > > > > static int btrfs_add_nondir(struct btrfs_trans_handle *trans, > > - struct dentry *dentry, struct inode *inode, > > - int backref, u64 index) > > + struct inode *dir, struct dentry *dentry, > > + struct inode *inode, int backref, u64 index) > > { > > - int err = btrfs_add_link(trans, dentry->d_parent->d_inode, > > - inode, dentry->d_name.name, > > - dentry->d_name.len, backref, index); > > + int err = btrfs_add_link(trans, dir, inode, > > + dentry->d_name.name, dentry->d_name.len, > > + backref, index); > > if (!err) { > > d_instantiate(dentry, inode); > > return 0; > > @@ -4673,8 +4674,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > > btrfs_set_trans_block_group(trans, dir); > > > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > > - dentry->d_name.len, > > - dentry->d_parent->d_inode->i_ino, objectid, > > + dentry->d_name.len, dir->i_ino, objectid, > > BTRFS_I(dir)->block_group, mode, &index); > > err = PTR_ERR(inode); > > if (IS_ERR(inode)) > > @@ -4687,7 +4687,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > > } > > > > btrfs_set_trans_block_group(trans, inode); > > - err = btrfs_add_nondir(trans, dentry, inode, 0, index); > > + err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index); > > if (err) > > drop_inode = 1; > > else { > > @@ -4735,10 +4735,8 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > > btrfs_set_trans_block_group(trans, dir); > > > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > > - dentry->d_name.len, > > - dentry->d_parent->d_inode->i_ino, > > - objectid, BTRFS_I(dir)->block_group, mode, > > - &index); > > + dentry->d_name.len, dir->i_ino, objectid, > > + BTRFS_I(dir)->block_group, mode, &index); > > err = PTR_ERR(inode); > > if (IS_ERR(inode)) > > goto out_unlock; > > @@ -4750,7 +4748,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > > } > > > > btrfs_set_trans_block_group(trans, inode); > > - err = btrfs_add_nondir(trans, dentry, inode, 0, index); > > + err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index); > > if (err) > > drop_inode = 1; > > else { > > @@ -4810,15 +4808,17 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir, > > btrfs_set_trans_block_group(trans, dir); > > atomic_inc(&inode->i_count); > > > > - err = btrfs_add_nondir(trans, dentry, inode, 1, index); > > + err = btrfs_add_nondir(trans, dir, dentry, inode, 1, index); > > > > if (err) { > > drop_inode = 1; > > } else { > > + struct dentry *parent = dget_parent(dentry); > > btrfs_update_inode_block_group(trans, dir); > > err = btrfs_update_inode(trans, root, inode); > > BUG_ON(err); > > - btrfs_log_new_name(trans, inode, NULL, dentry->d_parent); > > + btrfs_log_new_name(trans, inode, NULL, parent); > > + dput(parent); > > } > > > > nr = trans->blocks_used; > > @@ -4858,8 +4858,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, int mode) > > btrfs_set_trans_block_group(trans, dir); > > > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > > - dentry->d_name.len, > > - dentry->d_parent->d_inode->i_ino, objectid, > > + dentry->d_name.len, dir->i_ino, objectid, > > BTRFS_I(dir)->block_group, S_IFDIR | mode, > > &index); > > if (IS_ERR(inode)) { > > @@ -4882,9 +4881,8 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, int mode) > > if (err) > > goto out_fail; > > > > - err = btrfs_add_link(trans, dentry->d_parent->d_inode, > > - inode, dentry->d_name.name, > > - dentry->d_name.len, 0, index); > > + err = btrfs_add_link(trans, dir, inode, dentry->d_name.name, > > + dentry->d_name.len, 0, index); > > if (err) > > goto out_fail; > > > > @@ -6613,8 +6611,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, > > BUG_ON(ret); > > > > if (old_inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) { > > + struct dentry *parent = dget_parent(new_dentry); > > + > > btrfs_log_new_name(trans, old_inode, old_dir, > > - new_dentry->d_parent); > > + parent); > > + dput(parent); > > btrfs_end_log_trans(root); > > } > > out_fail: > > @@ -6764,8 +6765,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > > btrfs_set_trans_block_group(trans, dir); > > > > inode = btrfs_new_inode(trans, root, dir, dentry->d_name.name, > > - dentry->d_name.len, > > - dentry->d_parent->d_inode->i_ino, objectid, > > + dentry->d_name.len, dir->i_ino, objectid, > > BTRFS_I(dir)->block_group, S_IFLNK|S_IRWXUGO, > > &index); > > err = PTR_ERR(inode); > > @@ -6779,7 +6779,7 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > > } > > > > btrfs_set_trans_block_group(trans, inode); > > - err = btrfs_add_nondir(trans, dentry, inode, 0, index); > > + err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index); > > if (err) > > drop_inode = 1; > > else { > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index e264072..396ccd1 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -232,13 +232,17 @@ static noinline int create_subvol(struct btrfs_root *root, > > struct btrfs_inode_item *inode_item; > > struct extent_buffer *leaf; > > struct btrfs_root *new_root; > > - struct inode *dir = dentry->d_parent->d_inode; > > + struct dentry *parent = dget_parent(dentry); > > + struct inode *dir; > > int ret; > > int err; > > u64 objectid; > > u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID; > > u64 index = 0; > > > > + dir = parent->d_inode; > > + dput(parent); > > I get that parent is, well, the parent so the child were using will hold > a reference to it and so the dput() should be safe. But, since we have > the reference why not hold it while we use the inode to make certain the > inode cannot go away? >Yeah good point, I wasn''t entirely sure how to deal with these cases so I just made something up. I''m happy to have a more solid alternative. Thanks for the review, I''ll fix all this up at a more reasonable hour, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-10-28 at 02:43 -0400, Josef Bacik wrote:> On Thu, Oct 28, 2010 at 11:42:04AM +0800, Ian Kent wrote: > > On Wed, 2010-10-27 at 10:39 -0400, Josef Bacik wrote: > > > There are lots of places where we do dentry->d_parent->d_inode without holding > > > the dentry->d_lock. This could cause problems with rename. So instead use > > > dget_parent where we can, or in some cases we don''t even need to use > > > dentry->d_parent->d_inode since we get the inode of the dir passed to us from > > > VFS. I tested this with xfstests and my no space tests and everything turned > > > out fine. Thanks, > > > > > > Signed-off-by: Josef Bacik <josef@redhat.com> > > > --- > > > fs/btrfs/file.c | 2 ++ > > > fs/btrfs/inode.c | 48 ++++++++++++++++++++++++------------------------ > > > fs/btrfs/ioctl.c | 11 +++++++++-- > > > fs/btrfs/transaction.c | 5 ++++- > > > fs/btrfs/tree-log.c | 22 ++++++++++++++++++---- > > > 5 files changed, 57 insertions(+), 31 deletions(-) > > > > > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > > > index e354c33..6a4daa0 100644 > > > --- a/fs/btrfs/file.c > > > +++ b/fs/btrfs/file.c > > > @@ -1047,8 +1047,10 @@ out: > > > > > > if ((file->f_flags & O_DSYNC) || IS_SYNC(inode)) { > > > trans = btrfs_start_transaction(root, 0); > > > > If btrfs_start_transaction() fails now the inode mutex will be held as > > well. I guess the resulting oops makes this irrelevant, ;) > > > > Looking through the dead end BUG_ON()s in the transaction code, and > > callers, one thing I''ve found difficult is working out how to recover > > from IS_ERR() returns from btrfs_start_transaction() in situations like > > this. > > > > Any chance of a patch to handle this to give me an example (well one > > case anyway) of how to do it? > > > > Yup I can do that. Recovering from some of these errors is going to be tricky, > if you are going to work on that what I had in mind was doing alot ofYep, looks downright difficult to me, but it has to be done sometime. I''ve looked at a few functions in relation to eliminating dead end BUG_ON()s (loosely related to this), some at the bottom of the call tree upward and others at the top of the call tree downward, and it doesn''t take much to realize why no-one has tackled it already. The bigger issue is that the problem is self perpetuating in that, in order to fix things, often one has to continue to not handle some of the error cases because of possible side effects, ouch! Ian -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html