Btrfs devel - Aug 2013 - [PATCH 0/2] btrfs-progs: prevent mkfs from aborting with small volume

I found that mkfs.btrfs aborts when one of assigned volume is too small.
Here are 2 patches to fix 2 independent problems.
Both are based on top of Chris''s btrfs-progs.git:

  git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git

Thanks,
H.Seto


Hidetoshi Seto (2):
      btrfs-progs: treat reserved 1MB for superblock properly
      btrfs-progs: exit if there is not enough free space for mkfs

 ctree.h   |    3 +++
 mkfs.c    |    4 ++++
 volumes.c |    7 ++++++-
 3 files changed, 13 insertions(+), 1 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I found that mkfs.btrfs aborts when assigned multi volumes contain
a small volume:

  # parted /dev/sdf p
  Model: LSI MegaRAID SAS RMB (scsi)
  Disk /dev/sdf: 72.8GB
  Sector size (logical/physical): 512B/512B
  Partition Table: msdos

  Number  Start   End     Size    Type     File system  Flags
   1      32.3kB  72.4GB  72.4GB  primary
   2      72.4GB  72.8GB  461MB   primary

  # ./mkfs.btrfs -f /dev/sdf1 /dev/sdf2
  :
  SMALL VOLUME: forcing mixed metadata/data groups
  adding device /dev/sdf2 id 2
  mkfs.btrfs: volumes.c:852: btrfs_alloc_chunk: Assertion `!(ret)''
failed.
  Aborted (core dumped)

This failure of btrfs_alloc_chunk was caused by following steps:
 1) since there is only small space in the small device, mkfs was
    going to allocate a chunk from free space as much as available.
    So mkfs called btrfs_alloc_chunk with
        size = device->total_bytes - device->used_bytes.
 2) To avoid overwriting superblock, btrfs_alloc_chunk starts taking
    chunks at an offset of 1MB. It means that the layout of a disk
    will be like:
     [[1MB at begging for sb][allocated chunks]* ... free space ... ]
    and you can see that the available free space for allocation is:
        avail = device->total_bytes - device->used_bytes - 1MB.
 3) Therefore there is only free space 1MB less than requested. damn.

So this fix let mkfs know how much spaces are really there.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 ctree.h   |    3 +++
 volumes.c |    7 ++++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/ctree.h b/ctree.h
index 0b0d701..791bd14 100644
--- a/ctree.h
+++ b/ctree.h
@@ -811,6 +811,9 @@ struct btrfs_csum_item {
 	u8 csum;
 } __attribute__ ((__packed__));
 
+/* we have reserved 1M for superblock at the begging of device */
+#define BTRFS_BLOCK_RESERVED_1M_FOR_SUPER	((u64)1024 * 1024)
+
 /* tag for the radix tree of block groups in ram */
 #define BTRFS_BLOCK_GROUP_DATA		(1ULL << 0)
 #define BTRFS_BLOCK_GROUP_SYSTEM	(1ULL << 1)
diff --git a/volumes.c b/volumes.c
index 0ff2283..bf6b2e1 100644
--- a/volumes.c
+++ b/volumes.c
@@ -283,7 +283,7 @@ static int find_free_dev_extent(struct btrfs_trans_handle
*trans,
 	/* we don''t want to overwrite the superblock on the drive,
 	 * so we make sure to start at an offset of at least 1MB
 	 */
-	search_start = max((u64)1024 * 1024, search_start);
+	search_start = max(BTRFS_BLOCK_RESERVED_1M_FOR_SUPER, search_start);
 
 	if (root->fs_info->alloc_start + num_bytes <= device->total_bytes)
 		search_start = max(root->fs_info->alloc_start, search_start);
@@ -783,6 +783,11 @@ again:
 	while(index < num_stripes) {
 		device = list_entry(cur, struct btrfs_device, dev_list);
 		avail = device->total_bytes - device->bytes_used;
+		/* we have reserved 1M for superblock at the head of device */
+		if (avail > BTRFS_BLOCK_RESERVED_1M_FOR_SUPER)
+			avail -= BTRFS_BLOCK_RESERVED_1M_FOR_SUPER;
+		else
+			avail = 0;
 		cur = cur->next;
 		if (avail >= min_free) {
 			list_move_tail(&device->dev_list, &private_devs);
-- 
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Again, while playing mkfs with small volumes, I found that mkfs.btrfs
aborts if there is really no spaces for a brand-new filesystem:

  # ./mkfs.btrfs -f /dev/sdf1 /dev/sdf2
  :
  SMALL VOLUME: forcing mixed metadata/data groups
  adding device /dev/sdf2 id 2
  mkfs.btrfs: mkfs.c:184: create_one_raid_group: Assertion `!(ret)''
failed.
  Aborted (core dumped)

This fix let mkfs prints error message if it cannot make filesystem due
to a lack of free spaces.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 mkfs.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index b412b7e..3c0bc60 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -181,6 +181,10 @@ static int create_one_raid_group(struct btrfs_trans_handle
*trans,
 
 	ret = btrfs_alloc_chunk(trans, root->fs_info->extent_root,
 				&chunk_start, &chunk_size, type);
+	if (ret == -ENOSPC) {
+		fprintf(stderr, "not enough free space\n");
+		exit(1);
+	}
 	BUG_ON(ret);
 	ret = btrfs_make_block_group(trans, root->fs_info->extent_root, 0,
 				     type, BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-- 
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 8/23/13 1:28 AM, Hidetoshi Seto wrote:
> I found that mkfs.btrfs aborts when one of assigned volume is too small.
> Here are 2 patches to fix 2 independent problems.
> Both are based on top of Chris''s btrfs-progs.git:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git
> 
> Thanks,
> H.Seto
> 
Thanks for looking into this - how small of a device did you test?

I tried a 2MB device w/ these 2 patches and still got:

[btrfs-progs]# truncate --size=2m testfile
[btrfs-progs]# ./mkfs.btrfs testfile

WARNING! - Btrfs v0.20-rc1-360-geeeb4e9 IS EXPERIMENTAL
WARNING! - see http://btrfs.wiki.kernel.org before using

SMALL VOLUME: forcing mixed metadata/data groups
mkfs.btrfs: volumes.c:857: btrfs_alloc_chunk: Assertion `!(ret)''
failed.
Aborted (core dumped)

which was at:

                ret = btrfs_alloc_dev_extent(trans, device,
                             info->chunk_root->root_key.objectid,
                             BTRFS_FIRST_CHUNK_TREE_OBJECTID, key.offset,
                             calc_size, &dev_offset);
                BUG_ON(ret);

:(

Also, I''m curious - I know the code existed before your patch 2/2,
but I don''t understand why it reserves 1MB for the first superblock 
when the first superblock is actually at 64k.  Any idea?

-Eric

> Hidetoshi Seto (2):
>       btrfs-progs: treat reserved 1MB for superblock properly
>       btrfs-progs: exit if there is not enough free space for mkfs
> 
>  ctree.h   |    3 +++
>  mkfs.c    |    4 ++++
>  volumes.c |    7 ++++++-
>  3 files changed, 13 insertions(+), 1 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(2013/08/26 23:23), Eric Sandeen wrote:
> Thanks for looking into this - how small of a device did you test?
> 
> I tried a 2MB device w/ these 2 patches and still got:
> 
> [btrfs-progs]# truncate --size=2m testfile
> [btrfs-progs]# ./mkfs.btrfs testfile
> 
> WARNING! - Btrfs v0.20-rc1-360-geeeb4e9 IS EXPERIMENTAL
> WARNING! - see http://btrfs.wiki.kernel.org before using
> 
> SMALL VOLUME: forcing mixed metadata/data groups
> mkfs.btrfs: volumes.c:857: btrfs_alloc_chunk: Assertion `!(ret)''
failed.
> Aborted (core dumped)
> 
> which was at:
> 
>                 ret = btrfs_alloc_dev_extent(trans, device,
>                              info->chunk_root->root_key.objectid,
>                              BTRFS_FIRST_CHUNK_TREE_OBJECTID, key.offset,
>                              calc_size, &dev_offset);
>                 BUG_ON(ret);
> 
> :(
Wow...
It seems that this abort is different problem from the bug which
my patches are going to fix.  I''ll try to make new patch to fix this
problem.
> 
> Also, I''m curious - I know the code existed before your patch 2/2,
> but I don''t understand why it reserves 1MB for the first
superblock
> when the first superblock is actually at 64k.  Any idea?
> 
> -Eric
I''m not sure... According to the git-log, this 1M trick is in 
the following old commit by Chris:

  commit a6de0bd778475504f42a142c83b8077993cbddfe
  Author: Chris Mason <chris.mason@oracle.com>
  Date:   Thu Apr 3 16:35:48 2008 -0400

     Add mirroring support across multiple drives


Thanks,
H.Seto

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 8/28/13 12:01 AM, Hidetoshi Seto wrote:
> (2013/08/26 23:23), Eric Sandeen wrote:
>> Thanks for looking into this - how small of a device did you test?
>>
>> I tried a 2MB device w/ these 2 patches and still got:
>>
>> [btrfs-progs]# truncate --size=2m testfile
>> [btrfs-progs]# ./mkfs.btrfs testfile
>>
>> WARNING! - Btrfs v0.20-rc1-360-geeeb4e9 IS EXPERIMENTAL
>> WARNING! - see http://btrfs.wiki.kernel.org before using
>>
>> SMALL VOLUME: forcing mixed metadata/data groups
>> mkfs.btrfs: volumes.c:857: btrfs_alloc_chunk: Assertion
`!(ret)'' failed.
>> Aborted (core dumped)
>>
>> which was at:
>>
>>                 ret = btrfs_alloc_dev_extent(trans, device,
>>                              info->chunk_root->root_key.objectid,
>>                              BTRFS_FIRST_CHUNK_TREE_OBJECTID,
key.offset,
>>                              calc_size, &dev_offset);
>>                 BUG_ON(ret);
>>
>> :(
> 
> Wow...
> It seems that this abort is different problem from the bug which
> my patches are going to fix.  I''ll try to make new patch to fix
this
> problem.
> 
>>
>> Also, I''m curious - I know the code existed before your patch
2/2,
>> but I don''t understand why it reserves 1MB for the first
superblock
>> when the first superblock is actually at 64k.  Any idea?
>>
>> -Eric
> 
> I''m not sure... According to the git-log, this 1M trick is in 
> the following old commit by Chris:
> 
>   commit a6de0bd778475504f42a142c83b8077993cbddfe
>   Author: Chris Mason <chris.mason@oracle.com>
>   Date:   Thu Apr 3 16:35:48 2008 -0400
> 
>      Add mirroring support across multiple drives
Yep I saw that too.  Seemingly unrelated.  :(  Unless I''m missing
something (which I probably am).

-Eric
> 
> Thanks,
> H.Seto
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Aug 29, 2013 at 11:11:50AM -0500, Eric Sandeen
wrote:
> >> Also, I''m curious - I know the code existed before your
patch 2/2,
> >> but I don''t understand why it reserves 1MB for the first
superblock
> >> when the first superblock is actually at 64k.  Any idea?
> > 
> > I''m not sure... According to the git-log, this 1M trick is in
> > the following old commit by Chris:
> > 
> >   commit a6de0bd778475504f42a142c83b8077993cbddfe
> >   Author: Chris Mason <chris.mason@oracle.com>
> >   Date:   Thu Apr 3 16:35:48 2008 -0400
> > 
> >      Add mirroring support across multiple drives
> 
> Yep I saw that too.  Seemingly unrelated.  :(  Unless I''m missing
> something (which I probably am).
IIRC the 1 MB of unused space (minus the first superblock) is there to
avoid random overwrites from paritioners or somesuch. If the whole
megabyte is overwritten including the superblock, the 1st copy lives at
64 MB which exists on practically every fs and the one at 64k can be
retored from it.

david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html