| Message ID | 1537849462-619-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix _scratch_mkfs_sized() for btrfs | expand |
On 25.09.2018 07:24, Anand Jain wrote: > As of now _scratch_mkfs_sized() checks if the requested size is below 1G > and forces the --mixed option for the mkfs.btrfs. Well the correct size > considering all possible group profiles at which we need to force the > mixed option is roughly 256Mbytes. So fix that. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Have you considered the implications of this w.r.t commit d4da414a9a9d ("common/rc: raise btrfs mixed mode threshold to 1GB") Initially this threshold was 100mb then Omar changed it to 1g. Does this change affect generic/427? > --- > common/rc | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/common/rc b/common/rc > index d5bb1feee2c3..90dc3002bc3d 100644 > --- a/common/rc > +++ b/common/rc > @@ -969,7 +969,10 @@ _scratch_mkfs_sized() > ;; > btrfs) > local mixed_opt= > - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' > + # minimum size that's needed without the mixed option. > + # Ref: btrfs-prog: btrfs_min_dev_size() > + # Non mixed mode is also the default option. > + (( fssize < $((256 * 1024 *1024)) )) && mixed_opt='--mixed' > $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV > ;; > jfs) >
On 09/25/2018 06:51 PM, Nikolay Borisov wrote: > > > On 25.09.2018 07:24, Anand Jain wrote: >> As of now _scratch_mkfs_sized() checks if the requested size is below 1G >> and forces the --mixed option for the mkfs.btrfs. Well the correct size >> considering all possible group profiles at which we need to force the >> mixed option is roughly 256Mbytes. So fix that. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > Have you considered the implications of this w.r.t commit > d4da414a9a9d ("common/rc: raise btrfs mixed mode threshold to 1GB") > > Initially this threshold was 100mb then Omar changed it to 1g. Does this > change affect generic/427? d4da414a9a9d does not explain what was the problem that Omar wanted to address, mainly what was the failure about. And no it does not affect. I have verified generic/427 with kernel 4.1 and 4.19-rc5 with btrfs-progs 4.1, 4.9 and latest from kdave they all run fine. Good to integrate. Thanks, Anand > >> --- >> common/rc | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/common/rc b/common/rc >> index d5bb1feee2c3..90dc3002bc3d 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -969,7 +969,10 @@ _scratch_mkfs_sized() >> ;; >> btrfs) >> local mixed_opt= >> - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' >> + # minimum size that's needed without the mixed option. >> + # Ref: btrfs-prog: btrfs_min_dev_size() >> + # Non mixed mode is also the default option. >> + (( fssize < $((256 * 1024 *1024)) )) && mixed_opt='--mixed' >> $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV >> ;; >> jfs) >>
On 26.09.2018 07:07, Anand Jain wrote: > > > On 09/25/2018 06:51 PM, Nikolay Borisov wrote: >> >> >> On 25.09.2018 07:24, Anand Jain wrote: >>> As of now _scratch_mkfs_sized() checks if the requested size is below 1G >>> and forces the --mixed option for the mkfs.btrfs. Well the correct size >>> considering all possible group profiles at which we need to force the >>> mixed option is roughly 256Mbytes. So fix that. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> Have you considered the implications of this w.r.t commit >> d4da414a9a9d ("common/rc: raise btrfs mixed mode threshold to 1GB") >> >> Initially this threshold was 100mb then Omar changed it to 1g. Does this >> change affect generic/427? > > d4da414a9a9d does not explain what was the problem that Omar wanted to > address, mainly what was the failure about. I just retested on upstream 4.19.0-rc3 with Omar's patch reverted (so anything above 100m for fs size is created with non-mixed block groups) and the test succeeded. So indeed your change seems to not make a difference for this test. > > And no it does not affect. I have verified generic/427 with kernel 4.1 > and 4.19-rc5 with btrfs-progs 4.1, 4.9 and latest from kdave they all > run fine. Good to integrate. > > Thanks, Anand > >> >>> --- >>> common/rc | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/common/rc b/common/rc >>> index d5bb1feee2c3..90dc3002bc3d 100644 >>> --- a/common/rc >>> +++ b/common/rc >>> @@ -969,7 +969,10 @@ _scratch_mkfs_sized() >>> ;; >>> btrfs) >>> local mixed_opt= >>> - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' >>> + # minimum size that's needed without the mixed option. >>> + # Ref: btrfs-prog: btrfs_min_dev_size() >>> + # Non mixed mode is also the default option. >>> + (( fssize < $((256 * 1024 *1024)) )) && mixed_opt='--mixed' >>> $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV >>> ;; >>> jfs) >>> >
On 09/26/2018 02:34 PM, Nikolay Borisov wrote: > > > On 26.09.2018 07:07, Anand Jain wrote: >> >> >> On 09/25/2018 06:51 PM, Nikolay Borisov wrote: >>> >>> >>> On 25.09.2018 07:24, Anand Jain wrote: >>>> As of now _scratch_mkfs_sized() checks if the requested size is below 1G >>>> and forces the --mixed option for the mkfs.btrfs. Well the correct size >>>> considering all possible group profiles at which we need to force the >>>> mixed option is roughly 256Mbytes. So fix that. >>>> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> >>> Have you considered the implications of this w.r.t commit >>> d4da414a9a9d ("common/rc: raise btrfs mixed mode threshold to 1GB") >>> >>> Initially this threshold was 100mb then Omar changed it to 1g. Does this >>> change affect generic/427? >> >> d4da414a9a9d does not explain what was the problem that Omar wanted to >> address, mainly what was the failure about. > > I just retested on upstream 4.19.0-rc3 with Omar's patch reverted (so > anything above 100m for fs size is created with non-mixed block groups) > and the test succeeded. So indeed your change seems to not make a > difference for this test. Thanks for testing. Anand >> >> And no it does not affect. I have verified generic/427 with kernel 4.1 >> and 4.19-rc5 with btrfs-progs 4.1, 4.9 and latest from kdave they all >> run fine. Good to integrate. >> >> Thanks, Anand >> >>> >>>> --- >>>> common/rc | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index d5bb1feee2c3..90dc3002bc3d 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -969,7 +969,10 @@ _scratch_mkfs_sized() >>>> ;; >>>> btrfs) >>>> local mixed_opt= >>>> - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' >>>> + # minimum size that's needed without the mixed option. >>>> + # Ref: btrfs-prog: btrfs_min_dev_size() >>>> + # Non mixed mode is also the default option. >>>> + (( fssize < $((256 * 1024 *1024)) )) && mixed_opt='--mixed' >>>> $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV >>>> ;; >>>> jfs) >>>> >>
On Wed, Sep 26, 2018 at 09:34:27AM +0300, Nikolay Borisov wrote: > > > On 26.09.2018 07:07, Anand Jain wrote: > > > > > > On 09/25/2018 06:51 PM, Nikolay Borisov wrote: > >> > >> > >> On 25.09.2018 07:24, Anand Jain wrote: > >>> As of now _scratch_mkfs_sized() checks if the requested size is below 1G > >>> and forces the --mixed option for the mkfs.btrfs. Well the correct size > >>> considering all possible group profiles at which we need to force the > >>> mixed option is roughly 256Mbytes. So fix that. > >>> > >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> > >> > >> Have you considered the implications of this w.r.t commit > >> d4da414a9a9d ("common/rc: raise btrfs mixed mode threshold to 1GB") > >> > >> Initially this threshold was 100mb then Omar changed it to 1g. Does this > >> change affect generic/427? > > > > d4da414a9a9d does not explain what was the problem that Omar wanted to > > address, mainly what was the failure about. > > I just retested on upstream 4.19.0-rc3 with Omar's patch reverted (so > anything above 100m for fs size is created with non-mixed block groups) > and the test succeeded. So indeed your change seems to not make a > difference for this test. > > > > > And no it does not affect. I have verified generic/427 with kernel 4.1 > > and 4.19-rc5 with btrfs-progs 4.1, 4.9 and latest from kdave they all > > run fine. Good to integrate. I had to double check, but it only happens with -m dup. If I apply the following patch: diff --git a/common/rc b/common/rc index d5bb1fe..989b846 100644 --- a/common/rc +++ b/common/rc @@ -969,7 +969,7 @@ _scratch_mkfs_sized() ;; btrfs) local mixed_opt= - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' + (( fssize <= 100 * 1024 * 1024 )) && mixed_opt='--mixed' $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV ;; jfs) diff --git a/tests/generic/427 b/tests/generic/427 index e8ebffe..206cf08 100755 --- a/tests/generic/427 +++ b/tests/generic/427 @@ -65,6 +65,7 @@ fi # start a background aio writer, which does several extending loops # internally and check data integrality $AIO_TEST -s $fsize -b 65536 $SCRATCH_MNT/tst-aio-dio-eof-race.$seq +btrfs fi usage $SCRATCH_MNT status=$? kill $open_close_pid And run with MKFS_OPTIONS="-m dup", then we don't have enough data space for the test: --- /root/linux/xfstests/tests/generic/427.out 2017-11-28 16:05:46.811435644 -0800 +++ /root/linux/xfstests/results/generic/427.out.bad 2018-09-27 13:01:00.540510385 -0700 @@ -1,2 +1,24 @@ QA output created by 427 -Success, all done. +pwrite: No space left on device +Overall: + Device size: 256.00MiB + Device allocated: 255.00MiB + Device unallocated: 1.00MiB + Device missing: 0.00B + Used: 179.03MiB + Free (estimated): 0.00B (min: 0.00B) + Data ratio: 1.00 + Metadata ratio: 2.00 + Global reserve: 16.00MiB (used: 0.00B) + +Data,single: Size:175.00MiB, Used:175.00MiB + /dev/nvme0n1p2 175.00MiB + +Metadata,DUP: Size:32.00MiB, Used:2.00MiB + /dev/nvme0n1p2 64.00MiB + +System,DUP: Size:8.00MiB, Used:16.00KiB + /dev/nvme0n1p2 16.00MiB + +Unallocated: + /dev/nvme0n1p2 1.00MiB
On 09/28/2018 04:07 AM, Omar Sandoval wrote: > On Wed, Sep 26, 2018 at 09:34:27AM +0300, Nikolay Borisov wrote: >> >> >> On 26.09.2018 07:07, Anand Jain wrote: >>> >>> >>> On 09/25/2018 06:51 PM, Nikolay Borisov wrote: >>>> >>>> >>>> On 25.09.2018 07:24, Anand Jain wrote: >>>>> As of now _scratch_mkfs_sized() checks if the requested size is below 1G >>>>> and forces the --mixed option for the mkfs.btrfs. Well the correct size >>>>> considering all possible group profiles at which we need to force the >>>>> mixed option is roughly 256Mbytes. So fix that. >>>>> >>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>>> >>>> Have you considered the implications of this w.r.t commit >>>> d4da414a9a9d ("common/rc: raise btrfs mixed mode threshold to 1GB") >>>> >>>> Initially this threshold was 100mb then Omar changed it to 1g. Does this >>>> change affect generic/427? >>> >>> d4da414a9a9d does not explain what was the problem that Omar wanted to >>> address, mainly what was the failure about. >> >> I just retested on upstream 4.19.0-rc3 with Omar's patch reverted (so >> anything above 100m for fs size is created with non-mixed block groups) >> and the test succeeded. So indeed your change seems to not make a >> difference for this test. >> >>> >>> And no it does not affect. I have verified generic/427 with kernel 4.1 >>> and 4.19-rc5 with btrfs-progs 4.1, 4.9 and latest from kdave they all >>> run fine. Good to integrate. > > I had to double check, but it only happens with -m dup. If I apply the > following patch: > > diff --git a/common/rc b/common/rc > index d5bb1fe..989b846 100644 > --- a/common/rc > +++ b/common/rc > @@ -969,7 +969,7 @@ _scratch_mkfs_sized() > ;; > btrfs) > local mixed_opt= > - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' > + (( fssize <= 100 * 1024 * 1024 )) && mixed_opt='--mixed' > $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV > ;; > jfs) > diff --git a/tests/generic/427 b/tests/generic/427 > index e8ebffe..206cf08 100755 > --- a/tests/generic/427 > +++ b/tests/generic/427 > @@ -65,6 +65,7 @@ fi > # start a background aio writer, which does several extending loops > # internally and check data integrality > $AIO_TEST -s $fsize -b 65536 $SCRATCH_MNT/tst-aio-dio-eof-race.$seq > +btrfs fi usage $SCRATCH_MNT > status=$? > > kill $open_close_pid > > And run with MKFS_OPTIONS="-m dup", then we don't have enough data space > for the test: > > --- /root/linux/xfstests/tests/generic/427.out 2017-11-28 16:05:46.811435644 -0800 > +++ /root/linux/xfstests/results/generic/427.out.bad 2018-09-27 13:01:00.540510385 -0700 > @@ -1,2 +1,24 @@ > QA output created by 427 > -Success, all done. > +pwrite: No space left on device Thanks Omar. Unfortunately I can't reproduce with the diff as above + MKFS_OPTIONS="-m dup". In any case the objective of this patch is to ensure _scratch_mkfs_sized() provides default group profile with the minimum disk size that's actually be required. And related to that there isn't any issue in this patch. Thanks, Anand > +Overall: > + Device size: 256.00MiB > + Device allocated: 255.00MiB > + Device unallocated: 1.00MiB > + Device missing: 0.00B > + Used: 179.03MiB > + Free (estimated): 0.00B (min: 0.00B) > + Data ratio: 1.00 > + Metadata ratio: 2.00 > + Global reserve: 16.00MiB (used: 0.00B) > + > +Data,single: Size:175.00MiB, Used:175.00MiB > + /dev/nvme0n1p2 175.00MiB > + > +Metadata,DUP: Size:32.00MiB, Used:2.00MiB > + /dev/nvme0n1p2 64.00MiB > + > +System,DUP: Size:8.00MiB, Used:16.00KiB > + /dev/nvme0n1p2 16.00MiB > + > +Unallocated: > + /dev/nvme0n1p2 1.00MiB >
diff --git a/common/rc b/common/rc index d5bb1feee2c3..90dc3002bc3d 100644 --- a/common/rc +++ b/common/rc @@ -969,7 +969,10 @@ _scratch_mkfs_sized() ;; btrfs) local mixed_opt= - (( fssize <= 1024 * 1024 * 1024 )) && mixed_opt='--mixed' + # minimum size that's needed without the mixed option. + # Ref: btrfs-prog: btrfs_min_dev_size() + # Non mixed mode is also the default option. + (( fssize < $((256 * 1024 *1024)) )) && mixed_opt='--mixed' $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV ;; jfs)
As of now _scratch_mkfs_sized() checks if the requested size is below 1G and forces the --mixed option for the mkfs.btrfs. Well the correct size considering all possible group profiles at which we need to force the mixed option is roughly 256Mbytes. So fix that. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- common/rc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)