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

[Boris Burkov <boris@bur.io>]

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

As long as my test (and presumably some fix) goes in and I don't have to
carry this patch internally for two more years, then I am happy.

Thanks,
Boris