| Message ID | 20220427143409.987-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph/005: verify correct statfs behaviour with quotas | expand |
Hi Luís, It looks like this one is still in need of review... On Wed, 27 Apr 2022 15:34:09 +0100, Luís Henriques wrote: > When using a directory with 'max_bytes' quota as a base for a mount, > statfs shall use that 'max_bytes' value as the total disk size. That > value shall be used even when using subdirectory as base for the mount. > > A bug was found where, when this subdirectory also had a 'max_files' > quota, the real filesystem size would be returned instead of the parent > 'max_bytes' quota value. This test case verifies this bug is fixed. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ > tests/ceph/005.out | 2 ++ > 2 files changed, 42 insertions(+) > create mode 100755 tests/ceph/005 > create mode 100644 tests/ceph/005.out > > diff --git a/tests/ceph/005 b/tests/ceph/005 > new file mode 100755 > index 000000000000..0763a235a677 > --- /dev/null > +++ b/tests/ceph/005 > @@ -0,0 +1,40 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 005 > +# > +# Make sure statfs reports correct total size when: > +# 1. using a directory with 'max_byte' quota as base for a mount > +# 2. using a subdirectory of the above directory with 'max_files' quota > +# > +. ./common/preamble > +_begin_fstest auto quick quota > + > +_supported_fs generic > +_require_scratch > + > +_scratch_mount > +mkdir -p $SCRATCH_MNT/quota-dir/subdir > + > +# set quotas > +quota=$((1024*10000)) > +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir > +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir > +_scratch_unmount > + > +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount Aside from the standard please-quote-your-variables gripe, I'm a little confused with the use of SCRATCH_DEV for this test. Network FSes where mkfs isn't provided don't generally use it. Is there any way that this could be run against TEST_DEV, or does the umount / mount complicate things too much? Cheers, David
David Disseldorp <ddiss@suse.de> writes: > Hi Luís, > > It looks like this one is still in need of review... Ah! Thanks for reminding me about it, David! > > On Wed, 27 Apr 2022 15:34:09 +0100, Luís Henriques wrote: > >> When using a directory with 'max_bytes' quota as a base for a mount, >> statfs shall use that 'max_bytes' value as the total disk size. That >> value shall be used even when using subdirectory as base for the mount. >> >> A bug was found where, when this subdirectory also had a 'max_files' >> quota, the real filesystem size would be returned instead of the parent >> 'max_bytes' quota value. This test case verifies this bug is fixed. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ >> tests/ceph/005.out | 2 ++ >> 2 files changed, 42 insertions(+) >> create mode 100755 tests/ceph/005 >> create mode 100644 tests/ceph/005.out >> >> diff --git a/tests/ceph/005 b/tests/ceph/005 >> new file mode 100755 >> index 000000000000..0763a235a677 >> --- /dev/null >> +++ b/tests/ceph/005 >> @@ -0,0 +1,40 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. >> +# >> +# FS QA Test 005 >> +# >> +# Make sure statfs reports correct total size when: >> +# 1. using a directory with 'max_byte' quota as base for a mount >> +# 2. using a subdirectory of the above directory with 'max_files' quota >> +# >> +. ./common/preamble >> +_begin_fstest auto quick quota >> + >> +_supported_fs generic >> +_require_scratch >> + >> +_scratch_mount >> +mkdir -p $SCRATCH_MNT/quota-dir/subdir >> + >> +# set quotas >> +quota=$((1024*10000)) >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir >> +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir >> +_scratch_unmount >> + >> +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount > > Aside from the standard please-quote-your-variables gripe, I'm a little Sure, I'll fix those in next iteration. > confused with the use of SCRATCH_DEV for this test. Network FSes where > mkfs isn't provided don't generally use it. Is there any way that this > could be run against TEST_DEV, or does the umount / mount complicate > things too much? When I looked at other tests doing similar things (i.e. changing the mount device during the test), they all seemed to be using SCRATCH_DEV. I guess that I could change TEST_DEV instead. I'll revisit this and see if that works. Anyway, regarding the usage of SCRATCH_DEV in cephfs, I've used several different approaches: - Use 2 different filesystems created on the same cluster, - Use 2 volumes on the same filesystem, or - Simply use 2 directories in the same filesystem. I tend to use the later most of the times, as it's easier to setup :-) Cheers,
On Wed, Apr 27, 2022 at 03:34:09PM +0100, Luís Henriques wrote: > When using a directory with 'max_bytes' quota as a base for a mount, > statfs shall use that 'max_bytes' value as the total disk size. That > value shall be used even when using subdirectory as base for the mount. > > A bug was found where, when this subdirectory also had a 'max_files' > quota, the real filesystem size would be returned instead of the parent > 'max_bytes' quota value. This test case verifies this bug is fixed. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ > tests/ceph/005.out | 2 ++ > 2 files changed, 42 insertions(+) > create mode 100755 tests/ceph/005 > create mode 100644 tests/ceph/005.out > > diff --git a/tests/ceph/005 b/tests/ceph/005 > new file mode 100755 > index 000000000000..0763a235a677 > --- /dev/null > +++ b/tests/ceph/005 > @@ -0,0 +1,40 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 005 > +# > +# Make sure statfs reports correct total size when: > +# 1. using a directory with 'max_byte' quota as base for a mount > +# 2. using a subdirectory of the above directory with 'max_files' quota > +# > +. ./common/preamble > +_begin_fstest auto quick quota > + > +_supported_fs generic As this case name is ceph/005, so I suppose you'd like to support 'ceph' only. > +_require_scratch > + > +_scratch_mount > +mkdir -p $SCRATCH_MNT/quota-dir/subdir > + > +# set quotas > +quota=$((1024*10000)) > +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir > +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir > +_scratch_unmount > + > +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount Try to not use SCRATCH_DEV like this. > +total=`df -kP $SCRATCH_MNT | grep -v Filesystem | awk '{print $2}'` ^^ $DF_PROG As we have _get_total_inode(), _get_used_inode(), _get_used_inode_percent(), _get_free_inode() and _get_available_space() in common/rc, I don't mind add one more: _get_total_space() { if [ -z "$1" ]; then echo "Usage: _get_total_space <mnt>" exit 1 fi local total_kb; total_kb=`$DF_PROG $1 | tail -n1 | awk '{ print $2 }'` echo $((total_kb * 1024)) } > +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_unmount > +[ $total -eq 8192 ] || _fail "Incorrect statfs for quota-dir: $total" ^^^^ I'm not familar with ceph, I just found "quota=$((1024*10000))" in this case, didn't find any place metioned 8192. So may you help to demystify why we expect "8192" at here? And if "8192" is a fixed expected number at here, then we can print it directly, as golden image, see below ... > + > +SCRATCH_DEV=$SCRATCH_DEV/quota-dir/subdir _scratch_mount > +total=`df -kP $SCRATCH_MNT | grep -v Filesystem | awk '{print $2}'` > +SCRATCH_DEV=$SCRATCH_DEV/quota-dir/subdir _scratch_unmount > +[ $total -eq 8192 ] || _fail "Incorrect statfs for quota-dir/subdir: $total" May below code helps? _require_test localdir=$TEST_DIR/ceph-quota-dir-$seq rm -rf $localdir mkdir -p ${localdir}/subdir ... $SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $localdir $SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $localdir/subdir ... SCRATCH_DEV=$localdir _scratch_mount echo ceph quota size is $(_get_total_space $SCRATCH_MNT) SCRATCH_DEV=$localdir _scratch_unmount SCRATCH_DEV=$localdir/subdir _scratch_mount echo subdir ceph quota size is $(_get_total_space $SCRATCH_MNT) SCRATCH_DEV=$localdir/subdir _scratch_unmount Thanks, Zorro > + > +echo "Silence is golden" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/ceph/005.out b/tests/ceph/005.out > new file mode 100644 > index 000000000000..a5027f127cf0 > --- /dev/null > +++ b/tests/ceph/005.out > @@ -0,0 +1,2 @@ > +QA output created by 005 > +Silence is golden >
On Wed, 25 May 2022 09:53:53 +0100, Luís Henriques wrote: > David Disseldorp <ddiss@suse.de> writes: > > > Hi Luís, > > > > It looks like this one is still in need of review... > > Ah! Thanks for reminding me about it, David! > > > > > On Wed, 27 Apr 2022 15:34:09 +0100, Luís Henriques wrote: > > > >> When using a directory with 'max_bytes' quota as a base for a mount, > >> statfs shall use that 'max_bytes' value as the total disk size. That > >> value shall be used even when using subdirectory as base for the mount. > >> > >> A bug was found where, when this subdirectory also had a 'max_files' > >> quota, the real filesystem size would be returned instead of the parent > >> 'max_bytes' quota value. This test case verifies this bug is fixed. > >> > >> Signed-off-by: Luís Henriques <lhenriques@suse.de> > >> --- > >> tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ > >> tests/ceph/005.out | 2 ++ > >> 2 files changed, 42 insertions(+) > >> create mode 100755 tests/ceph/005 > >> create mode 100644 tests/ceph/005.out > >> > >> diff --git a/tests/ceph/005 b/tests/ceph/005 > >> new file mode 100755 > >> index 000000000000..0763a235a677 > >> --- /dev/null > >> +++ b/tests/ceph/005 > >> @@ -0,0 +1,40 @@ > >> +#! /bin/bash > >> +# SPDX-License-Identifier: GPL-2.0 > >> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. > >> +# > >> +# FS QA Test 005 > >> +# > >> +# Make sure statfs reports correct total size when: > >> +# 1. using a directory with 'max_byte' quota as base for a mount > >> +# 2. using a subdirectory of the above directory with 'max_files' quota > >> +# > >> +. ./common/preamble > >> +_begin_fstest auto quick quota > >> + > >> +_supported_fs generic > >> +_require_scratch > >> + > >> +_scratch_mount > >> +mkdir -p $SCRATCH_MNT/quota-dir/subdir > >> + > >> +# set quotas > >> +quota=$((1024*10000)) > >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir > >> +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir > >> +_scratch_unmount > >> + > >> +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount > > > > Aside from the standard please-quote-your-variables gripe, I'm a little > > Sure, I'll fix those in next iteration. > > > confused with the use of SCRATCH_DEV for this test. Network FSes where > > mkfs isn't provided don't generally use it. Is there any way that this > > could be run against TEST_DEV, or does the umount / mount complicate > > things too much? > > When I looked at other tests doing similar things (i.e. changing the mount > device during the test), they all seemed to be using SCRATCH_DEV. I guess > that I could change TEST_DEV instead. I'll revisit this and see if that > works. > > Anyway, regarding the usage of SCRATCH_DEV in cephfs, I've used several > different approaches: > > - Use 2 different filesystems created on the same cluster, > - Use 2 volumes on the same filesystem, or > - Simply use 2 directories in the same filesystem. Looking at _scratch_mkfs($FSTYP=ceph) there is support for scratch filesystem reinitialization, so I suppose this should be okay. With cephfs we could actually go one step further and call "ceph fs rm/new", but that's something for another day :-). Cheers, David
David Disseldorp <ddiss@suse.de> writes: > On Wed, 25 May 2022 09:53:53 +0100, Luís Henriques wrote: > >> David Disseldorp <ddiss@suse.de> writes: >> >> > Hi Luís, >> > >> > It looks like this one is still in need of review... >> >> Ah! Thanks for reminding me about it, David! >> >> > >> > On Wed, 27 Apr 2022 15:34:09 +0100, Luís Henriques wrote: >> > >> >> When using a directory with 'max_bytes' quota as a base for a mount, >> >> statfs shall use that 'max_bytes' value as the total disk size. That >> >> value shall be used even when using subdirectory as base for the mount. >> >> >> >> A bug was found where, when this subdirectory also had a 'max_files' >> >> quota, the real filesystem size would be returned instead of the parent >> >> 'max_bytes' quota value. This test case verifies this bug is fixed. >> >> >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> >> --- >> >> tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ >> >> tests/ceph/005.out | 2 ++ >> >> 2 files changed, 42 insertions(+) >> >> create mode 100755 tests/ceph/005 >> >> create mode 100644 tests/ceph/005.out >> >> >> >> diff --git a/tests/ceph/005 b/tests/ceph/005 >> >> new file mode 100755 >> >> index 000000000000..0763a235a677 >> >> --- /dev/null >> >> +++ b/tests/ceph/005 >> >> @@ -0,0 +1,40 @@ >> >> +#! /bin/bash >> >> +# SPDX-License-Identifier: GPL-2.0 >> >> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. >> >> +# >> >> +# FS QA Test 005 >> >> +# >> >> +# Make sure statfs reports correct total size when: >> >> +# 1. using a directory with 'max_byte' quota as base for a mount >> >> +# 2. using a subdirectory of the above directory with 'max_files' quota >> >> +# >> >> +. ./common/preamble >> >> +_begin_fstest auto quick quota >> >> + >> >> +_supported_fs generic >> >> +_require_scratch >> >> + >> >> +_scratch_mount >> >> +mkdir -p $SCRATCH_MNT/quota-dir/subdir >> >> + >> >> +# set quotas >> >> +quota=$((1024*10000)) >> >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir >> >> +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir >> >> +_scratch_unmount >> >> + >> >> +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount >> > >> > Aside from the standard please-quote-your-variables gripe, I'm a little >> >> Sure, I'll fix those in next iteration. >> >> > confused with the use of SCRATCH_DEV for this test. Network FSes where >> > mkfs isn't provided don't generally use it. Is there any way that this >> > could be run against TEST_DEV, or does the umount / mount complicate >> > things too much? >> >> When I looked at other tests doing similar things (i.e. changing the mount >> device during the test), they all seemed to be using SCRATCH_DEV. I guess >> that I could change TEST_DEV instead. I'll revisit this and see if that >> works. >> >> Anyway, regarding the usage of SCRATCH_DEV in cephfs, I've used several >> different approaches: >> >> - Use 2 different filesystems created on the same cluster, >> - Use 2 volumes on the same filesystem, or >> - Simply use 2 directories in the same filesystem. > > Looking at _scratch_mkfs($FSTYP=ceph) there is support for scratch > filesystem reinitialization, so I suppose this should be okay. With > cephfs we could actually go one step further and call "ceph fs rm/new", > but that's something for another day :-). Right, that would require a more complex test setup, with user-space tools on the test box/VM. It definitely would be desirable to have such an option for example in teuthology (the ceph testing framework), but for the simple runs I usually do on VMs, I'd rather keep the default as-is. Cheers,
Zorro Lang <zlang@redhat.com> writes: > On Wed, Apr 27, 2022 at 03:34:09PM +0100, Luís Henriques wrote: >> When using a directory with 'max_bytes' quota as a base for a mount, >> statfs shall use that 'max_bytes' value as the total disk size. That >> value shall be used even when using subdirectory as base for the mount. >> >> A bug was found where, when this subdirectory also had a 'max_files' >> quota, the real filesystem size would be returned instead of the parent >> 'max_bytes' quota value. This test case verifies this bug is fixed. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ >> tests/ceph/005.out | 2 ++ >> 2 files changed, 42 insertions(+) >> create mode 100755 tests/ceph/005 >> create mode 100644 tests/ceph/005.out >> >> diff --git a/tests/ceph/005 b/tests/ceph/005 >> new file mode 100755 >> index 000000000000..0763a235a677 >> --- /dev/null >> +++ b/tests/ceph/005 >> @@ -0,0 +1,40 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. >> +# >> +# FS QA Test 005 >> +# >> +# Make sure statfs reports correct total size when: >> +# 1. using a directory with 'max_byte' quota as base for a mount >> +# 2. using a subdirectory of the above directory with 'max_files' quota >> +# >> +. ./common/preamble >> +_begin_fstest auto quick quota >> + >> +_supported_fs generic > > As this case name is ceph/005, so I suppose you'd like to support 'ceph' only. Yep, my mistake, sorry. I'll fix it in next rev. >> +_require_scratch >> + >> +_scratch_mount >> +mkdir -p $SCRATCH_MNT/quota-dir/subdir >> + >> +# set quotas >> +quota=$((1024*10000)) >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir >> +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir >> +_scratch_unmount >> + >> +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount > > Try to not use SCRATCH_DEV like this. I used this because I saw other tests doing something similar. Basically, I need to remount a filesystem with a different base directory. Changing the SCRATCH_DEV looked like a simple solution. >> +total=`df -kP $SCRATCH_MNT | grep -v Filesystem | awk '{print $2}'` > ^^ $DF_PROG > > As we have _get_total_inode(), _get_used_inode(), _get_used_inode_percent(), > _get_free_inode() and _get_available_space() in common/rc, I don't mind add > one more: > > _get_total_space() > { > if [ -z "$1" ]; then > echo "Usage: _get_total_space <mnt>" > exit 1 > fi > local total_kb; > total_kb=`$DF_PROG $1 | tail -n1 | awk '{ print $2 }'` > echo $((total_kb * 1024)) > } Right, this makes sense. >> +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_unmount >> +[ $total -eq 8192 ] || _fail "Incorrect statfs for quota-dir: $total" > ^^^^ > I'm not familar with ceph, I just found "quota=$((1024*10000))" in this case, > didn't find any place metioned 8192. So may you help to demystify why we expect > "8192" at here? > > And if "8192" is a fixed expected number at here, then we can print it directly, > as golden image, see below ... Hmm... OK, I'm struggling to remember the details about this, and it was only a month ago I wrote this test! Which is a sign that I should have, at least, added a comment explaining this value. I'll need to dig into the statfs code again to explain why we're setting quotas to 10M and 'df' shows 8M (which is the default size for 2 ceph objects btw). I'll revisit this test in the next few days and sort this mystery. Thanks a lot for your review. Cheers
diff --git a/tests/ceph/005 b/tests/ceph/005 new file mode 100755 index 000000000000..0763a235a677 --- /dev/null +++ b/tests/ceph/005 @@ -0,0 +1,40 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved. +# +# FS QA Test 005 +# +# Make sure statfs reports correct total size when: +# 1. using a directory with 'max_byte' quota as base for a mount +# 2. using a subdirectory of the above directory with 'max_files' quota +# +. ./common/preamble +_begin_fstest auto quick quota + +_supported_fs generic +_require_scratch + +_scratch_mount +mkdir -p $SCRATCH_MNT/quota-dir/subdir + +# set quotas +quota=$((1024*10000)) +$SETFATTR_PROG -n ceph.quota.max_bytes -v $quota $SCRATCH_MNT/quota-dir +$SETFATTR_PROG -n ceph.quota.max_files -v $quota $SCRATCH_MNT/quota-dir/subdir +_scratch_unmount + +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_mount +total=`df -kP $SCRATCH_MNT | grep -v Filesystem | awk '{print $2}'` +SCRATCH_DEV=$SCRATCH_DEV/quota-dir _scratch_unmount +[ $total -eq 8192 ] || _fail "Incorrect statfs for quota-dir: $total" + +SCRATCH_DEV=$SCRATCH_DEV/quota-dir/subdir _scratch_mount +total=`df -kP $SCRATCH_MNT | grep -v Filesystem | awk '{print $2}'` +SCRATCH_DEV=$SCRATCH_DEV/quota-dir/subdir _scratch_unmount +[ $total -eq 8192 ] || _fail "Incorrect statfs for quota-dir/subdir: $total" + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/ceph/005.out b/tests/ceph/005.out new file mode 100644 index 000000000000..a5027f127cf0 --- /dev/null +++ b/tests/ceph/005.out @@ -0,0 +1,2 @@ +QA output created by 005 +Silence is golden
When using a directory with 'max_bytes' quota as a base for a mount, statfs shall use that 'max_bytes' value as the total disk size. That value shall be used even when using subdirectory as base for the mount. A bug was found where, when this subdirectory also had a 'max_files' quota, the real filesystem size would be returned instead of the parent 'max_bytes' quota value. This test case verifies this bug is fixed. Signed-off-by: Luís Henriques <lhenriques@suse.de> --- tests/ceph/005 | 40 ++++++++++++++++++++++++++++++++++++++++ tests/ceph/005.out | 2 ++ 2 files changed, 42 insertions(+) create mode 100755 tests/ceph/005 create mode 100644 tests/ceph/005.out