| Message ID | 1406934766-16974-4-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 01, 2014 at 06:12:37PM -0500, Eric Sandeen wrote: > Reading the quota tree root may fail with ENOENT > if there is no quota, which is fine, but the code was > ignoring every other error as well, which is not fine. Kinda makes you want to write a test that would have caught this. Kinda. Also, if you're still keen to iterate on this series, it looks like this pattern is copied and pasted a few times in open_ctree(). With temporary root pointers for each block, for some reason. A little helper function could take a bite out of open_ctree(). - z -- 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 8/4/14, 1:35 PM, Zach Brown wrote: > On Fri, Aug 01, 2014 at 06:12:37PM -0500, Eric Sandeen wrote: >> Reading the quota tree root may fail with ENOENT >> if there is no quota, which is fine, but the code was >> ignoring every other error as well, which is not fine. > > Kinda makes you want to write a test that would have caught this. > > Kinda. /me looks at ground, shuffles feet ... > Also, if you're still keen to iterate on this series, it looks like this > pattern is copied and pasted a few times in open_ctree(). With > temporary root pointers for each block, for some reason. A little > helper function could take a bite out of open_ctree(). Hm, the uuid tree is roughly similar, but not exactly. I think those are the only 2 "optional" roots (uuid because it'll get regenerated). I'm guessing the temporary root pointer is so we don't ever assign a PTR_ERR to the root in fs_info? -Eric -- 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 Mon, Aug 04, 2014 at 01:42:23PM -0500, Eric Sandeen wrote: > On 8/4/14, 1:35 PM, Zach Brown wrote: > > On Fri, Aug 01, 2014 at 06:12:37PM -0500, Eric Sandeen wrote: > >> Reading the quota tree root may fail with ENOENT > >> if there is no quota, which is fine, but the code was > >> ignoring every other error as well, which is not fine. > > > > Kinda makes you want to write a test that would have caught this. > > > > Kinda. > > /me looks at ground, shuffles feet ... > > > Also, if you're still keen to iterate on this series, it looks like this > > pattern is copied and pasted a few times in open_ctree(). With > > temporary root pointers for each block, for some reason. A little > > helper function could take a bite out of open_ctree(). > > Hm, the uuid tree is roughly similar, but not exactly. I think those > are the only 2 "optional" roots (uuid because it'll get regenerated). > > I'm guessing the temporary root pointer is so we don't ever assign a > PTR_ERR to the root in fs_info? It took me a while to see what you meant. Yeah, using a temporary root makes sense. Using a different one for each block makes less sense. a = f(A); if (a) goto out; info->a = a; b = f(B); if (b) goto out; info->b = b; vs. r = f(A); if (r) goto out; info->a = r; r = f(B); if (r) goto out; info->b = r; - z -- 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 8/4/14, 1:51 PM, Zach Brown wrote: > On Mon, Aug 04, 2014 at 01:42:23PM -0500, Eric Sandeen wrote: >> On 8/4/14, 1:35 PM, Zach Brown wrote: >>> On Fri, Aug 01, 2014 at 06:12:37PM -0500, Eric Sandeen wrote: >>>> Reading the quota tree root may fail with ENOENT >>>> if there is no quota, which is fine, but the code was >>>> ignoring every other error as well, which is not fine. >>> >>> Kinda makes you want to write a test that would have caught this. >>> >>> Kinda. >> >> /me looks at ground, shuffles feet ... >> >>> Also, if you're still keen to iterate on this series, it looks like this >>> pattern is copied and pasted a few times in open_ctree(). With >>> temporary root pointers for each block, for some reason. A little >>> helper function could take a bite out of open_ctree(). >> >> Hm, the uuid tree is roughly similar, but not exactly. I think those >> are the only 2 "optional" roots (uuid because it'll get regenerated). >> >> I'm guessing the temporary root pointer is so we don't ever assign a >> PTR_ERR to the root in fs_info? > > It took me a while to see what you meant. > > Yeah, using a temporary root makes sense. Using a different one for > each block makes less sense. > <snip> > - z > Yeah, fair enough, I thought about that after I hit send ;) I could send a V2 of patch 11/12 to do that w/o needing to redo the series too much. :) I'll see if there are any other comments. Thanks! -Eric -- 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
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index e6746be..28d35a8 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2733,7 +2733,12 @@ retry_root_backup: location.objectid = BTRFS_QUOTA_TREE_OBJECTID; quota_root = btrfs_read_tree_root(tree_root, &location); - if (!IS_ERR(quota_root)) { + if (IS_ERR(quota_root)) { + ret = PTR_ERR(quota_root); + /* It's fine to not have quotas */ + if (ret != -ENOENT) + goto recovery_tree_root; + } else { set_bit(BTRFS_ROOT_TRACK_DIRTY, "a_root->state); fs_info->quota_enabled = 1; fs_info->pending_quota_state = 1;
Reading the quota tree root may fail with ENOENT if there is no quota, which is fine, but the code was ignoring every other error as well, which is not fine. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/btrfs/disk-io.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)