Re: [PATCH] btrfs: do not clear read-only when adding sprout device

[David Sterba <dsterba@suse.cz>]

On Thu, Oct 17, 2024 at 09:41:59AM -0700, Boris Burkov wrote:
> On Thu, Oct 17, 2024 at 04:01:12PM +0200, David Sterba wrote:
> > On Tue, Oct 15, 2024 at 02:38:34PM -0700, Boris Burkov wrote:
> > > If you follow the seed/sprout wiki, it suggests the following workflow:
> > > 
> > > btrfstune -S 1 seed_dev
> > > mount seed_dev mnt
> > > btrfs device add sprout_dev
> > > mount -o remount,rw mnt
> > > 
> > > The first mount mounts the FS readonly, which results in not setting
> > > BTRFS_FS_OPEN, and setting the readonly bit on the sb. The device add
> > > somewhat surprisingly clears the readonly bit on the sb (though the
> > > mount is still practically readonly, from the users perspective...).
> > > Finally, the remount checks the readonly bit on the sb against the flag
> > > and sees no change, so it does not run the code intended to run on
> > > ro->rw transitions, leaving BTRFS_FS_OPEN unset.
> > > 
> > > As a result, when the cleaner_kthread runs, it sees no BTRFS_FS_OPEN and
> > > does no work. This results in leaking deleted snapshots until we run out
> > > of space.
> > > 
> > > I propose fixing it at the first departure from what feels reasonable:
> > > when we clear the readonly bit on the sb during device add.
> > > 
> > > A new fstest I have written reproduces the bug and confirms the fix.
> > > 
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > ---
> > > Note that this is a resend of an old unmerged fix:
> > > https://lore.kernel.org/linux-btrfs/16c05d39566858bb8bc1e03bd19947cf2b601b98.1647906815.git.boris@bur.io/T/#u
> > > Some other ideas for fixing it by modifying how we set BTRFS_FS_OPEN
> > > were also explored but not merged around that time:
> > > https://lore.kernel.org/linux-btrfs/cover.1654216941.git.anand.jain@oracle.com/
> > > 
> > > I don't have a strong preference, but I would really like to see this
> > > trivial bug fixed. For what it is worth, we have been carrying this
> > > patch internally at Meta since I first sent it with no incident.
> > 
> > We have an unresolved dispute about the fix and now the patch got to
> > for-next within a few days because it got two reviews but not mine.
> > The way you use it in Meta works for you but the fix is changing
> > behaviour so we can either ignore everybody else relying on the old
> > way or say that seeding is so niche that we don't care and see what we
> > can do once we get some report.
> 
> Please feel free to remove it, and I am happy to review whatever other
> fix you think is best. Sorry for rushing, I just wanted to get it done
> and out of my head so I could move on to other issues.

I'm concerned because change like that needs an announcement,
documentation changes or eventually an optional change so both use cases
are available. I haven't merged it since the last time you or somebody
posted it because I don't see a way how to fix it without fixing the bug
and not breaking the use case.

> I assume your concern is that before this fix, the filesystem is actually
> read-write after the device add, which this patch breaks?
> 
> My only argument against this is that the documentation has always said
> you needed to remount/cycle-mount after adding the sprout, so there is
> no fair reason to assume this would work. (In fact, it *doesn't*, the fs
> is once again in a unexpected, degraded, state...)
> 
> But if existing LiveCD users are relying on this undocumented behavior,
> then I think you are right and it's a bad idea to break them.

The problem here is that we don't know how the feature is used, the
documentation came much later than the feature. So I take it as that
people rely on code, not documenation, even if there's an undocumented
behaviour.