Re: [PATCH v3 3/4] btrfs: remove free space items when creating free space tree

[Boris Burkov <boris@bur.io>]

On Mon, Sep 21, 2020 at 07:13:04PM +0200, David Sterba wrote:
> On Thu, Sep 17, 2020 at 11:13:40AM -0700, Boris Burkov wrote:
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3333,6 +3333,15 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >  			close_ctree(fs_info);
> >  			return ret;
> >  		}
> > +		/*
> > +		 * Creating the free space tree creates inode orphan items and
> > +		 * delayed iputs when it deletes the free space inodes. Later in
> > +		 * open_ctree, we run btrfs_orphan_cleanup which tries to clean
> > +		 * up the orphan items. However, the outstanding references on
> > +		 * the inodes from the delayed iputs causes the cleanup to fail.
> > +		 * To fix it, force going through the delayed iputs here.
> > +		 */
> > +		btrfs_run_delayed_iputs(fs_info);
> 
> This is called from open_ctree, so this is mount context and the free
> space tree creation is called before that. That will schedule all free
> space inodes for deletion and waits here. This takes time proportional
> to the filesystem size.
> 
> We've had reports that this takes a lot of time already, so I wonder if
> the delayed iputs can be avoided here.

Good point. The way I see it, deleting the inodes creates legitimate
orphan items, and the delayed_iput doesn't go down until after
open_ctree, I assume for the reason you give. So to delete the free
space inodes without incurring the iput cost during mount, we could:

1. make orphan cleanup smart enough to tell the difference between
orphan items left over from an old mount, vs. orphan items created while
mounting.

2. move orphan cleanup to before free space tree creation/free space
inode deletion in open_ctree.

I'll try to think of a way to do 1. or think of some option 3, as 2.
seems a bit hacky/fragile.

While thinking about a legitimate case for orphans, I realized that
unconditionally failing on free space inode delete errors in
create_free_space_tree without handling ENOENT is almost certainly
wrong, so I will likely need to resend this patch anyway.