Message ID | 20190523134549.23103-1-chrubis@suse.cz |
---|---|
State | Accepted |
Headers | show |
Series | [COMMITTED] syscalls/fcntl33: Fix typo overlapfs -> overlayfs | expand |
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.
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.
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.
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.
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
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.
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.
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 --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"); }
Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- testcases/kernel/syscalls/fcntl/fcntl33.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)