Re: [PATCH 2/2] btrfs: warn about dev extents that are inside the reserved range

[Boris Burkov <boris@bur.io>]

On Tue, Jun 14, 2022 at 03:48:06PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/6/14 03:05, Boris Burkov wrote:
> > On Mon, Jun 13, 2022 at 03:06:35PM +0800, Qu Wenruo wrote:
> > > Btrfs has reserved the first 1MiB for the primary super block (at 64KiB
> > > offset) and legacy programs like older bootloaders.
> > > 
> > > This behavior is only introduced since v4.1 btrfs-progs release,
> > > although kernel can ensure we never touch the reserved range of super
> > > blocks, it's better to inform the end users, and a balance will resolve
> > > the problem.
> > > 
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > ---
> > >   fs/btrfs/volumes.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > index 051d124679d1..b39f4030d2ba 100644
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -7989,6 +7989,16 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
> > >   		goto out;
> > >   	}
> > > 
> > > +	/*
> > > +	 * Very old mkfs.btrfs (before v4.1) will not respect the reserved
> > > +	 * space. Although kernel can handle it without problem, better to
> > > +	 * warn the users.
> > > +	 */
> > > +	if (physical_offset < BTRFS_DEFAULT_RESERVED)
> > > +		btrfs_warn(fs_info,
> > > +"devid %llu physical %llu len %llu is inside the reserved space, balance is needed to solve this problem.",
> > 
> > If I saw this warning, I wouldn't know what balance to run, and it's
> > not obvious what to search for online either (if it's even documented).
> > I think a more explicit instruction like "btrfs balance start XXXX"
> > would be helpful.
> 
> Firstly, the balance command needs extra filters, thus the command can
> be pretty long, like:
> 
> # btrfs balance start -mdrange=0..1048576 -ddrange=0..1048576
> -srange0..1048576 <mnt>
> 
> I'm not sure if this is a good idea to put all these into the already
> long message.
> 
> > 
> > If it's something we're ok with in general, then maybe a URL for a wiki
> > page that explains the issue and the workaround would be the most
> > useful.
> 
> URL can be helpful but not always. Imagine a poor sysadmin in a noisy
> server room, seeing a URL in dmesg, and has to type the full URL into
> their phone, if the server has very limited network access.

I don't see how the poor sysadmin would be any better off with "you need
to do a balance" vs "you need to do a balance: <URL>" or "you need to do
a balance using mdrange and ddrange to move the affected extents" etc..

My high level point is that you clearly have something in mind that the
person needs to do in the unlikely event they hit this, but I have no
idea how they are supposed to figure it out. Send a mail to our mailing
list and hope you notice it?

> 
> In fact, this error message for now will be super rare already.
> 
> The main usage of this message is for the incoming feature, which will
> allow btrfs to reserve extra space for its internal usage.
> 
> In that case, we will allow btrfstune to set the reservation (even it's
> already used by some dev extent), and btrfstune would give a commandline
> how to do the balance.
> 
> I guess I'd put all these preparation patches into the incoming on-disk
> format change patchset to make it clear.
> 
> Thanks,
> Qu
> 
> > 
> > > +			   devid, physical_offset, physical_len);
> > > +
> > >   	for (i = 0; i < map->num_stripes; i++) {
> > >   		if (map->stripes[i].dev->devid == devid &&
> > >   		    map->stripes[i].physical == physical_offset) {
> > > --
> > > 2.36.1
> > >