diff mbox series

[COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs

Message ID 20190523134549.23103-1-chrubis@suse.cz
State Accepted
Headers show
Series [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs | expand

Commit Message

Cyril Hrubis May 23, 2019, 1:45 p.m. UTC 
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Amir Goldstein May 23, 2019, 3:42 p.m. UTC | #1 
On Thu, May 23, 2019 at 4:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> index 43dc5a2af..627823c5c 100644
> --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
>         if (TST_RET == -1) {
>                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
>                         tst_res(TINFO | TTERRNO,
> -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");

You have 3 more of this typo in fcntl tests.

If you ask me, silencing this error seems wrong.
While the error is *expected* it is still a broken interface.
It may be just a matter of terminology, but I am reading this message as:

TEST PASSED: Overlayfs failed as expected

While it really should be more along the lines of:

TEST SKIPPED: Overlayfs doesn't support write leased

Besides, this problem looks quite easy to fix.
I think Bruce was already looking at changing the implementation of
check_conflicting_open(), so if the test is not failing, nobody is going to
nudge for a fix...

Thanks,
Amir.
J. Bruce Fields May 23, 2019, 4:46 p.m. UTC | #2 
On Thu, May 23, 2019 at 06:42:12PM +0300, Amir Goldstein wrote:
> On Thu, May 23, 2019 at 4:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> >
> > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> > ---
> >  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > index 43dc5a2af..627823c5c 100644
> > --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> > +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
> >         if (TST_RET == -1) {
> >                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
> >                         tst_res(TINFO | TTERRNO,
> > -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> > +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");
> 
> You have 3 more of this typo in fcntl tests.
> 
> If you ask me, silencing this error seems wrong.
> While the error is *expected* it is still a broken interface.
> It may be just a matter of terminology, but I am reading this message as:
> 
> TEST PASSED: Overlayfs failed as expected
> 
> While it really should be more along the lines of:
> 
> TEST SKIPPED: Overlayfs doesn't support write leased
> 
> Besides, this problem looks quite easy to fix.
> I think Bruce was already looking at changing the implementation of
> check_conflicting_open(), so if the test is not failing, nobody is going to
> nudge for a fix...

Um, I remember that discussion but I can't remember what the obstacles
were in the end.  Trying to find that thread....

--b.
Amir Goldstein May 23, 2019, 5:26 p.m. UTC | #3 
On Thu, May 23, 2019 at 7:46 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, May 23, 2019 at 06:42:12PM +0300, Amir Goldstein wrote:
> > On Thu, May 23, 2019 at 4:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> > >
> > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> > > ---
> > >  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > index 43dc5a2af..627823c5c 100644
> > > --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
> > >         if (TST_RET == -1) {
> > >                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
> > >                         tst_res(TINFO | TTERRNO,
> > > -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> > > +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");
> >
> > You have 3 more of this typo in fcntl tests.
> >
> > If you ask me, silencing this error seems wrong.
> > While the error is *expected* it is still a broken interface.
> > It may be just a matter of terminology, but I am reading this message as:
> >
> > TEST PASSED: Overlayfs failed as expected
> >
> > While it really should be more along the lines of:
> >
> > TEST SKIPPED: Overlayfs doesn't support write leased
> >
> > Besides, this problem looks quite easy to fix.
> > I think Bruce was already looking at changing the implementation of
> > check_conflicting_open(), so if the test is not failing, nobody is going to
> > nudge for a fix...
>
> Um, I remember that discussion but I can't remember what the obstacles
> were in the end.  Trying to find that thread....
>

i_readcount exists, but its with #ifdef CONFIG_IMA and it counts
only O_RDONLY users.

It wouldn't increase struct inode if we always have i_readcount for
64bit arch.

I think F_WRLCK should require i_readcount == 0 && i_writecount == 1.

Can't remember if and why you needed the readers count?

Thanks,
Amir.
J. Bruce Fields May 23, 2019, 5:40 p.m. UTC | #4 
On Thu, May 23, 2019 at 08:26:24PM +0300, Amir Goldstein wrote:
> On Thu, May 23, 2019 at 7:46 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Thu, May 23, 2019 at 06:42:12PM +0300, Amir Goldstein wrote:
> > > On Thu, May 23, 2019 at 4:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> > > >
> > > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> > > > ---
> > > >  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > > index 43dc5a2af..627823c5c 100644
> > > > --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > > +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > > @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
> > > >         if (TST_RET == -1) {
> > > >                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
> > > >                         tst_res(TINFO | TTERRNO,
> > > > -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> > > > +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");
> > >
> > > You have 3 more of this typo in fcntl tests.
> > >
> > > If you ask me, silencing this error seems wrong.
> > > While the error is *expected* it is still a broken interface.
> > > It may be just a matter of terminology, but I am reading this message as:
> > >
> > > TEST PASSED: Overlayfs failed as expected
> > >
> > > While it really should be more along the lines of:
> > >
> > > TEST SKIPPED: Overlayfs doesn't support write leased
> > >
> > > Besides, this problem looks quite easy to fix.
> > > I think Bruce was already looking at changing the implementation of
> > > check_conflicting_open(), so if the test is not failing, nobody is going to
> > > nudge for a fix...
> >
> > Um, I remember that discussion but I can't remember what the obstacles
> > were in the end.  Trying to find that thread....
> >
> 
> i_readcount exists, but its with #ifdef CONFIG_IMA and it counts
> only O_RDONLY users.
> 
> It wouldn't increase struct inode if we always have i_readcount for
> 64bit arch.
> 
> I think F_WRLCK should require i_readcount == 0 && i_writecount == 1.
> 
> Can't remember if and why you needed the readers count?

We don't want to grant a write lease while somebody holds a read open.

This is the last message on the previous thread that I was thinking of:

	https://marc.info/?l=linux-fsdevel&m=155026138713969&w=2

I haven't gotten back to it since then.

--b.
Petr Vorel May 23, 2019, 8:01 p.m. UTC | #5 
Hi,

> On Thu, May 23, 2019 at 4:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> > ---
> >  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > index 43dc5a2af..627823c5c 100644
> > --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> > +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
> >         if (TST_RET == -1) {
> >                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
> >                         tst_res(TINFO | TTERRNO,
> > -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> > +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");

> You have 3 more of this typo in fcntl tests.
Thanks for report, Amir. Cyril will fix it tomorrow I guess.

> If you ask me, silencing this error seems wrong.
> While the error is *expected* it is still a broken interface.
> It may be just a matter of terminology, but I am reading this message as:

> TEST PASSED: Overlayfs failed as expected

> While it really should be more along the lines of:

> TEST SKIPPED: Overlayfs doesn't support write leased
+1, so besides changed phrasing use TCONF instead of TINFO in the error message.

> Besides, this problem looks quite easy to fix.
> I think Bruce was already looking at changing the implementation of
> check_conflicting_open(), so if the test is not failing, nobody is going to
> nudge for a fix...

> Thanks,
> Amir.


Kind regards,
Petr
Cyril Hrubis May 24, 2019, 8:59 a.m. UTC | #6 
Hi!
> >  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > index 43dc5a2af..627823c5c 100644
> > --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> > +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
> >         if (TST_RET == -1) {
> >                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
> >                         tst_res(TINFO | TTERRNO,
> > -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> > +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");
> 
> You have 3 more of this typo in fcntl tests.

Ah, right, I should have done git grep before commiting this. I will fix
that right away.

> If you ask me, silencing this error seems wrong.
> While the error is *expected* it is still a broken interface.
> It may be just a matter of terminology, but I am reading this message as:
> 
> TEST PASSED: Overlayfs failed as expected
> 
> While it really should be more along the lines of:
> 
> TEST SKIPPED: Overlayfs doesn't support write leased

Agreed, I'm always against working around kernel bugs/deficiencies in
tests, unfortunately that usually conflicts with QA deparenments that
wants to skip known problems and have everything green. So we usually
end up somewhere in a middle ground.

Also as usuall, do you care enough to send a patch? :-)

> Besides, this problem looks quite easy to fix.
> I think Bruce was already looking at changing the implementation of
> check_conflicting_open(), so if the test is not failing, nobody is going to
> nudge for a fix...

Once it's fixed we can change that to a failure for new enough kernels,
old ones should probably stay with SKIPPED/TCONF.
Amir Goldstein May 24, 2019, 11:03 a.m. UTC | #7 
On Fri, May 24, 2019 at 11:59 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > >  testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > index 43dc5a2af..627823c5c 100644
> > > --- a/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > +++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
> > > @@ -117,7 +117,7 @@ static void do_test(unsigned int i)
> > >         if (TST_RET == -1) {
> > >                 if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
> > >                         tst_res(TINFO | TTERRNO,
> > > -                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
> > > +                               "fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");
> >
> > You have 3 more of this typo in fcntl tests.
>
> Ah, right, I should have done git grep before commiting this. I will fix
> that right away.
>
> > If you ask me, silencing this error seems wrong.
> > While the error is *expected* it is still a broken interface.
> > It may be just a matter of terminology, but I am reading this message as:
> >
> > TEST PASSED: Overlayfs failed as expected
> >
> > While it really should be more along the lines of:
> >
> > TEST SKIPPED: Overlayfs doesn't support write leased
>
> Agreed, I'm always against working around kernel bugs/deficiencies in
> tests, unfortunately that usually conflicts with QA deparenments that
> wants to skip known problems and have everything green. So we usually
> end up somewhere in a middle ground.

But is everything green though?
Does QA department know that if you run samba inside a container
whose storage driver is overlayfs, if samba is configured with
"kernel oplock = yes"
Samba clients would never be able to acquire an oplock and
write performance would be horrible?

Sure, not everyone cares about this case, but seems to be that
silencing the error should be in the hands of the user and that LTP
project should just report the problems as they are.

Worse is the fact that this error will only trigger for people that
configured LTP to test overlayfs specifically, not all LTP users.
This group of users is even more likely to be interested in
bugs/deficiencies of overlayfs.

>
> Also as usuall, do you care enough to send a patch? :-)

No, not yet.
Give me a few days to cook.
When I get to caring enough I will fix the kernel ;-)

>
> > Besides, this problem looks quite easy to fix.
> > I think Bruce was already looking at changing the implementation of
> > check_conflicting_open(), so if the test is not failing, nobody is going to
> > nudge for a fix...
>
> Once it's fixed we can change that to a failure for new enough kernels,
> old ones should probably stay with SKIPPED/TCONF.
>

This too would be wrong practice IMO.
If stable kernel users see that the test passes on mainline and fails
on old kernel, somebody may get the idea to backport the fix to stable kernel
and fix the bug.
IOW, setting min_kver is a tool that should be reserved IMO to situations
where:
1. The interface/functionality does not exist -OR-
2. The maintainers have made it clear that the fix will not be backported

Anyway, just my POV.
I full understand the reasons for the "all green" methodology.

Thanks,
Amir.
Cyril Hrubis May 24, 2019, 1:45 p.m. UTC | #8 
Hi!
> > Agreed, I'm always against working around kernel bugs/deficiencies in
> > tests, unfortunately that usually conflicts with QA deparenments that
> > wants to skip known problems and have everything green. So we usually
> > end up somewhere in a middle ground.
> 
> But is everything green though?
> Does QA department know that if you run samba inside a container
> whose storage driver is overlayfs, if samba is configured with
> "kernel oplock = yes"
> Samba clients would never be able to acquire an oplock and
> write performance would be horrible?
> 
> Sure, not everyone cares about this case, but seems to be that
> silencing the error should be in the hands of the user and that LTP
> project should just report the problems as they are.
> 
> Worse is the fact that this error will only trigger for people that
> configured LTP to test overlayfs specifically, not all LTP users.
> This group of users is even more likely to be interested in
> bugs/deficiencies of overlayfs.

I can see how this is wrong.

On the other hand it took us some time to explain our release managers
that kernel is OK when we say that it's OK and that the actuall test
results are not the end result. But even then we never attempted to
to put workarounds into the upstream tests. So I guess that we can
remove the workaround when there is a fix in upstream.

> > Also as usuall, do you care enough to send a patch? :-)
> 
> No, not yet.
> Give me a few days to cook.
> When I get to caring enough I will fix the kernel ;-)

Ok.

> > > Besides, this problem looks quite easy to fix.
> > > I think Bruce was already looking at changing the implementation of
> > > check_conflicting_open(), so if the test is not failing, nobody is going to
> > > nudge for a fix...
> >
> > Once it's fixed we can change that to a failure for new enough kernels,
> > old ones should probably stay with SKIPPED/TCONF.
> >
> 
> This too would be wrong practice IMO.
> If stable kernel users see that the test passes on mainline and fails
> on old kernel, somebody may get the idea to backport the fix to stable kernel
> and fix the bug.
> IOW, setting min_kver is a tool that should be reserved IMO to situations
> where:
> 1. The interface/functionality does not exist -OR-
> 2. The maintainers have made it clear that the fix will not be backported

It's even worse with the distribution kernels that have arbitrary
version numbers and thousands of patches on the top of it, so we use it
as a last option...
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/fcntl/fcntl33.c b/testcases/kernel/syscalls/fcntl/fcntl33.c
index 43dc5a2af..627823c5c 100644
--- a/testcases/kernel/syscalls/fcntl/fcntl33.c
+++ b/testcases/kernel/syscalls/fcntl/fcntl33.c
@@ -117,7 +117,7 @@  static void do_test(unsigned int i)
 	if (TST_RET == -1) {
 		if (type == TST_OVERLAYFS_MAGIC && TST_ERR == EAGAIN) {
 			tst_res(TINFO | TTERRNO,
-				"fcntl(F_SETLEASE, F_WRLCK) failed on overlapfs as expected");
+				"fcntl(F_SETLEASE, F_WRLCK) failed on overlayfs as expected");
 		} else {
 			tst_res(TFAIL | TTERRNO, "fcntl() failed to set lease");
 		}