[LTP] [PATCH 0/3] Fix O_DIRECT definition for various archs

[LTP] [PATCH 0/3] Fix O_DIRECT definition for various archs

[Petr Vorel]

Hi,

this work is based on Neil's report.

I'm not sure if better would be  if the fallback O_DIRECT definition
would be removed everywhere and tests just define _GNU_SOURCE (no
library code is using O_DIRECT atm).

But the problem was found when we during some debugging use lapi/fcntl.h
inside of the library (where we don't want to set _GNU_SOURCE, otherwise
all C based LTP tests would be _GNU_SOURCE).

Kind regards,
Petr

Petr Vorel (3):
  lapi/fcntl.h: Fix O_DIRECT definition for various archs
  fsstress/global.h: Include lapi/fcntl.h
  ltpscsi/scsimain.c: Remove O_DIRECT fallback definition

 include/lapi/fcntl.h                        | 11 ++++++++++-
 testcases/kernel/fs/fsstress/global.h       |  2 +-
 testcases/kernel/fs/scsi/ltpscsi/scsimain.c |  5 +----
 3 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/3] lapi/fcntl.h: Fix O_DIRECT definition for various archs

[Petr Vorel]

O_DIRECT definitions differ a lot depending on architecture.
Because this can lead to problems (e.g. the generic value O_DIRECT
040000 is O_DIRECTORY on powerpc, address that in lapi file.

NOTE: Deliberately use correct definitions also on less common archs
(maybe LTP even cannot be compiled on m68k or mips, but better to be
safe then sorry).  But the problem would IMHO be avoided if the fallback
O_DIRECT definition would be removed everywhere and tests just define
_GNU_SOURCE (no library code is using O_DIRECT atm).

Reported-by: NeilBrown <neilb@suse.de>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/lapi/fcntl.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/lapi/fcntl.h b/include/lapi/fcntl.h
index 848ac7865..27da9f076 100644
--- a/include/lapi/fcntl.h
+++ b/include/lapi/fcntl.h
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2014 Cyril Hrubis <chrubis@suse.cz>
+ * Copyright (c) Linux Test Project, 2014-2023
  */
 
 #ifndef LAPI_FCNTL_H__
@@ -11,7 +12,15 @@
 #include <sys/socket.h>
 
 #ifndef O_DIRECT
-# define O_DIRECT 040000
+# if defined(__mips__)
+#  define O_DIRECT 0100000
+# elif defined(__arm__) || defined(__aarch64__) || defined(__m68k__)
+#  define O_DIRECT 0200000
+# elif defined(__powerpc__) || defined(__powerpc64__)
+#  define O_DIRECT 0400000
+# else
+#  define O_DIRECT 040000
+# endif
 #endif
 
 #ifndef O_CLOEXEC
-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/3] fsstress/global.h: Include lapi/fcntl.h

[Petr Vorel]

To avoid potential problems with different O_DIRECT values across archs.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/fs/fsstress/global.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/fs/fsstress/global.h b/testcases/kernel/fs/fsstress/global.h
index 4ec382426..863fff263 100644
--- a/testcases/kernel/fs/fsstress/global.h
+++ b/testcases/kernel/fs/fsstress/global.h
@@ -50,7 +50,6 @@
 #include <sys/ioctl.h>
 #include <sys/wait.h>
 #include <sys/types.h>
-#include <fcntl.h>
 #include <stdlib.h>
 #include <dirent.h>
 #include <errno.h>
@@ -58,6 +57,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>
+#include "lapi/fcntl.h"
 
 #ifndef O_DIRECT
 #define O_DIRECT 040000
-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/3] ltpscsi/scsimain.c: Remove O_DIRECT fallback definition

[Petr Vorel]

To avoid potential problems with different O_DIRECT values across archs,
just replace the fallback definition with _GNU_SOURCE definition.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/fs/scsi/ltpscsi/scsimain.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/testcases/kernel/fs/scsi/ltpscsi/scsimain.c b/testcases/kernel/fs/scsi/ltpscsi/scsimain.c
index ae484e0f1..ce23ae1e7 100644
--- a/testcases/kernel/fs/scsi/ltpscsi/scsimain.c
+++ b/testcases/kernel/fs/scsi/ltpscsi/scsimain.c
@@ -28,6 +28,7 @@
 [0x12][   |lu][pg cde][res   ][al len][cntrl ]
 */
 
+#define _GNU_SOURCE
 #include <unistd.h>
 #include <fcntl.h>
 #include <stdio.h>
@@ -53,10 +54,6 @@
 
 #define ME "scsimain: "
 
-#ifndef O_DIRECT
-#define O_DIRECT 040000
-#endif
-
 #define NUMERIC_SCAN_DEF 1	/* change to 0 to make alpha scan default */
 //static char * version_str = "0.21 20030513";
 
-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] fsstress/global.h: Include lapi/fcntl.h

[Petr Vorel]

> To avoid potential problems with different O_DIRECT values across archs.

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/kernel/fs/fsstress/global.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/testcases/kernel/fs/fsstress/global.h b/testcases/kernel/fs/fsstress/global.h
> index 4ec382426..863fff263 100644
> --- a/testcases/kernel/fs/fsstress/global.h
> +++ b/testcases/kernel/fs/fsstress/global.h
> @@ -50,7 +50,6 @@
>  #include <sys/ioctl.h>
>  #include <sys/wait.h>
>  #include <sys/types.h>
> -#include <fcntl.h>
>  #include <stdlib.h>
>  #include <dirent.h>
>  #include <errno.h>
> @@ -58,6 +57,7 @@
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <unistd.h>
> +#include "lapi/fcntl.h"

>  #ifndef O_DIRECT
>  #define O_DIRECT 040000

I forget to amend. This fallback definition should have been removed.
But maybe we should put _GNU_SOURCE definition into the header.

Kind regards,
Petr

+++ testcases/kernel/fs/fsstress/global.h
@@ -59,8 +59,4 @@
 #include <unistd.h>
 #include "lapi/fcntl.h"
 
-#ifndef O_DIRECT
-#define O_DIRECT 040000
-#endif
-
 #endif

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] lapi/fcntl.h: Fix O_DIRECT definition for various archs

[Petr Vorel]

Hi all,

>  #ifndef LAPI_FCNTL_H__
> @@ -11,7 +12,15 @@
>  #include <sys/socket.h>

Maybe we should put note here:
/* Consider #define _GNU_SOURCE if you need O_DIRECT in tests */
>  #ifndef O_DIRECT
> -# define O_DIRECT 040000
> +# if defined(__mips__)
> +#  define O_DIRECT 0100000
> +# elif defined(__arm__) || defined(__aarch64__) || defined(__m68k__)
> +#  define O_DIRECT 0200000
> +# elif defined(__powerpc__) || defined(__powerpc64__)
> +#  define O_DIRECT 0400000
> +# else
> +#  define O_DIRECT 040000
> +# endif
>  #endif

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] lapi/fcntl.h: Fix O_DIRECT definition for various archs

[Cyril Hrubis]

Hi!
> O_DIRECT definitions differ a lot depending on architecture.
> Because this can lead to problems (e.g. the generic value O_DIRECT
> 040000 is O_DIRECTORY on powerpc, address that in lapi file.
> 
> NOTE: Deliberately use correct definitions also on less common archs
> (maybe LTP even cannot be compiled on m68k or mips, but better to be
> safe then sorry).  But the problem would IMHO be avoided if the fallback
> O_DIRECT definition would be removed everywhere and tests just define
> _GNU_SOURCE (no library code is using O_DIRECT atm).
> 
> Reported-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  include/lapi/fcntl.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/lapi/fcntl.h b/include/lapi/fcntl.h
> index 848ac7865..27da9f076 100644
> --- a/include/lapi/fcntl.h
> +++ b/include/lapi/fcntl.h
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2014 Cyril Hrubis <chrubis@suse.cz>
> + * Copyright (c) Linux Test Project, 2014-2023
>   */
>  
>  #ifndef LAPI_FCNTL_H__
> @@ -11,7 +12,15 @@
>  #include <sys/socket.h>
>  
>  #ifndef O_DIRECT
> -# define O_DIRECT 040000
> +# if defined(__mips__)
> +#  define O_DIRECT 0100000
> +# elif defined(__arm__) || defined(__aarch64__) || defined(__m68k__)
> +#  define O_DIRECT 0200000
> +# elif defined(__powerpc__) || defined(__powerpc64__)
> +#  define O_DIRECT 0400000
> +# else
> +#  define O_DIRECT 040000
> +# endif
>  #endif

I do not think that we should default to any value here, just do

#error Define O_DIRECT for your architecture

However I think that actually removing the O_DIRECT fallback and adding
_GNU_SOURCE to tests that use O_DIRECT is more future proof solution.
There is about 70 tests that use O_DIRECT and many of them define
_GNU_SOURCE already.

Actually I tried to remove the fallback from lapi/fcntl.h and recompiled
LTP without any change, which is strange. Does that work for you as
well?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] lapi/fcntl.h: Fix O_DIRECT definition for various archs

[Petr Vorel]

> Hi!
> > O_DIRECT definitions differ a lot depending on architecture.
> > Because this can lead to problems (e.g. the generic value O_DIRECT
> > 040000 is O_DIRECTORY on powerpc, address that in lapi file.

> > NOTE: Deliberately use correct definitions also on less common archs
> > (maybe LTP even cannot be compiled on m68k or mips, but better to be
> > safe then sorry).  But the problem would IMHO be avoided if the fallback
> > O_DIRECT definition would be removed everywhere and tests just define
> > _GNU_SOURCE (no library code is using O_DIRECT atm).

> > Reported-by: NeilBrown <neilb@suse.de>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  include/lapi/fcntl.h | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)

> > diff --git a/include/lapi/fcntl.h b/include/lapi/fcntl.h
> > index 848ac7865..27da9f076 100644
> > --- a/include/lapi/fcntl.h
> > +++ b/include/lapi/fcntl.h
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * Copyright (c) 2014 Cyril Hrubis <chrubis@suse.cz>
> > + * Copyright (c) Linux Test Project, 2014-2023
> >   */

> >  #ifndef LAPI_FCNTL_H__
> > @@ -11,7 +12,15 @@
> >  #include <sys/socket.h>

> >  #ifndef O_DIRECT
> > -# define O_DIRECT 040000
> > +# if defined(__mips__)
> > +#  define O_DIRECT 0100000
> > +# elif defined(__arm__) || defined(__aarch64__) || defined(__m68k__)
> > +#  define O_DIRECT 0200000
> > +# elif defined(__powerpc__) || defined(__powerpc64__)
> > +#  define O_DIRECT 0400000
> > +# else
> > +#  define O_DIRECT 040000
> > +# endif
> >  #endif

> I do not think that we should default to any value here, just do

> #error Define O_DIRECT for your architecture

> However I think that actually removing the O_DIRECT fallback and adding
> _GNU_SOURCE to tests that use O_DIRECT is more future proof solution.
> There is about 70 tests that use O_DIRECT and many of them define
> _GNU_SOURCE already.

I have no problem to remove O_DIRECT. The only question is what to do if we need
it in the library one day. Will we be ok that whole LTP becaume _GNU_SOURCE due
that?

> Actually I tried to remove the fallback from lapi/fcntl.h and recompiled
> LTP without any change, which is strange. Does that work for you as
> well?

I'm verifying it [1] in a patch, where I only defined _GNU_SOURCE in
testcases/kernel/fs/scsi/ltpscsi/scsimain.c and
testcases/kernel/fs/fsstress/global.h.

Kind regards,
Petr

[1] https://github.com/pevik/ltp/actions/runs/5401264765

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] lapi/fcntl.h: Fix O_DIRECT definition for various archs

[Cyril Hrubis]

Hi!
> I have no problem to remove O_DIRECT. The only question is what to do if we need
> it in the library one day. Will we be ok that whole LTP becaume _GNU_SOURCE due
> that?

Nothing stops us from adding _GNU_SOURCE into a lib/foo.c if that is
ever needed.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] lapi/fcntl.h: Fix O_DIRECT definition for various archs

[Petr Vorel]

Hi Cyril,

> Hi!
> > I have no problem to remove O_DIRECT. The only question is what to do if we need
> > it in the library one day. Will we be ok that whole LTP becaume _GNU_SOURCE due
> > that?

> Nothing stops us from adding _GNU_SOURCE into a lib/foo.c if that is
> ever needed.

You're right, now I wonder why I didn't realize that it does not have to be
added to header file, just to C file :). Thanks!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp