[PATCH] common/rc: skip atime related tests on CIFS

[PATCH] common/rc: skip atime related tests on CIFS

[XiaoLi Feng]

From: xiaoli feng <xifeng@redhat.com>

The atime related mount options have no effect on cifs mounts
now or in the future. So skip these tests on CIFS.

Signed-off-by: xiaoli feng <xifeng@redhat.com>
---
 common/rc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/rc b/common/rc
index ffe5323..d3af55f 100644
--- a/common/rc
+++ b/common/rc
@@ -3244,6 +3244,8 @@ _require_atime()
 	_exclude_scratch_mount_option "noatime"
 	if [ "$FSTYP" == "nfs" ]; then
 		_notrun "atime related mount options have no effect on NFS"
+	elif [ "$FSTYP" == "cifs" ]; then
+		_notrun "atime related mount options have no effect on CIFS"
 	fi
 }
 
-- 
1.8.3.1

Re: [PATCH] common/rc: skip atime related tests on CIFS

[Eryu Guan]

On Tue, May 22, 2018 at 11:23:56AM +0800, Xiaoli Feng wrote:
> From: xiaoli feng <xifeng@redhat.com>
> 
> The atime related mount options have no effect on cifs mounts
> now or in the future. So skip these tests on CIFS.
> 
> Signed-off-by: xiaoli feng <xifeng@redhat.com>

Looks fine to me. But I really need an ACK from cifs folks.

> ---
>  common/rc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index ffe5323..d3af55f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3244,6 +3244,8 @@ _require_atime()
>  	_exclude_scratch_mount_option "noatime"
>  	if [ "$FSTYP" == "nfs" ]; then
>  		_notrun "atime related mount options have no effect on NFS"
> +	elif [ "$FSTYP" == "cifs" ]; then
> +		_notrun "atime related mount options have no effect on CIFS"

Or just change the check to a case-switch on $FSTYP?

Thanks,
Eryu

>  	fi
>  }
>  
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] common/rc: skip atime related tests on CIFS

[Xiaoli Feng]

Add Ronnie to to list.

Hello Ronnie,
  what's the expected behavior from CIFS on atime/noatime? We want to skip the
atime related tests on cifs for xfstests just like nfs. But not sure if it's
correct. What do you think about it?

Thanks.

----- Original Message -----
> From: "Eryu Guan" <guaneryu@gmail.com>
> To: "XiaoLi Feng" <xifeng@redhat.com>
> Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
> Sent: Sunday, May 27, 2018 8:50:58 PM
> Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
> 
> On Tue, May 22, 2018 at 11:23:56AM +0800, Xiaoli Feng wrote:
> > From: xiaoli feng <xifeng@redhat.com>
> > 
> > The atime related mount options have no effect on cifs mounts
> > now or in the future. So skip these tests on CIFS.
> > 
> > Signed-off-by: xiaoli feng <xifeng@redhat.com>
> 
> Looks fine to me. But I really need an ACK from cifs folks.
> 
> > ---
> >  common/rc | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index ffe5323..d3af55f 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3244,6 +3244,8 @@ _require_atime()
> >  	_exclude_scratch_mount_option "noatime"
> >  	if [ "$FSTYP" == "nfs" ]; then
> >  		_notrun "atime related mount options have no effect on NFS"
> > +	elif [ "$FSTYP" == "cifs" ]; then
> > +		_notrun "atime related mount options have no effect on CIFS"
> 
> Or just change the check to a case-switch on $FSTYP?

Ok, I will change it on the next commit.
Thanks!

> 
> Thanks,
> Eryu
> 
> >  	fi
> >  }
> >  
> > --
> > 1.8.3.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] common/rc: skip atime related tests on CIFS

[Ronnie Sahlberg]

Hi Xiaoli,

I think it is fine to skip the atime tests for CIFS.

The behaviour of atime/noatime for cifs is basically noatime always.
When we read from the server the server will update the atime independently of
cisf.ko but then any further reads that we do from cache will never push any updates
to the server.

Ronnie



----- Original Message -----
> From: "Xiaoli Feng" <xifeng@redhat.com>
> To: "Eryu Guan" <guaneryu@gmail.com>, "Leif Sahlberg" <lsahlber@redhat.com>
> Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
> Sent: Monday, 28 May, 2018 4:40:56 PM
> Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
> 
> 
> Add Ronnie to to list.
> 
> Hello Ronnie,
>   what's the expected behavior from CIFS on atime/noatime? We want to skip
>   the
> atime related tests on cifs for xfstests just like nfs. But not sure if it's
> correct. What do you think about it?
> 
> Thanks.
> 
> ----- Original Message -----
> > From: "Eryu Guan" <guaneryu@gmail.com>
> > To: "XiaoLi Feng" <xifeng@redhat.com>
> > Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
> > Sent: Sunday, May 27, 2018 8:50:58 PM
> > Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
> > 
> > On Tue, May 22, 2018 at 11:23:56AM +0800, Xiaoli Feng wrote:
> > > From: xiaoli feng <xifeng@redhat.com>
> > > 
> > > The atime related mount options have no effect on cifs mounts
> > > now or in the future. So skip these tests on CIFS.
> > > 
> > > Signed-off-by: xiaoli feng <xifeng@redhat.com>
> > 
> > Looks fine to me. But I really need an ACK from cifs folks.
> > 
> > > ---
> > >  common/rc | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index ffe5323..d3af55f 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -3244,6 +3244,8 @@ _require_atime()
> > >  	_exclude_scratch_mount_option "noatime"
> > >  	if [ "$FSTYP" == "nfs" ]; then
> > >  		_notrun "atime related mount options have no effect on NFS"
> > > +	elif [ "$FSTYP" == "cifs" ]; then
> > > +		_notrun "atime related mount options have no effect on CIFS"
> > 
> > Or just change the check to a case-switch on $FSTYP?
> 
> Ok, I will change it on the next commit.
> Thanks!
> 
> > 
> > Thanks,
> > Eryu
> > 
> > >  	fi
> > >  }
> > >  
> > > --
> > > 1.8.3.1
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] common/rc: skip atime related tests on CIFS

[Xiaoli Feng]

Thanks a lot, Ronnie.

----- Original Message -----
> From: "Ronnie Sahlberg" <lsahlber@redhat.com>
> To: "Xiaoli Feng" <xifeng@redhat.com>
> Cc: "Eryu Guan" <guaneryu@gmail.com>, fstests@vger.kernel.org, linux-cifs@vger.kernel.org
> Sent: Tuesday, May 29, 2018 7:26:41 AM
> Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
> 
> Hi Xiaoli,
> 
> I think it is fine to skip the atime tests for CIFS.
> 
> The behaviour of atime/noatime for cifs is basically noatime always.
> When we read from the server the server will update the atime independently
> of
> cisf.ko but then any further reads that we do from cache will never push any
> updates
> to the server.
> 
> Ronnie
> 
> 
> 
> ----- Original Message -----
> > From: "Xiaoli Feng" <xifeng@redhat.com>
> > To: "Eryu Guan" <guaneryu@gmail.com>, "Leif Sahlberg" <lsahlber@redhat.com>
> > Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
> > Sent: Monday, 28 May, 2018 4:40:56 PM
> > Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
> > 
> > 
> > Add Ronnie to to list.
> > 
> > Hello Ronnie,
> >   what's the expected behavior from CIFS on atime/noatime? We want to skip
> >   the
> > atime related tests on cifs for xfstests just like nfs. But not sure if
> > it's
> > correct. What do you think about it?
> > 
> > Thanks.
> > 
> > ----- Original Message -----
> > > From: "Eryu Guan" <guaneryu@gmail.com>
> > > To: "XiaoLi Feng" <xifeng@redhat.com>
> > > Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
> > > Sent: Sunday, May 27, 2018 8:50:58 PM
> > > Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
> > > 
> > > On Tue, May 22, 2018 at 11:23:56AM +0800, Xiaoli Feng wrote:
> > > > From: xiaoli feng <xifeng@redhat.com>
> > > > 
> > > > The atime related mount options have no effect on cifs mounts
> > > > now or in the future. So skip these tests on CIFS.
> > > > 
> > > > Signed-off-by: xiaoli feng <xifeng@redhat.com>
> > > 
> > > Looks fine to me. But I really need an ACK from cifs folks.
> > > 
> > > > ---
> > > >  common/rc | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index ffe5323..d3af55f 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -3244,6 +3244,8 @@ _require_atime()
> > > >  	_exclude_scratch_mount_option "noatime"
> > > >  	if [ "$FSTYP" == "nfs" ]; then
> > > >  		_notrun "atime related mount options have no effect on NFS"
> > > > +	elif [ "$FSTYP" == "cifs" ]; then
> > > > +		_notrun "atime related mount options have no effect on CIFS"
> > > 
> > > Or just change the check to a case-switch on $FSTYP?
> > 
> > Ok, I will change it on the next commit.
> > Thanks!
> > 
> > > 
> > > Thanks,
> > > Eryu
> > > 
> > > >  	fi
> > > >  }
> > > >  
> > > > --
> > > > 1.8.3.1
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH] common/rc: skip atime related tests on CIFS

[Eryu Guan]

On Mon, May 28, 2018 at 11:11:02PM -0500, Steve French wrote:
> Agreed but I do want to follow up with Ronnie on loosely related question of
> how cifs and NFS and similar filesystems parse no op mount options like noatime

I take this as an ACK then, please let me know if you didn't really mean
that :)

Thank you both for review!

Eryu

> 
> On Mon, May 28, 2018, 22:55 Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> 
>     Hi Xiaoli,
> 
>     I think it is fine to skip the atime tests for CIFS.
> 
>     The behaviour of atime/noatime for cifs is basically noatime always.
>     When we read from the server the server will update the atime independently
>     of
>     cisf.ko but then any further reads that we do from cache will never push
>     any updates
>     to the server.
> 
>     Ronnie
> 
>     ----- Original Message -----
>     > From: "Xiaoli Feng" <xifeng@redhat.com>
>     > To: "Eryu Guan" <guaneryu@gmail.com>, "Leif Sahlberg" <
>     lsahlber@redhat.com>
>     > Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
>     > Sent: Monday, 28 May, 2018 4:40:56 PM
>     > Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
>     >
>     > Add Ronnie to to list.
>     >
>     > Hello Ronnie,
>     >   what's the expected behavior from CIFS on atime/noatime? We want to
>     skip
>     >   the
>     > atime related tests on cifs for xfstests just like nfs. But not sure if
>     it's
>     > correct. What do you think about it?
>     >
>     > Thanks.
>     >
>     > ----- Original Message -----
>     > > From: "Eryu Guan" <guaneryu@gmail.com>
>     > > To: "XiaoLi Feng" <xifeng@redhat.com>
>     > > Cc: fstests@vger.kernel.org, linux-cifs@vger.kernel.org
>     > > Sent: Sunday, May 27, 2018 8:50:58 PM
>     > > Subject: Re: [PATCH] common/rc: skip atime related tests on CIFS
>     > >
>     > > On Tue, May 22, 2018 at 11:23:56AM +0800, Xiaoli Feng wrote:
>     > > > From: xiaoli feng <xifeng@redhat.com>
>     > > >
>     > > > The atime related mount options have no effect on cifs mounts
>     > > > now or in the future. So skip these tests on CIFS.
>     > > >
>     > > > Signed-off-by: xiaoli feng <xifeng@redhat.com>
>     > >
>     > > Looks fine to me. But I really need an ACK from cifs folks.
>     > >
>     > > > ---
>     > > >  common/rc | 2 ++
>     > > >  1 file changed, 2 insertions(+)
>     > > >
>     > > > diff --git a/common/rc b/common/rc
>     > > > index ffe5323..d3af55f 100644
>     > > > --- a/common/rc
>     > > > +++ b/common/rc
>     > > > @@ -3244,6 +3244,8 @@ _require_atime()
>     > > >   _exclude_scratch_mount_option "noatime"
>     > > >   if [ "$FSTYP" == "nfs" ]; then
>     > > >           _notrun "atime related mount options have no effect on NFS"
>     > > > + elif [ "$FSTYP" == "cifs" ]; then
>     > > > +         _notrun "atime related mount options have no effect on
>     CIFS"
>     > >
>     > > Or just change the check to a case-switch on $FSTYP?
>     >
>     > Ok, I will change it on the next commit.
>     > Thanks!
>     >
>     > >
>     > > Thanks,
>     > > Eryu
>     > >
>     > > >   fi
>     > > >  }
>     > > > 
>     > > > --
>     > > > 1.8.3.1
>     > > >
>     > > > --
>     > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
>     > > > the body of a message to majordomo@vger.kernel.org
>     > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     > > --
>     > > To unsubscribe from this list: send the line "unsubscribe fstests" in
>     > > the body of a message to majordomo@vger.kernel.org
>     > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     > >
>     > --
>     > To unsubscribe from this list: send the line "unsubscribe fstests" in
>     > the body of a message to majordomo@vger.kernel.org
>     > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     >
>     --
>     To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>     the body of a message to majordomo@vger.kernel.org
>     More majordomo info at  http://vger.kernel.org/majordomo-info.html
>