| Message ID | 20200124143301.2186319-28-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup how we handle root refs, part 1 | expand |
On Fri, Jan 24, 2020 at 09:32:44AM -0500, Josef Bacik wrote: > We look up the fs root in various places in here when recovering from a > crashed relcoation. Make sure we hold a ref on the root whenever we > look them up. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/relocation.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 9531739b5a8c..81f383df8f63 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4593,6 +4593,10 @@ int btrfs_recover_relocation(struct btrfs_root *root) > if (btrfs_root_refs(&reloc_root->root_item) > 0) { > fs_root = read_fs_root(fs_info, > reloc_root->root_key.offset); > + if (!btrfs_grab_fs_root(fs_root)) { > + err = -ENOENT; > + goto out; > + } > if (IS_ERR(fs_root)) { Also in the wrong order. > ret = PTR_ERR(fs_root); > if (ret != -ENOENT) {
On Fri, Jan 24, 2020 at 09:32:44AM -0500, Josef Bacik wrote: > @@ -4593,6 +4593,10 @@ int btrfs_recover_relocation(struct btrfs_root *root) > if (btrfs_root_refs(&reloc_root->root_item) > 0) { > fs_root = read_fs_root(fs_info, > reloc_root->root_key.offset); > + if (!btrfs_grab_fs_root(fs_root)) { > + err = -ENOENT; > + goto out; > + } > if (IS_ERR(fs_root)) { > ret = PTR_ERR(fs_root); > if (ret != -ENOENT) { > @@ -4604,6 +4608,8 @@ int btrfs_recover_relocation(struct btrfs_root *root) > err = ret; > goto out; > } > + } else { > + btrfs_put_fs_root(fs_root); > } > } The order of IS_ERR and btrfs_grab_fs_root is reversed but then it looks strange: 4637 fs_root = read_fs_root(fs_info, _ 4638 reloc_root->root_key.offset); 4639 if (IS_ERR(fs_root)) { 4640 ret = PTR_ERR(fs_root); 4641 if (ret != -ENOENT) { 4642 err = ret; 4643 goto out; 4644 } 4645 ret = mark_garbage_root(reloc_root); 4646 if (ret < 0) { 4647 err = ret; 4648 goto out; 4649 } 4650 } else { + 4651 if (!btrfs_grab_fs_root(fs_root)) { + 4652 err = -ENOENT; + 4653 goto out; + 4654 } 4655 btrfs_put_fs_root(fs_root); 4656 } 4657 } Seems that the refcounting is not necessary here at all, it just tries to read the fs root and handle errors if it does not exist, no operation that would want to keep the fs_root.
On 2/6/20 11:26 AM, David Sterba wrote: > On Fri, Jan 24, 2020 at 09:32:44AM -0500, Josef Bacik wrote: >> @@ -4593,6 +4593,10 @@ int btrfs_recover_relocation(struct btrfs_root *root) >> if (btrfs_root_refs(&reloc_root->root_item) > 0) { >> fs_root = read_fs_root(fs_info, >> reloc_root->root_key.offset); >> + if (!btrfs_grab_fs_root(fs_root)) { >> + err = -ENOENT; >> + goto out; >> + } >> if (IS_ERR(fs_root)) { >> ret = PTR_ERR(fs_root); >> if (ret != -ENOENT) { >> @@ -4604,6 +4608,8 @@ int btrfs_recover_relocation(struct btrfs_root *root) >> err = ret; >> goto out; >> } >> + } else { >> + btrfs_put_fs_root(fs_root); >> } >> } > > The order of IS_ERR and btrfs_grab_fs_root is reversed but then it looks > strange: > > 4637 fs_root = read_fs_root(fs_info, > _ 4638 reloc_root->root_key.offset); > 4639 if (IS_ERR(fs_root)) { > 4640 ret = PTR_ERR(fs_root); > 4641 if (ret != -ENOENT) { > 4642 err = ret; > 4643 goto out; > 4644 } > 4645 ret = mark_garbage_root(reloc_root); > 4646 if (ret < 0) { > 4647 err = ret; > 4648 goto out; > 4649 } > 4650 } else { > + 4651 if (!btrfs_grab_fs_root(fs_root)) { > + 4652 err = -ENOENT; > + 4653 goto out; > + 4654 } > 4655 btrfs_put_fs_root(fs_root); > 4656 } > 4657 } > > Seems that the refcounting is not necessary here at all, it just tries > to read the fs root and handle errors if it does not exist, no operation > that would want to keep the fs_root. > Yeah we aren't actually using the root here, so strictly speaking we don't need the refcount. But in the future read_fs_root() will return a root with a refcount so we will still have to clean it up. Thanks, Josef
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 9531739b5a8c..81f383df8f63 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4593,6 +4593,10 @@ int btrfs_recover_relocation(struct btrfs_root *root) if (btrfs_root_refs(&reloc_root->root_item) > 0) { fs_root = read_fs_root(fs_info, reloc_root->root_key.offset); + if (!btrfs_grab_fs_root(fs_root)) { + err = -ENOENT; + goto out; + } if (IS_ERR(fs_root)) { ret = PTR_ERR(fs_root); if (ret != -ENOENT) { @@ -4604,6 +4608,8 @@ int btrfs_recover_relocation(struct btrfs_root *root) err = ret; goto out; } + } else { + btrfs_put_fs_root(fs_root); } } @@ -4653,10 +4659,15 @@ int btrfs_recover_relocation(struct btrfs_root *root) list_add_tail(&reloc_root->root_list, &reloc_roots); goto out_free; } + if (!btrfs_grab_fs_root(fs_root)) { + err = -ENOENT; + goto out_free; + } err = __add_reloc_root(reloc_root); BUG_ON(err < 0); /* -ENOMEM or logic error */ fs_root->reloc_root = reloc_root; + btrfs_put_fs_root(fs_root); } err = btrfs_commit_transaction(trans); @@ -4688,10 +4699,14 @@ int btrfs_recover_relocation(struct btrfs_root *root) if (err == 0) { /* cleanup orphan inode in data relocation tree */ fs_root = read_fs_root(fs_info, BTRFS_DATA_RELOC_TREE_OBJECTID); - if (IS_ERR(fs_root)) + if (IS_ERR(fs_root)) { err = PTR_ERR(fs_root); - else - err = btrfs_orphan_cleanup(fs_root); + } else { + if (btrfs_grab_fs_root(fs_root)) { + err = btrfs_orphan_cleanup(fs_root); + btrfs_put_fs_root(fs_root); + } + } } return err; }
We look up the fs root in various places in here when recovering from a crashed relcoation. Make sure we hold a ref on the root whenever we look them up. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)