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

[David Sterba <dsterba@suse.cz>]

On Mon, Nov 30, 2020 at 03:31:42PM -0800, Boris Burkov wrote:
> 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 checking, the committed changelog does not contain 'leak' and
I slighthly rephrased only that sentence.