[PATCH v3] common/rc: add filter in _test_inode_flag

[PATCH v3] common/rc: add filter in _test_inode_flag

[XiaoLi Feng]

From: Xiaoli Feng <xifeng@redhat.com>

Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when
mount point contains flag string.

Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
---
 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 649b1cfd..473188c1 100644
--- a/common/rc
+++ b/common/rc
@@ -3112,7 +3112,7 @@ _test_inode_flag()
 	local flag=$1
 	local file=$2
 
-	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then
+	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | grep -q "$flag" ; then
 		return 0
 	fi
 	return 1
-- 
2.18.1

Re: [PATCH v3] common/rc: add filter in _test_inode_flag

[Zorro Lang]

On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote:
> From: Xiaoli Feng <xifeng@redhat.com>
> 
> Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when
> mount point contains flag string.
> 
> Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
> ---
>  common/rc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 649b1cfd..473188c1 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3112,7 +3112,7 @@ _test_inode_flag()
>  	local flag=$1
>  	local file=$2
>  
> -	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then
> +	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch | grep -q "$flag" ; then

I can't understand that. Could you give us an example about why we need this
filter before "quiet grep"? I think this line doesn't have any output
(except xfs_io fails), so why a filter is needed?

Thanks,
Zorro

>  		return 0
>  	fi
>  	return 1
> -- 
> 2.18.1
> 

Re: [PATCH v3] common/rc: add filter in _test_inode_flag

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Zorro Lang" <zlang@redhat.com>
> 收件人: "XiaoLi Feng" <xifeng@redhat.com>
> 抄送: fstests@vger.kernel.org
> 发送时间: 星期四, 2021年 2 月 18日 下午 9:41:34
> 主题: Re: [PATCH v3] common/rc: add filter in _test_inode_flag
> 
> On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote:
> > From: Xiaoli Feng <xifeng@redhat.com>
> > 
> > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when
> > mount point contains flag string.
> > 
> > Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
> > ---
> >  common/rc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 649b1cfd..473188c1 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3112,7 +3112,7 @@ _test_inode_flag()
> >  	local flag=$1
> >  	local file=$2
> >  
> > -	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then
> > +	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch |
> > grep -q "$flag" ; then
> 
> I can't understand that. Could you give us an example about why we need this
> filter before "quiet grep"? I think this line doesn't have any output
> (except xfs_io fails), so why a filter is needed?

Oh, I just saw your reply for V1 patch review:

"When test generic/608 for dax on xfs, "_check_xflag $t_file 0" is always
failed if the $f_file has dax string. Yes, here should also include filter
for TEST_DIR."

The _check_xflag() function as below:

_check_xflag()
{
        local target=$1
        local exp_xflag=$2

        if [ $exp_xflag -eq 0 ]; then
                _test_inode_flag dax $target && echo "$target has unexpected FS_XFLAG_DAX flag"
        else
                _test_inode_flag dax $target || echo "$target doesn't have expected FS_XFLAG_DAX flag"
        fi
}

Sure, if the $1 is something likes "/mnt/scratch/file", later echo "$target ...." will print "/mnt/scratch".
If you're trying to fix this issue, I think you should filter the "$target" in or behind _check_xflag(),
not in _test_inode_flag() which has no output.

Thanks,
Zorro

> 
> Thanks,
> Zorro
> 
> >  		return 0
> >  	fi
> >  	return 1
> > --
> > 2.18.1
> > 
> 

Re: [PATCH v3] common/rc: add filter in _test_inode_flag

[Xiaoli Feng]

Hi,

----- Original Message -----
> From: "Zorro Lang" <zlang@redhat.com>
> To: "XiaoLi Feng" <xifeng@redhat.com>
> Cc: fstests@vger.kernel.org
> Sent: Thursday, February 18, 2021 9:41:35 PM
> Subject: Re: [PATCH v3] common/rc: add filter in _test_inode_flag
> 
> On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote:
> > From: Xiaoli Feng <xifeng@redhat.com>
> > 
> > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when
> > mount point contains flag string.
> > 
> > Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
> > ---
> >  common/rc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 649b1cfd..473188c1 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3112,7 +3112,7 @@ _test_inode_flag()
> >  	local flag=$1
> >  	local file=$2
> >  
> > -	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then
> > +	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch |
> > grep -q "$flag" ; then
> 
> I can't understand that. Could you give us an example about why we need this
> filter before "quiet grep"? I think this line doesn't have any output
> (except xfs_io fails), so why a filter is needed?

Yes. this line doesn't have any output. But the return value is different. I send
this patch to fix the issue: If testdir or scratch_dir has "$flag". Then it will
always return 0. Even if this file doesn't have this "$flag".

Example:
 xfs_io -r -c 'lsattr -v' /daxmnt/fileB
[] /daxmnt/fileB
# xfs_io -r -c 'lsattr -v' /daxmnt/fileB | grep -q "dax"
# echo $?
0

fileB doesn't have dax flag. But _test_inode_flag will always return 0(expect 1).

Thanks.

> 
> Thanks,
> Zorro
> 
> >  		return 0
> >  	fi
> >  	return 1
> > --
> > 2.18.1
> > 
> 
> 

Re: [PATCH v3] common/rc: add filter in _test_inode_flag

[Zorro Lang]

On Fri, Feb 19, 2021 at 11:00:18AM -0500, Xiaoli Feng wrote:
> Hi,
> 
> ----- Original Message -----
> > From: "Zorro Lang" <zlang@redhat.com>
> > To: "XiaoLi Feng" <xifeng@redhat.com>
> > Cc: fstests@vger.kernel.org
> > Sent: Thursday, February 18, 2021 9:41:35 PM
> > Subject: Re: [PATCH v3] common/rc: add filter in _test_inode_flag
> > 
> > On Thu, Feb 18, 2021 at 04:57:52PM +0800, XiaoLi Feng wrote:
> > > From: Xiaoli Feng <xifeng@redhat.com>
> > > 
> > > Add _filter_testdir_and_scratch to avoid _test_inode_flag failed when
> > > mount point contains flag string.
> > > 
> > > Signed-off-by: Xiaoli Feng <xifeng@redhat.com>
> > > ---
> > >  common/rc | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 649b1cfd..473188c1 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -3112,7 +3112,7 @@ _test_inode_flag()
> > >  	local flag=$1
> > >  	local file=$2
> > >  
> > > -	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | grep -q "$flag" ; then
> > > +	if $XFS_IO_PROG -r -c 'lsattr -v' "$file" | _filter_testdir_and_scratch |
> > > grep -q "$flag" ; then
> > 
> > I can't understand that. Could you give us an example about why we need this
> > filter before "quiet grep"? I think this line doesn't have any output
> > (except xfs_io fails), so why a filter is needed?
> 
> Yes. this line doesn't have any output. But the return value is different. I send
> this patch to fix the issue: If testdir or scratch_dir has "$flag". Then it will
> always return 0. Even if this file doesn't have this "$flag".
> 
> Example:
>  xfs_io -r -c 'lsattr -v' /daxmnt/fileB
> [] /daxmnt/fileB
> # xfs_io -r -c 'lsattr -v' /daxmnt/fileB | grep -q "dax"
> # echo $?
> 0
> 
> fileB doesn't have dax flag. But _test_inode_flag will always return 0(expect 1).

Oh, got it. So the 'grep -q "dax"' is a litte inaccurate. If the mountpoint or
file name, or any sub-string in the $file contains "dax", this function will
return 0.

If that's the issue you're fixing, I think only filter SCRATCH_MNT and TEST_MNT
isn't enough. If we only care about the "attr" part output, how about filter
out junk (path name), only keep the [attr]?

Due to looks like the output format of 'lsattr -v' is: "[$attr] $filename" (correct
me if I'm wrong). So if we only keep the attr part, that can avoid the 'grep' find
"dax" from $filename.

Thanks,
Zorro

> 
> Thanks.
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >  		return 0
> > >  	fi
> > >  	return 1
> > > --
> > > 2.18.1
> > > 
> > 
> >