[PATCH] btrfs: don't force read-only after error in drop snapshot

[PATCH] btrfs: don't force read-only after error in drop snapshot

[David Sterba]

Deleting a subvolume on a full filesystem leads to ENOSPC followed by a
forced read-only. This is not a transaction abort and the filesystem is
otherwise ok, so the error should be just propagated.

This is caused by unnecessary call to btrfs_handle_fs_error for almost
all errors, except EAGAIN. This does not make sense as the standard
transaction abort mechanism is in btrfs_drop_snapshot so all relevant
failures are handled.

Originally in commit cb1b69f4508a ("Btrfs: forced readonly when
btrfs_drop_snapshot() fails") there was no return value at all, so the
btrfs_std_error made some sense but once the error handling and
propagation has been we don't need it.

Signed-off-by: David Sterba <dsterba@suse.com>
---

The use of btrfs_handle_fs_error in other places looks fishy, it makes
sense only in case there's a real error and transaction abort is not
possible, ~40 calls sound too much.

 fs/btrfs/extent-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 161274118853..b18db1b3a412 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5426,8 +5426,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
 	 */
 	if (!for_reloc && !root_dropped)
 		btrfs_add_dead_root(root);
-	if (err && err != -EAGAIN)
-		btrfs_handle_fs_error(fs_info, err, NULL);
 	return err;
 }
 
-- 
2.25.0

Re: [PATCH] btrfs: don't force read-only after error in drop snapshot

[Anand Jain]

On 25/2/20 10:05 PM, David Sterba wrote:
> Deleting a subvolume on a full filesystem leads to ENOSPC followed by a
> forced read-only. This is not a transaction abort and the filesystem is
> otherwise ok, so the error should be just propagated.

yep.

> This is caused by unnecessary call to btrfs_handle_fs_error for almost
> all errors, except EAGAIN. This does not make sense as the standard
> transaction abort mechanism is in btrfs_drop_snapshot so all relevant
> failures are handled.
> 
> Originally in commit cb1b69f4508a ("Btrfs: forced readonly when
> btrfs_drop_snapshot() fails") there was no return value at all, so the
> btrfs_std_error made some sense but once the error handling and
> propagation has been we don't need it.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> The use of btrfs_handle_fs_error in other places looks fishy, it makes
> sense only in case there's a real error and transaction abort is not
> possible, ~40 calls sound too much.
> 
>   fs/btrfs/extent-tree.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 161274118853..b18db1b3a412 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -5426,8 +5426,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
>   	 */
>   	if (!for_reloc && !root_dropped)
>   		btrfs_add_dead_root(root);
> -	if (err && err != -EAGAIN)
> -		btrfs_handle_fs_error(fs_info, err, NULL);
>   	return err;
>   }

However can we confirm that the error returned here are logged by its 
parents (relocation thread and the cleaner thread) ?

Thanks, Anand

Re: [PATCH] btrfs: don't force read-only after error in drop snapshot

[David Sterba]

On Wed, Feb 26, 2020 at 12:35:37PM +0800, Anand Jain wrote:
> On 25/2/20 10:05 PM, David Sterba wrote:
> > Deleting a subvolume on a full filesystem leads to ENOSPC followed by a
> > forced read-only. This is not a transaction abort and the filesystem is
> > otherwise ok, so the error should be just propagated.
> 
> yep.
> 
> > This is caused by unnecessary call to btrfs_handle_fs_error for almost
> > all errors, except EAGAIN. This does not make sense as the standard
> > transaction abort mechanism is in btrfs_drop_snapshot so all relevant
> > failures are handled.
> > 
> > Originally in commit cb1b69f4508a ("Btrfs: forced readonly when
> > btrfs_drop_snapshot() fails") there was no return value at all, so the
> > btrfs_std_error made some sense but once the error handling and
> > propagation has been we don't need it.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> > 
> > The use of btrfs_handle_fs_error in other places looks fishy, it makes
> > sense only in case there's a real error and transaction abort is not
> > possible, ~40 calls sound too much.
> > 
> >   fs/btrfs/extent-tree.c | 2 --
> >   1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 161274118853..b18db1b3a412 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -5426,8 +5426,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
> >   	 */
> >   	if (!for_reloc && !root_dropped)
> >   		btrfs_add_dead_root(root);
> > -	if (err && err != -EAGAIN)
> > -		btrfs_handle_fs_error(fs_info, err, NULL);
> >   	return err;
> >   }
> 
> However can we confirm that the error returned here are logged by its 
> parents (relocation thread and the cleaner thread) ?

What do you mean logged? The error is propagated to the callers.

In btrfs_clean_one_deleted_snapshot it will result in not looping again
to clean futher subvolumes, silent error should be ok here.

In clean_dirty_subvols the last error is propagated as the loop
continues until all subvolumes from the list are processed, but reloc
dropping snapshot can handle EAGAIN and ENOSPC will be the same.

Re: [PATCH] btrfs: don't force read-only after error in drop snapshot

[David Sterba]

On Tue, Feb 25, 2020 at 03:05:53PM +0100, David Sterba wrote:
> Deleting a subvolume on a full filesystem leads to ENOSPC followed by a
> forced read-only. This is not a transaction abort and the filesystem is
> otherwise ok, so the error should be just propagated.
> 
> This is caused by unnecessary call to btrfs_handle_fs_error for almost
> all errors, except EAGAIN. This does not make sense as the standard
> transaction abort mechanism is in btrfs_drop_snapshot so all relevant
> failures are handled.
> 
> Originally in commit cb1b69f4508a ("Btrfs: forced readonly when
> btrfs_drop_snapshot() fails") there was no return value at all, so the
> btrfs_std_error made some sense but once the error handling and
> propagation has been we don't need it.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

I'm hitting the ENOSPC and now also EINTR after the fast balance cancel
patches so this is needed. If anybody has still concerns, please let me
know, patch goes to misc-next.