[LTP] [PATCH] syscalls/ioctl_loop05: Do not fail on success

[LTP] [PATCH] syscalls/ioctl_loop05: Do not fail on success

[Cyril Hrubis]

The code in __loop_update_dio() uses inode->i_sb->s_bdev to get the
logical block size for the backing file for the loop device. If that
pointer is NULL, which happens to be the case for Btrfs, the checks for
alignment and block size are ignored and direct I/O can be turned on
regardless of the offset and logical block size.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/ioctl/ioctl_loop05.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
index 3a028ba2c..d26fa61bf 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
@@ -71,7 +71,7 @@ static void verify_ioctl_loop(void)
 
 	TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
 	if (TST_RET == 0) {
-		tst_res(TFAIL, "LOOP_SET_DIRECT_IO succeeded unexpectedly");
+		tst_res(TPASS, "LOOP_SET_DIRECT_IO succeeded");
 		SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
 		return;
 	}
-- 
2.24.1

[LTP] [PATCH] syscalls/ioctl_loop05: Do not fail on success

[Yang Xu]

Hi Cyril


> The code in __loop_update_dio() uses inode->i_sb->s_bdev to get the
> logical block size for the backing file for the loop device. If that
> pointer is NULL, which happens to be the case for Btrfs, the checks for
> alignment and block size are ignored and direct I/O can be turned on
> regardless of the offset and logical block size.Since kernel comment "the above condition may be loosed in the future, 
and direct I/O may be switched runtime at that time because most
of requests in sane applications should be PAGE_SIZE aligned". I think 
pass is ok, also print filesystem let user know this fs igores this 
align is better.

   loopinfo.lo_offset = 0;
   TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), 
TST_RETVAL_EQ0);

These should be moved to the beginning of test function.

Best Regards
Yang Xu
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>   testcases/kernel/syscalls/ioctl/ioctl_loop05.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
> index 3a028ba2c..d26fa61bf 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
> @@ -71,7 +71,7 @@ static void verify_ioctl_loop(void)
>   
>   	TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
>   	if (TST_RET == 0) {
> -		tst_res(TFAIL, "LOOP_SET_DIRECT_IO succeeded unexpectedly");
> +		tst_res(TPASS, "LOOP_SET_DIRECT_IO succeeded");
>   		SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
>   		return;
>   	}
> 

[LTP] [PATCH] syscalls/ioctl_loop05: Do not fail on success

[Cyril Hrubis]

Hi!
> > The code in __loop_update_dio() uses inode->i_sb->s_bdev to get the
> > logical block size for the backing file for the loop device. If that
> > pointer is NULL, which happens to be the case for Btrfs, the checks for
> > alignment and block size are ignored and direct I/O can be turned on
> > regardless of the offset and logical block size.Since kernel comment "the above condition may be loosed in the future, 
> and direct I/O may be switched runtime at that time because most
> of requests in sane applications should be PAGE_SIZE aligned". I think 
> pass is ok, also print filesystem let user know this fs igores this 
> align is better.

I do not get what you mean here. Should we change the TPASS message to
something "LOOP_SET_DIRECT_IO succeeded, offset is ignored" or something
similar?

>    loopinfo.lo_offset = 0;
>    TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), 
> TST_RETVAL_EQ0);
> 
> These should be moved to the beginning of test function.

I guess that we can do so, just to be extra sure, but we do zero the
loopinfo structure in lib/tst_device.c so we start with zero offset,
hence it does not matter if we reset it at the start or at the end of
the test.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/ioctl_loop05: Do not fail on success

[Yang Xu]

Hi Cyril


> Hi!
>>> The code in __loop_update_dio() uses inode->i_sb->s_bdev to get the
>>> logical block size for the backing file for the loop device. If that
>>> pointer is NULL, which happens to be the case for Btrfs, the checks for
>>> alignment and block size are ignored and direct I/O can be turned on
>>> regardless of the offset and logical block size.Since kernel comment "the above condition may be loosed in the future,
>> and direct I/O may be switched runtime at that time because most
>> of requests in sane applications should be PAGE_SIZE aligned". I think
>> pass is ok, also print filesystem let user know this fs igores this
>> align is better.
> 
> I do not get what you mean here. Should we change the TPASS message to
> something "LOOP_SET_DIRECT_IO succeeded, offset is ignored" or something
> similar?
Yes. Add a comment here is better.
> 
>>     loopinfo.lo_offset = 0;
>>     TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo),
>> TST_RETVAL_EQ0);
>>
>> These should be moved to the beginning of test function.
> 
> I guess that we can do so, just to be extra sure, but we do zero the
> loopinfo structure in lib/tst_device.c so we start with zero offset,
> hence it does not matter if we reset it at the start or at the end of
> the test.
When I debug this case as below(early return, ext4 filesystem):
TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
         if (TST_RET == 0) {
                 tst_res(TFAIL, "LOOP_SET_DIRECT_IO succeeded 
unexpectedly");
                 SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
         }
	return;
this case will broke when using i parameter,
ioctl_loop05.c:50: BROK: ioctl(3,LOOP_SET_DIRECT_IO,...) failed: EINVAL (22)

It seems the last test affected the next test, so I think we should use 
goto instead of return. Also including two typo, updata->update, need->needs
--- a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
@@ -5,8 +5,8 @@
   *
   * This is a basic ioctl test about loopdevice.
   *
- * It is designed to test LOOP_SET_DIRECT_IO can updata a live
- * loop device dio mode. It need the backing file also supports
+ * It is designed to test LOOP_SET_DIRECT_IO can update a live
+ * loop device dio mode. It needs the backing file also supports
71,15 +71,16 @@ static void verify_ioctl_loop(void)

         TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
         if (TST_RET == 0) {
-               tst_res(TFAIL, "LOOP_SET_DIRECT_IO succeeded unexpectedly");
+               tst_res(TPASS, "Ignoring offset, LOOP_SET_DIRECT_IO 
succeeded");
                 SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
-               return;
+               goto reset_offset;
         }
         if (TST_ERR == EINVAL)
                 tst_res(TPASS | TTERRNO, "LOOP_SET_DIRECT_IO failed as 
expected");
         else
                 tst_res(TFAIL | TTERRNO, "LOOP_SET_DIRECT_IO failed 
expected EINVAL got");

+reset_offset:
         loopinfo.lo_offset = 0;
         TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), 
TST_RETVAL_EQ0);


Best Regards
Yang Xu
> 

[LTP] [PATCH] syscalls/ioctl_loop05: Do not fail on success

[Cyril Hrubis]

Hi!
> >>> The code in __loop_update_dio() uses inode->i_sb->s_bdev to get the
> >>> logical block size for the backing file for the loop device. If that
> >>> pointer is NULL, which happens to be the case for Btrfs, the checks for
> >>> alignment and block size are ignored and direct I/O can be turned on
> >>> regardless of the offset and logical block size.Since kernel comment "the above condition may be loosed in the future,
> >> and direct I/O may be switched runtime at that time because most
> >> of requests in sane applications should be PAGE_SIZE aligned". I think
> >> pass is ok, also print filesystem let user know this fs igores this
> >> align is better.
> > 
> > I do not get what you mean here. Should we change the TPASS message to
> > something "LOOP_SET_DIRECT_IO succeeded, offset is ignored" or something
> > similar?
> Yes. Add a comment here is better.

I've added a longer top level comment explaining why we pass the test
with both outcomes on non-zero offset and pushed the patch.

> > 
> >>     loopinfo.lo_offset = 0;
> >>     TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo),
> >> TST_RETVAL_EQ0);
> >>
> >> These should be moved to the beginning of test function.
> > 
> > I guess that we can do so, just to be extra sure, but we do zero the
> > loopinfo structure in lib/tst_device.c so we start with zero offset,
> > hence it does not matter if we reset it at the start or at the end of
> > the test.
> When I debug this case as below(early return, ext4 filesystem):
> TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
>          if (TST_RET == 0) {
>                  tst_res(TFAIL, "LOOP_SET_DIRECT_IO succeeded 
> unexpectedly");
>                  SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
>          }
> 	return;
> this case will broke when using i parameter,
> ioctl_loop05.c:50: BROK: ioctl(3,LOOP_SET_DIRECT_IO,...) failed: EINVAL (22)
> 
> It seems the last test affected the next test, so I think we should use 
> goto instead of return. Also including two typo, updata->update, need->needs

Let's keep the git history clean and fix that in a subsequent patch.

-- 
Cyril Hrubis
chrubis@suse.cz