| Message ID | f566e1432b19f82d9c647b1c0e8e43743818bd7a.1686219923.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: some fixes and updates around handling errors for tree mod log operations | expand |
On 2023/6/8 18:27, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At balance_level(), when trying to promote a child node to a root node, if > we fail to read the child we call btrfs_handle_fs_error(), which turns the > fs to RO mode and sets it to error state as well, causing any ongoing > transaction to abort. This however is not necessary because at that point > we have not made any change yet at balance_level(), so any error reading > the child node does not leaves us in any inconsistent state. Therefore we > can just return the error to the caller. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Although I'd like to add some comments on the error handling. The catch here is, we can only hit the branch because @mid is already the highest tree block of the path. Thus the path has no CoWed tree block in it at all. If the condition is not met, we will return an error while some CoWed tree blocks are still in the path. In that case, a simple btrfs_release_path() will only reduce the refs and unlock, but not remove the delayed refs. Thus this is more like an exception, other locations can not follow the practice here. Thanks, Qu > --- > fs/btrfs/ctree.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index e98f9e205e25..4dcdcf25c3fe 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1040,7 +1040,6 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, > child = btrfs_read_node_slot(mid, 0); > if (IS_ERR(child)) { > ret = PTR_ERR(child); > - btrfs_handle_fs_error(fs_info, ret, NULL); > goto out; > } >
On Thu, Jun 8, 2023 at 11:51 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2023/6/8 18:27, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At balance_level(), when trying to promote a child node to a root node, if > > we fail to read the child we call btrfs_handle_fs_error(), which turns the > > fs to RO mode and sets it to error state as well, causing any ongoing > > transaction to abort. This however is not necessary because at that point > > we have not made any change yet at balance_level(), so any error reading > > the child node does not leaves us in any inconsistent state. Therefore we > > can just return the error to the caller. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although I'd like to add some comments on the error handling. > > The catch here is, we can only hit the branch because @mid is already > the highest tree block of the path. > Thus the path has no CoWed tree block in it at all. Even if it's not the highest level, there's no problem at all. COWing blocks without doing anything else doesn't leave a tree in an inconsistent state, as long as each parent points to the new (COWed) child. > > If the condition is not met, we will return an error while some CoWed > tree blocks are still in the path. As said before, the COWed blocks are fine, the tree is consistent as long as each parent points to the new blocks. > In that case, a simple btrfs_release_path() will only reduce the refs > and unlock, but not remove the delayed refs. btrfs_release_path() is never responsible for adding delayed blocks. That happens during COW, when we call btrfs_free_tree_block(). > > Thus this is more like an exception, other locations can not follow the > practice here. > > Thanks, > Qu > > > --- > > fs/btrfs/ctree.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > > index e98f9e205e25..4dcdcf25c3fe 100644 > > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -1040,7 +1040,6 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, > > child = btrfs_read_node_slot(mid, 0); > > if (IS_ERR(child)) { > > ret = PTR_ERR(child); > > - btrfs_handle_fs_error(fs_info, ret, NULL); > > goto out; > > } > >
On 2023/6/8 19:00, Filipe Manana wrote: > On Thu, Jun 8, 2023 at 11:51 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> On 2023/6/8 18:27, fdmanana@kernel.org wrote: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> At balance_level(), when trying to promote a child node to a root node, if >>> we fail to read the child we call btrfs_handle_fs_error(), which turns the >>> fs to RO mode and sets it to error state as well, causing any ongoing >>> transaction to abort. This however is not necessary because at that point >>> we have not made any change yet at balance_level(), so any error reading >>> the child node does not leaves us in any inconsistent state. Therefore we >>> can just return the error to the caller. >>> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> >> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> Although I'd like to add some comments on the error handling. >> >> The catch here is, we can only hit the branch because @mid is already >> the highest tree block of the path. >> Thus the path has no CoWed tree block in it at all. > > Even if it's not the highest level, there's no problem at all. > COWing blocks without doing anything else doesn't leave a tree in an > inconsistent state, > as long as each parent points to the new (COWed) child. Oh, you're right, I forgot that the newly COWed tree blocks should always be accessible from the root node. There is no problem at all from the very beginning. Thanks, Qu > >> >> If the condition is not met, we will return an error while some CoWed >> tree blocks are still in the path. > > As said before, the COWed blocks are fine, the tree is consistent as > long as each > parent points to the new blocks. > >> In that case, a simple btrfs_release_path() will only reduce the refs >> and unlock, but not remove the delayed refs. > > btrfs_release_path() is never responsible for adding delayed blocks. > That happens during COW, when we call btrfs_free_tree_block(). > >> >> Thus this is more like an exception, other locations can not follow the >> practice here. >> >> Thanks, >> Qu >> >>> --- >>> fs/btrfs/ctree.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c >>> index e98f9e205e25..4dcdcf25c3fe 100644 >>> --- a/fs/btrfs/ctree.c >>> +++ b/fs/btrfs/ctree.c >>> @@ -1040,7 +1040,6 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, >>> child = btrfs_read_node_slot(mid, 0); >>> if (IS_ERR(child)) { >>> ret = PTR_ERR(child); >>> - btrfs_handle_fs_error(fs_info, ret, NULL); >>> goto out; >>> } >>>
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index e98f9e205e25..4dcdcf25c3fe 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1040,7 +1040,6 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, child = btrfs_read_node_slot(mid, 0); if (IS_ERR(child)) { ret = PTR_ERR(child); - btrfs_handle_fs_error(fs_info, ret, NULL); goto out; }