On 10/10/2017 07:07 PM, Hans van Kranenburg wrote: > On 10/10/2017 01:31 PM, David Sterba wrote: >> Hi, >> >> On Fri, Sep 29, 2017 at 04:20:51PM +0900, Naohiro Aota wrote: >>> Balancing a fresh METADATA=dup btrfs file system (with size < 50G) >>> generates a 128MB sized block group. While we set max_stripe_size = >>> max_chunk_size = 256MB, we get this half sized block group: >>> >>> $ btrfs ins dump-t -t CHUNK_TREE btrfs.img|grep length >>> length 8388608 owner 2 stripe_len 65536 type DATA >>> length 33554432 owner 2 stripe_len 65536 type SYSTEM|DUP >>> length 134217728 owner 2 stripe_len 65536 type METADATA|DUP >>> >>> Before commit 86db25785a6e ("Btrfs: fix max chunk size on raid5/6"), we >>> used "stripe_size * ndevs > max_chunk_size * ncopies" to check the max >>> chunk size. Since stripe_size = 256MB * dev_stripes (= 2) = 512MB, ndevs >>> = 1, max_chunk_size = 256MB, and ncopies = 2, we allowed 256MB >>> METADATA|DUP block group. >>> >>> But now, we use "stripe_size * data_stripes > max_chunk_size". Since >>> data_stripes = 1 on DUP, it disallows the block group to have > 128MB. >>> What missing here is "dev_stripes". Proper logical space used by the block >>> group is "stripe_size * data_stripes / dev_stripes". Tweak the equations to >>> use the right value. >> >> I started looking into it and still don't fully understand it. Change >> deep in the allocator can easily break some blockgroup combinations, so >> I'm rather conservative here. > > I think that the added usage of data_stripes in 86db25785a6e is the > problematic change. data_stripes is something that was introduced as > part of RAID56 in 53b381b3a and clearly only has a meaning that's > properly thought of for RAID56. The RAID56 commit already adds "this > will have to be fixed for RAID1 and RAID10 over more drives", only the > author doesn't catch the DUP case, which already breaks at that point. > > At the beginning it says: > > int data_stripes; /* number of stripes that count for block group size */ > > For the example: > > This is DUP: > > .sub_stripes = 1, > .dev_stripes = 2, > .devs_max = 1, > .devs_min = 1, > .tolerated_failures = 0, > .devs_increment = 1, > .ncopies = 2, > > In the code: > > max_stripe_size = SZ_256M > max_chunk_size = max_stripe_size -> SZ_256M > > Then we have find_free_dev_extent: > max_stripe_size * dev_stripes -> SZ_256M * 2 -> 512M > > So we like to find 512M on a disk, to stuff 2 stripes of 256M inside for > the DUP. (remember: The two parts of DUP *never* end up on a different > disk, even if you have multiple) > > If we find one: > stripe_size = devices_info[ndevs-1].max_avail -> 512M, yay > > ndevs = min(ndevs, devs_max) -> min($whatever, 1) -> 1 > num_stripes = ndevs * dev_stripes -> 1 * 2 -> 2 > > data_stripes = num_stripes / ncopies = 2 / 2 = 1
Oh, wow, this is not true of course, because the "number of stripes that count for block group size" should still be 1 for DUP... > BOOM! There's the problem. The data_stripes only thinks about data that > is horizontally spread over disks, and not vertically spread... Hm... no Hans, you're wrong. > What I would propose is changing... > the data_stripes = <blah> and afterwards trying to correct it with > some ifs > ...to... > a switch/case thing where the explicit logic is put to get the right > value for each specific raid type. > > In case of DUP this simply means data_stripes = 2, because there is no > need for fancy calculations about spreading DUP data over X devices. > It's always 2 copies on 1 device. Eh, nope. 1. So, then I end up at the "stripe_size * data_stripes > max_chunk_size" again. So, yes, Naohiro is right, and DUP is the only case in which this logic breaks. DUP is the only one in which it this change makes a difference, because it's the only one which has dev_stripes set to something else than 1. \o/ > ... > > My general feeling when looking at the code, is that this single part of > code is responsible for too many different cases, or, more possible > cases than a developer can reason about at once "in his head" when > working on it. > > 7 raid options * 3 different types (data, metadata, system) = 21 > already... Some parts of the algorithm make only sense for a subset of > the combinations, but they're still part of the computation, which > sometimes "by accident" results in the correct outcome. :) > > If it can't be done in a way that's easier to understand when reading > the code, it should have unit tests with a list of known input/output to > detect unwanted changes. > -- Hans van Kranenburg -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html