| Message ID | 20180131055615.14454-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 31.01.2018 07:56, Qu Wenruo wrote: > When checking the minimal nr_devs, there is one dead and meaningless > condition: > > if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > This condition is meaningless, @devs_increment has nothing to do with > @sub_stripes. > > In fact, in btrfs_raid_array[], profile with sub_stripes larger than 1 > (RAID10) already has the @devs_increment set to 2. > So no need to multiple it by @sub_stripes. > > And above condition is also dead. > For RAID10, @devs_increment * @sub_stripes equals 4, which is also the > @devs_min of RAID10. > For other profiles, @sub_stripes is always 1, and since @ndevs is > rounded down to @devs_increment, the condition will always be true. > > Remove the meaningless condition to make later reader wander less. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> Quick question : What exactly is a substripe? Stripe is essentially how many contiguous portions of disk are necessary to satisfy the profile, right? So for raid1 we write 1 copy of the data per device (hence dev_stripes = 1). For DUP we have 2 copies of the data on the same disk hence dev_stripes 2. How does sub_stripes fit in the grand scheme of things? > --- > fs/btrfs/volumes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 215e85e22c8e..cb0a8d27661b 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > /* round down to number of usable stripes */ > ndevs = round_down(ndevs, devs_increment); > > - if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { > + if (ndevs < devs_min) { > ret = -ENOSPC; > goto error; > } > -- 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 2018年01月31日 15:35, Nikolay Borisov wrote: > > > On 31.01.2018 07:56, Qu Wenruo wrote: >> When checking the minimal nr_devs, there is one dead and meaningless >> condition: >> >> if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> This condition is meaningless, @devs_increment has nothing to do with >> @sub_stripes. >> >> In fact, in btrfs_raid_array[], profile with sub_stripes larger than 1 >> (RAID10) already has the @devs_increment set to 2. >> So no need to multiple it by @sub_stripes. >> >> And above condition is also dead. >> For RAID10, @devs_increment * @sub_stripes equals 4, which is also the >> @devs_min of RAID10. >> For other profiles, @sub_stripes is always 1, and since @ndevs is >> rounded down to @devs_increment, the condition will always be true. >> >> Remove the meaningless condition to make later reader wander less. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > Quick question : What exactly is a substripe? IMHO it is the number of copies inside the RAID0 virtual stripe. For current RAID10, it's 2 devices as a bundle, using RAID1, then RAID0 these bundles. > Stripe is essentially how > many contiguous portions of disk are necessary to satisfy the profile, > right? So for raid1 we write 1 copy of the data per device (hence > dev_stripes = 1). For DUP we have 2 copies of the data on the same disk > hence dev_stripes 2. How does sub_stripes fit in the grand scheme of things? Here we have extra number to describe the behavior. Mostly btrfs_raid_arrary. For sub_stripes, it should be acts like: Logical address space: 0 1G | RAID10 chunk | | RAID0 of virtual stripe 1~3 | / | \ / | \ | Virtual stripe 1|| Virtual stripe 2 || Vritual bundle 3 | |RAID1 of physical stripe 1~2 | / \ / \ |Physical stripe 1| |Physical stripe 2| And sub_stripes describes how many physical stripes are inside the virtual stripe. Thanks, Qu >> --- >> fs/btrfs/volumes.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 215e85e22c8e..cb0a8d27661b 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, >> /* round down to number of usable stripes */ >> ndevs = round_down(ndevs, devs_increment); >> >> - if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { >> + if (ndevs < devs_min) { >> ret = -ENOSPC; >> goto error; >> } >> > -- > 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 Wed, Jan 31, 2018 at 01:56:15PM +0800, Qu Wenruo wrote: > When checking the minimal nr_devs, there is one dead and meaningless > condition: > > if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > This condition is meaningless, @devs_increment has nothing to do with > @sub_stripes. > > In fact, in btrfs_raid_array[], profile with sub_stripes larger than 1 > (RAID10) already has the @devs_increment set to 2. > So no need to multiple it by @sub_stripes. > > And above condition is also dead. > For RAID10, @devs_increment * @sub_stripes equals 4, which is also the > @devs_min of RAID10. > For other profiles, @sub_stripes is always 1, and since @ndevs is > rounded down to @devs_increment, the condition will always be true. > > Remove the meaningless condition to make later reader wander less. I think the condition is a leftover from times when we did not have the nice raid table and the various values were defined in the function. Reviewed-by: David Sterba <dsterba@suse.com> > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/volumes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 215e85e22c8e..cb0a8d27661b 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > /* round down to number of usable stripes */ > ndevs = round_down(ndevs, devs_increment); > > - if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { > + if (ndevs < devs_min) { The redundant condtion is duplicated in the error message a few lines below: 4840 if (ndevs < devs_min) { 4841 ret = -ENOSPC; 4842 if (btrfs_test_opt(info, ENOSPC_DEBUG)) { 4843 btrfs_debug(info, 4844 "%s: not enough devices with free space: have=%d minimum required=%d", 4845 __func__, ndevs, min(devs_min, 4846 devs_increment * sub_stripes)); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ so I'll remove it as well. 4847 } 4848 goto error; 4849 } > ret = -ENOSPC; > goto error; > } -- 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
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 215e85e22c8e..cb0a8d27661b 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, /* round down to number of usable stripes */ ndevs = round_down(ndevs, devs_increment); - if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { + if (ndevs < devs_min) { ret = -ENOSPC; goto error; }
When checking the minimal nr_devs, there is one dead and meaningless condition: if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This condition is meaningless, @devs_increment has nothing to do with @sub_stripes. In fact, in btrfs_raid_array[], profile with sub_stripes larger than 1 (RAID10) already has the @devs_increment set to 2. So no need to multiple it by @sub_stripes. And above condition is also dead. For RAID10, @devs_increment * @sub_stripes equals 4, which is also the @devs_min of RAID10. For other profiles, @sub_stripes is always 1, and since @ndevs is rounded down to @devs_increment, the condition will always be true. Remove the meaningless condition to make later reader wander less. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)