| Message ID | 20220520105055.31520-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph/001: skip metrics check if no copyfrom mount option is used | expand |
On Fri, May 20, 2022 at 11:50:55AM +0100, Luís Henriques wrote: > Checking the metrics is only valid if 'copyfrom' mount option is > explicitly set, otherwise the kernel won't be doing any remote object > copies. Fix the logic to skip this metrics checking if 'copyfrom' isn't > used. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- The code logic looks good to me, but I'm not familiar with cephfs, so if there's not review points or objection from ceph-fs, I'll merge this patch this week. Reviewed-by: Zorro Lang <zlang@redhat.com> Thanks, Zorro > tests/ceph/001 | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tests/ceph/001 b/tests/ceph/001 > index 7970ce352bab..2e6a5e6be2d6 100755 > --- a/tests/ceph/001 > +++ b/tests/ceph/001 > @@ -86,11 +86,15 @@ check_copyfrom_metrics() > local copies=$4 > local c1=$(get_copyfrom_total_copies) > local s1=$(get_copyfrom_total_size) > + local hascopyfrom=$(_fs_options $TEST_DEV | grep "copyfrom") > local sum > > if [ ! -d $metrics_dir ]; then > return # skip metrics check if debugfs isn't mounted > fi > + if [ -z $hascopyfrom ]; then > + return # ... or if we don't have copyfrom mount option > + fi > > sum=$(($c0+$copies)) > if [ $sum -ne $c1 ]; then >
On Tue, May 24, 2022 at 12:45:07PM +0800, Zorro Lang wrote: > On Fri, May 20, 2022 at 11:50:55AM +0100, Luís Henriques wrote: > > Checking the metrics is only valid if 'copyfrom' mount option is > > explicitly set, otherwise the kernel won't be doing any remote object > > copies. Fix the logic to skip this metrics checking if 'copyfrom' isn't > > used. > > > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > > --- > > The code logic looks good to me, but I'm not familiar with cephfs, so if there's > not review points or objection from ceph-fs, I'll merge this patch this week. > > Reviewed-by: Zorro Lang <zlang@redhat.com> > > Thanks, > Zorro > > > tests/ceph/001 | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/tests/ceph/001 b/tests/ceph/001 > > index 7970ce352bab..2e6a5e6be2d6 100755 > > --- a/tests/ceph/001 > > +++ b/tests/ceph/001 > > @@ -86,11 +86,15 @@ check_copyfrom_metrics() > > local copies=$4 > > local c1=$(get_copyfrom_total_copies) > > local s1=$(get_copyfrom_total_size) > > + local hascopyfrom=$(_fs_options $TEST_DEV | grep "copyfrom") Oh, I forgot to metion that: I don't know what's the mount option output format at here, can't sure if "grep -w" is needed, or need special pattern. > > local sum > > > > if [ ! -d $metrics_dir ]; then > > return # skip metrics check if debugfs isn't mounted > > fi > > + if [ -z $hascopyfrom ]; then I think better to use quotes ("$hascopyfrom") at here. > > + return # ... or if we don't have copyfrom mount option > > + fi > > > > sum=$(($c0+$copies)) > > if [ $sum -ne $c1 ]; then > >
Zorro Lang <zlang@redhat.com> writes: > On Tue, May 24, 2022 at 12:45:07PM +0800, Zorro Lang wrote: >> On Fri, May 20, 2022 at 11:50:55AM +0100, Luís Henriques wrote: >> > Checking the metrics is only valid if 'copyfrom' mount option is >> > explicitly set, otherwise the kernel won't be doing any remote object >> > copies. Fix the logic to skip this metrics checking if 'copyfrom' isn't >> > used. >> > >> > Signed-off-by: Luís Henriques <lhenriques@suse.de> >> > --- >> >> The code logic looks good to me, but I'm not familiar with cephfs, so if there's >> not review points or objection from ceph-fs, I'll merge this patch this week. >> >> Reviewed-by: Zorro Lang <zlang@redhat.com> >> >> Thanks, >> Zorro >> >> > tests/ceph/001 | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/tests/ceph/001 b/tests/ceph/001 >> > index 7970ce352bab..2e6a5e6be2d6 100755 >> > --- a/tests/ceph/001 >> > +++ b/tests/ceph/001 >> > @@ -86,11 +86,15 @@ check_copyfrom_metrics() >> > local copies=$4 >> > local c1=$(get_copyfrom_total_copies) >> > local s1=$(get_copyfrom_total_size) >> > + local hascopyfrom=$(_fs_options $TEST_DEV | grep "copyfrom") > > Oh, I forgot to metion that: > > I don't know what's the mount option output format at here, can't sure if > "grep -w" is needed, or need special pattern. I don't think there's a need for the '-w' here; the mount option is exactly that string: 192.168.155.1:40768:/ on /mnt type ceph (rw,relatime,name=user,secret=<hidden>,acl,copyfrom) >> > local sum >> > >> > if [ ! -d $metrics_dir ]; then >> > return # skip metrics check if debugfs isn't mounted >> > fi >> > + if [ -z $hascopyfrom ]; then > > I think better to use quotes ("$hascopyfrom") at here. I'll send v2 with that changed. Thank you for your feedback. Cheers,
diff --git a/tests/ceph/001 b/tests/ceph/001 index 7970ce352bab..2e6a5e6be2d6 100755 --- a/tests/ceph/001 +++ b/tests/ceph/001 @@ -86,11 +86,15 @@ check_copyfrom_metrics() local copies=$4 local c1=$(get_copyfrom_total_copies) local s1=$(get_copyfrom_total_size) + local hascopyfrom=$(_fs_options $TEST_DEV | grep "copyfrom") local sum if [ ! -d $metrics_dir ]; then return # skip metrics check if debugfs isn't mounted fi + if [ -z $hascopyfrom ]; then + return # ... or if we don't have copyfrom mount option + fi sum=$(($c0+$copies)) if [ $sum -ne $c1 ]; then
Checking the metrics is only valid if 'copyfrom' mount option is explicitly set, otherwise the kernel won't be doing any remote object copies. Fix the logic to skip this metrics checking if 'copyfrom' isn't used. Signed-off-by: Luís Henriques <lhenriques@suse.de> --- tests/ceph/001 | 4 ++++ 1 file changed, 4 insertions(+)