diff mbox series

[v1] aiocp: Filter out O_DIRECT before read

Message ID 20230209090307.491586-1-pifang@redhat.com
State Changes Requested
Headers show
Series [v1] aiocp: Filter out O_DIRECT before read | expand

Commit Message

Ping Fang Feb. 9, 2023, 9:03 a.m. UTC
When aiocp executed with -f DIRECT will fail.

<<<test_start>>>
tag=AD049 stime=1675520824
cmdline="aiocp -b 8k -n 8 -f DIRECT"
contacts=""
analysis=exit
<<<test_output>>>
tst_test.c:1560: TINFO: Timeout per run is 0h 30m 30s
aiocp.c:211: TINFO: Maximum AIO blocks: 65536
tst_device.c:585: TINFO: Use uevent strategy
aiocp.c:250: TINFO: Fill srcfile.bin with random data
aiocp.c:279: TINFO: Copy srcfile.bin -> dstfile.bin
aiocp.c:291: TINFO: Comparing srcfile.bin with dstfile.bin
aiocp.c:306: TBROK: read(3,0x7ffcd743abe0,4096) failed, returned -1: EINVAL (22)
...

syscall read manual ERROR section said that:
EINVAL fd  is  attached to an object which is unsuitable for reading;
or the file was opened with the O_DIRECT flag, and either the address
specified in buf, the value specified in  count, or the file offset is
not suitably aligned.

We need filter out O_DIRECT flag before read.

Signed-off-by: Ping Fang <pifang@redhat.com>
---
 testcases/kernel/io/ltp-aiodio/aiocp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Cyril Hrubis Feb. 9, 2023, 11:06 a.m. UTC | #1
Hi!
> When aiocp executed with -f DIRECT will fail.
> 
> <<<test_start>>>
> tag=AD049 stime=1675520824
> cmdline="aiocp -b 8k -n 8 -f DIRECT"
> contacts=""
> analysis=exit
> <<<test_output>>>
> tst_test.c:1560: TINFO: Timeout per run is 0h 30m 30s
> aiocp.c:211: TINFO: Maximum AIO blocks: 65536
> tst_device.c:585: TINFO: Use uevent strategy
> aiocp.c:250: TINFO: Fill srcfile.bin with random data
> aiocp.c:279: TINFO: Copy srcfile.bin -> dstfile.bin
> aiocp.c:291: TINFO: Comparing srcfile.bin with dstfile.bin
> aiocp.c:306: TBROK: read(3,0x7ffcd743abe0,4096) failed, returned -1: EINVAL (22)
> ...
> 
> syscall read manual ERROR section said that:
> EINVAL fd  is  attached to an object which is unsuitable for reading;
> or the file was opened with the O_DIRECT flag, and either the address
> specified in buf, the value specified in  count, or the file offset is
> not suitably aligned.
> 
> We need filter out O_DIRECT flag before read.

This is not very good changelog, I had to look closely at the source to
figure out why we may need this.

Better description should say that the code which checks that the data
has been written correctly does not use aligned buffers, which may cause
a failure like the one above.

> Signed-off-by: Ping Fang <pifang@redhat.com>
> ---
>  testcases/kernel/io/ltp-aiodio/aiocp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/io/ltp-aiodio/aiocp.c b/testcases/kernel/io/ltp-aiodio/aiocp.c
> index bc0e209b2..e4252d641 100644
> --- a/testcases/kernel/io/ltp-aiodio/aiocp.c
> +++ b/testcases/kernel/io/ltp-aiodio/aiocp.c
> @@ -297,8 +297,8 @@ static void run(void)
>  		return;
>  	}
>  
> -	srcfd = SAFE_OPEN(srcname, srcflags | O_RDONLY, 0666);
> -	dstfd = SAFE_OPEN(dstname, srcflags | O_RDONLY, 0666);
> +	srcfd = SAFE_OPEN(srcname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
> +	dstfd = SAFE_OPEN(dstname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);

I guess that we can as well just remove the srcflags and keep just the
O_RDONLY since the srcflags are by definition either O_RDONLY or
O_DIRECT | O_RDONLY.

I suppose that using scrflags and dstflags for anything else than the
filedescriptors passed to the async_run() is actually a mistake.

>  	reads = howmany(filesize, buffsize);
>  
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
Andrea Cervesato Feb. 9, 2023, 11:24 a.m. UTC | #2
Hi,

On 2/9/23 12:06, Cyril Hrubis wrote:
> Hi!
>> When aiocp executed with -f DIRECT will fail.
>>
>> <<<test_start>>>
>> tag=AD049 stime=1675520824
>> cmdline="aiocp -b 8k -n 8 -f DIRECT"
>> contacts=""
>> analysis=exit
>> <<<test_output>>>
>> tst_test.c:1560: TINFO: Timeout per run is 0h 30m 30s
>> aiocp.c:211: TINFO: Maximum AIO blocks: 65536
>> tst_device.c:585: TINFO: Use uevent strategy
>> aiocp.c:250: TINFO: Fill srcfile.bin with random data
>> aiocp.c:279: TINFO: Copy srcfile.bin -> dstfile.bin
>> aiocp.c:291: TINFO: Comparing srcfile.bin with dstfile.bin
>> aiocp.c:306: TBROK: read(3,0x7ffcd743abe0,4096) failed, returned -1: EINVAL (22)
>> ...
>>
>> syscall read manual ERROR section said that:
>> EINVAL fd  is  attached to an object which is unsuitable for reading;
>> or the file was opened with the O_DIRECT flag, and either the address
>> specified in buf, the value specified in  count, or the file offset is
>> not suitably aligned.
>>
>> We need filter out O_DIRECT flag before read.
> This is not very good changelog, I had to look closely at the source to
> figure out why we may need this.
>
> Better description should say that the code which checks that the data
> has been written correctly does not use aligned buffers, which may cause
> a failure like the one above.
>
>> Signed-off-by: Ping Fang <pifang@redhat.com>
>> ---
>>   testcases/kernel/io/ltp-aiodio/aiocp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/testcases/kernel/io/ltp-aiodio/aiocp.c b/testcases/kernel/io/ltp-aiodio/aiocp.c
>> index bc0e209b2..e4252d641 100644
>> --- a/testcases/kernel/io/ltp-aiodio/aiocp.c
>> +++ b/testcases/kernel/io/ltp-aiodio/aiocp.c
>> @@ -297,8 +297,8 @@ static void run(void)
>>   		return;
>>   	}
>>   
>> -	srcfd = SAFE_OPEN(srcname, srcflags | O_RDONLY, 0666);
>> -	dstfd = SAFE_OPEN(dstname, srcflags | O_RDONLY, 0666);
>> +	srcfd = SAFE_OPEN(srcname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
>> +	dstfd = SAFE_OPEN(dstname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
> I guess that we can as well just remove the srcflags and keep just the
> O_RDONLY since the srcflags are by definition either O_RDONLY or
> O_DIRECT | O_RDONLY.
>
> I suppose that using scrflags and dstflags for anything else than the
> filedescriptors passed to the async_run() is actually a mistake.
>
I agree with this. Probably we only need:

     srcfd = SAFE_OPEN(srcname, O_RDONLY, 0666);
     dstfd = SAFE_OPEN(dstname, O_RDONLY, 0666);

>>   	reads = howmany(filesize, buffsize);
>>   
>> -- 
>> 2.31.1
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

Regards,
Andrea Cervesato
Jan Stancek Feb. 9, 2023, 1:39 p.m. UTC | #3
On Thu, Feb 9, 2023 at 12:05 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > When aiocp executed with -f DIRECT will fail.
> >
> > <<<test_start>>>
> > tag=AD049 stime=1675520824
> > cmdline="aiocp -b 8k -n 8 -f DIRECT"
> > contacts=""
> > analysis=exit
> > <<<test_output>>>
> > tst_test.c:1560: TINFO: Timeout per run is 0h 30m 30s
> > aiocp.c:211: TINFO: Maximum AIO blocks: 65536
> > tst_device.c:585: TINFO: Use uevent strategy
> > aiocp.c:250: TINFO: Fill srcfile.bin with random data
> > aiocp.c:279: TINFO: Copy srcfile.bin -> dstfile.bin
> > aiocp.c:291: TINFO: Comparing srcfile.bin with dstfile.bin
> > aiocp.c:306: TBROK: read(3,0x7ffcd743abe0,4096) failed, returned -1: EINVAL (22)
> > ...
> >
> > syscall read manual ERROR section said that:
> > EINVAL fd  is  attached to an object which is unsuitable for reading;
> > or the file was opened with the O_DIRECT flag, and either the address
> > specified in buf, the value specified in  count, or the file offset is
> > not suitably aligned.
> >
> > We need filter out O_DIRECT flag before read.
>
> This is not very good changelog, I had to look closely at the source to
> figure out why we may need this.
>
> Better description should say that the code which checks that the data
> has been written correctly does not use aligned buffers, which may cause
> a failure like the one above.

+1, I also found it confusing (do we pass DIRECT as parameter just to
ignore it?)

>
> > Signed-off-by: Ping Fang <pifang@redhat.com>
> > ---
> >  testcases/kernel/io/ltp-aiodio/aiocp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/testcases/kernel/io/ltp-aiodio/aiocp.c b/testcases/kernel/io/ltp-aiodio/aiocp.c
> > index bc0e209b2..e4252d641 100644
> > --- a/testcases/kernel/io/ltp-aiodio/aiocp.c
> > +++ b/testcases/kernel/io/ltp-aiodio/aiocp.c
> > @@ -297,8 +297,8 @@ static void run(void)
> >               return;
> >       }
> >
> > -     srcfd = SAFE_OPEN(srcname, srcflags | O_RDONLY, 0666);
> > -     dstfd = SAFE_OPEN(dstname, srcflags | O_RDONLY, 0666);
> > +     srcfd = SAFE_OPEN(srcname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
> > +     dstfd = SAFE_OPEN(dstname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
>
> I guess that we can as well just remove the srcflags and keep just the
> O_RDONLY since the srcflags are by definition either O_RDONLY or
> O_DIRECT | O_RDONLY.
>
> I suppose that using scrflags and dstflags for anything else than the
> filedescriptors passed to the async_run() is actually a mistake.
>
> >       reads = howmany(filesize, buffsize);
> >
> > --
> > 2.31.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
Li Wang Feb. 10, 2023, 5:26 a.m. UTC | #4
On Thu, Feb 9, 2023 at 9:39 PM Jan Stancek <jstancek@redhat.com> wrote:

> On Thu, Feb 9, 2023 at 12:05 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> >
> > Hi!
> > > When aiocp executed with -f DIRECT will fail.
> > >
> > > <<<test_start>>>
> > > tag=AD049 stime=1675520824
> > > cmdline="aiocp -b 8k -n 8 -f DIRECT"
> > > contacts=""
> > > analysis=exit
> > > <<<test_output>>>
> > > tst_test.c:1560: TINFO: Timeout per run is 0h 30m 30s
> > > aiocp.c:211: TINFO: Maximum AIO blocks: 65536
> > > tst_device.c:585: TINFO: Use uevent strategy
> > > aiocp.c:250: TINFO: Fill srcfile.bin with random data
> > > aiocp.c:279: TINFO: Copy srcfile.bin -> dstfile.bin
> > > aiocp.c:291: TINFO: Comparing srcfile.bin with dstfile.bin
> > > aiocp.c:306: TBROK: read(3,0x7ffcd743abe0,4096) failed, returned -1:
> EINVAL (22)
> > > ...
> > >
> > > syscall read manual ERROR section said that:
> > > EINVAL fd  is  attached to an object which is unsuitable for reading;
> > > or the file was opened with the O_DIRECT flag, and either the address
> > > specified in buf, the value specified in  count, or the file offset is
> > > not suitably aligned.
> > >
> > > We need filter out O_DIRECT flag before read.
> >
> > This is not very good changelog, I had to look closely at the source to
> > figure out why we may need this.
> >
> > Better description should say that the code which checks that the data
> > has been written correctly does not use aligned buffers, which may cause
> > a failure like the one above.
>
> +1, I also found it confusing (do we pass DIRECT as parameter just to
> ignore it?)
>

The data has been written successfully with DIRECT, the failure
happens when reading it again with DIRECT flag.



>
> >
> > > Signed-off-by: Ping Fang <pifang@redhat.com>
> > > ---
> > >  testcases/kernel/io/ltp-aiodio/aiocp.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/testcases/kernel/io/ltp-aiodio/aiocp.c
> b/testcases/kernel/io/ltp-aiodio/aiocp.c
> > > index bc0e209b2..e4252d641 100644
> > > --- a/testcases/kernel/io/ltp-aiodio/aiocp.c
> > > +++ b/testcases/kernel/io/ltp-aiodio/aiocp.c
> > > @@ -297,8 +297,8 @@ static void run(void)
> > >               return;
> > >       }
> > >
> > > -     srcfd = SAFE_OPEN(srcname, srcflags | O_RDONLY, 0666);
> > > -     dstfd = SAFE_OPEN(dstname, srcflags | O_RDONLY, 0666);
> > > +     srcfd = SAFE_OPEN(srcname, (srcflags & ~O_DIRECT) | O_RDONLY,
> 0666);
> > > +     dstfd = SAFE_OPEN(dstname, (srcflags & ~O_DIRECT) | O_RDONLY,
> 0666);
> >
> > I guess that we can as well just remove the srcflags and keep just the
> > O_RDONLY since the srcflags are by definition either O_RDONLY or
> > O_DIRECT | O_RDONLY.
> >
> > I suppose that using scrflags and dstflags for anything else than the
> > filedescriptors passed to the async_run() is actually a mistake.
> >
> > >       reads = howmany(filesize, buffsize);
> > >
> > > --
> > > 2.31.1
> > >
> > >
> > > --
> > > Mailing list info: https://lists.linux.it/listinfo/ltp
> >
> > --
> > Cyril Hrubis
> > chrubis@suse.cz
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> >
>
>
diff mbox series

Patch

diff --git a/testcases/kernel/io/ltp-aiodio/aiocp.c b/testcases/kernel/io/ltp-aiodio/aiocp.c
index bc0e209b2..e4252d641 100644
--- a/testcases/kernel/io/ltp-aiodio/aiocp.c
+++ b/testcases/kernel/io/ltp-aiodio/aiocp.c
@@ -297,8 +297,8 @@  static void run(void)
 		return;
 	}
 
-	srcfd = SAFE_OPEN(srcname, srcflags | O_RDONLY, 0666);
-	dstfd = SAFE_OPEN(dstname, srcflags | O_RDONLY, 0666);
+	srcfd = SAFE_OPEN(srcname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
+	dstfd = SAFE_OPEN(dstname, (srcflags & ~O_DIRECT) | O_RDONLY, 0666);
 
 	reads = howmany(filesize, buffsize);