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

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

[Luís Henriques]

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(+)

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

Re: [PATCH] ceph/001: skip metrics check if no copyfrom mount option is used

[Zorro Lang]

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
> 

Re: [PATCH] ceph/001: skip metrics check if no copyfrom mount option is used

[Zorro Lang]

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
> > 

Re: [PATCH] ceph/001: skip metrics check if no copyfrom mount option is used

[Luís Henriques]

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,
-- 
Luís

>> > +		return # ... or if we don't have copyfrom mount option
>> > +	fi
>> >  
>> >  	sum=$(($c0+$copies))
>> >  	if [ $sum -ne $c1 ]; then
>> > 
>