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@xxxxxxxx>
> > ---
> >
> > 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.
