Re: [PATCH v7 01/12] btrfs: lift rw mount setup from mount and remount

[Boris Burkov <boris@bur.io>]

On Mon, Nov 23, 2020 at 05:50:40PM +0100, David Sterba wrote:
> On Wed, Nov 18, 2020 at 03:06:16PM -0800, Boris Burkov wrote:
> > Mounting rw and remounting from ro to rw naturally share invariants and
> > functionality which result in a correctly setup rw filesystem. Luckily,
> > there is even a strong unity in the code which implements them. In
> > mount's open_ctree, these operations mostly happen after an early return
> > for ro file systems, and in remount, they happen in a section devoted to
> > remounting ro->rw, after some remount specific validation passes.
> > 
> > However, there are unfortunately a few differences. There are small
> > deviations in the order of some of the operations, remount does not
> > cleanup orphan inodes in root_tree or fs_tree, remount does not create
> > the free space tree, and remount does not handle "one-shot" mount
> > options like clear_cache and uuid tree rescan.
> > 
> > Since we want to add building the free space tree to remount, and since
> > it is possible to leak orphans on a filesystem mounted as ro then
> > remounted rw
> 
> The statement is not specific if the orphans are files or roots. But I
> don't agree that a leak is possible, or need a proof of the claim above.
> 
> The mount-time orphan cleanup will start early, but otherwise orphan
> cleanup is checked and started on dentry lookups (btrfs_lookup_dentry).
> Deleted but not clened tree roorts are all found and removed, regardless
> of rw or ro->rw mount.
> 
> So I wonder if you claim there's a leak just by lack of an explicit call
> on the remount path.

For what it's worth, the example I had in mind is the free space inode
orphans after a block_group delete or the new "clear v1 space cache"
code in this stack.

I hadn't considered btrfs_lookup_dentry because I was focused on those
specific inodes, but it's possible that gets called in a way that would
clean them too.

However, another thing I think I overlooked is that it doesn't look
like remount would affect the set of delayed_iputs, so that mechanism for
removing the orphans should still work. Further, the new function only
runs when going from ro->rw, but any ro mount would run delayed iputs
before completing as part of btrfs_commit_super.

So with all that, I agree with you that there isn't a leak. Going
forward with this, I can certainly fix the commit messages, or even get
rid of the patch that does the orphan cleanup in remount. I can't think
of a reason that the cleanup would be bad, but on the other hand, just
"unity" is a flimsy justification for adding it. Let me know what you
prefer.

Thanks for the review,
Boris