| Message ID | c4b02ac7bf6e4171d8cfb13dcd11b3bad8d2e4df.1655103954.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: cleanup related to the 1MiB reserved space | expand |
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. 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. > + 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 >
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. 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 >>
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 > > >
On 2022/6/14 23:30, Boris Burkov wrote: > 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? I guess you miss the point here. First, this is really rare case, it need older mkfs.btrfs and never balanced the fs. Second, the warning message itself is fine, kernel is 100% fine handling it. The warning message can be ignored as long as there is no usage of legacy bootloader. > >> >> 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. In fact, that would be where the detailed balance command line to be shown. Btrfs check and btrfstune would output the detailed command line to do that. Thanks, Qu >> >> 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 >>>>
On Wed, Jun 15, 2022 at 06:12:29AM +0800, Qu Wenruo wrote: > On 2022/6/14 23:30, Boris Burkov wrote: > > 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? > > I guess you miss the point here. > > First, this is really rare case, it need older mkfs.btrfs and never > balanced the fs. > > Second, the warning message itself is fine, kernel is 100% fine handling > it. The warning message can be ignored as long as there is no usage of > legacy bootloader. > > > > >> > >> 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. > > In fact, that would be where the detailed balance command line to be shown. > > Btrfs check and btrfstune would output the detailed command line to do that. I don't think this is a good place either. There's a WIP page file:///home/ds/x/btrfs-progs/Documentation/_build/html/trouble-index.html that should be the starting point to explain errors or error messages in greater detail than what can be fit to one line. There is/was a similar page on wiki but not was used or lacked details.
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.", + 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) {
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(+)