diff mbox series

ceph/001: skip metrics check if no copyfrom mount option is used

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

Commit Message

Luís Henriques May 20, 2022, 10:50 a.m. UTC
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(+)

Comments

Zorro Lang May 24, 2022, 4:45 a.m. UTC | #1
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
>
Zorro Lang May 24, 2022, 5:04 a.m. UTC | #2
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
> >
Luís Henriques May 24, 2022, 9:37 a.m. UTC | #3
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 mbox series

Patch

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