* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-23 15:42 ` [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs Amir Goldstein
@ 2019-05-23 16:46 ` J. Bruce Fields
2019-05-23 17:26 ` Amir Goldstein
2019-05-23 20:01 ` Petr Vorel
2019-05-24 8:59 ` Cyril Hrubis
2 siblings, 1 reply; 8+ messages in thread
From: J. Bruce Fields @ 2019-05-23 16:46 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Cyril Hrubis, ltp, overlayfs, Miklos Szeredi, Petr Vorel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-23 16:46 ` J. Bruce Fields
@ 2019-05-23 17:26 ` Amir Goldstein
2019-05-23 17:40 ` J. Bruce Fields
0 siblings, 1 reply; 8+ messages in thread
From: Amir Goldstein @ 2019-05-23 17:26 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Cyril Hrubis, ltp, overlayfs, Miklos Szeredi, Petr Vorel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-23 17:26 ` Amir Goldstein
@ 2019-05-23 17:40 ` J. Bruce Fields
0 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2019-05-23 17:40 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Cyril Hrubis, ltp, overlayfs, Miklos Szeredi, Petr Vorel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-23 15:42 ` [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs Amir Goldstein
2019-05-23 16:46 ` J. Bruce Fields
@ 2019-05-23 20:01 ` Petr Vorel
2019-05-24 8:59 ` Cyril Hrubis
2 siblings, 0 replies; 8+ messages in thread
From: Petr Vorel @ 2019-05-23 20:01 UTC (permalink / raw)
To: Amir Goldstein
Cc: Cyril Hrubis, ltp, overlayfs, Miklos Szeredi, J. Bruce Fields,
Li Wang
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-23 15:42 ` [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs Amir Goldstein
2019-05-23 16:46 ` J. Bruce Fields
2019-05-23 20:01 ` Petr Vorel
@ 2019-05-24 8:59 ` Cyril Hrubis
2019-05-24 11:03 ` Amir Goldstein
2 siblings, 1 reply; 8+ messages in thread
From: Cyril Hrubis @ 2019-05-24 8:59 UTC (permalink / raw)
To: Amir Goldstein
Cc: ltp, overlayfs, Miklos Szeredi, Petr Vorel, J. Bruce Fields
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.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-24 8:59 ` Cyril Hrubis
@ 2019-05-24 11:03 ` Amir Goldstein
2019-05-24 13:45 ` Cyril Hrubis
0 siblings, 1 reply; 8+ messages in thread
From: Amir Goldstein @ 2019-05-24 11:03 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp, overlayfs, Miklos Szeredi, Petr Vorel, J. Bruce Fields
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [LTP] [PATCH] [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs
2019-05-24 11:03 ` Amir Goldstein
@ 2019-05-24 13:45 ` Cyril Hrubis
0 siblings, 0 replies; 8+ messages in thread
From: Cyril Hrubis @ 2019-05-24 13:45 UTC (permalink / raw)
To: Amir Goldstein
Cc: ltp, overlayfs, Miklos Szeredi, Petr Vorel, J. Bruce Fields
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...
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 8+ messages in thread