diff mbox series

[v3,2/2] syscalls/ioctl_loop05: Using LOOP_CONFIGURE to set direct io

Message ID 1595556357-29932-2-git-send-email-xuyang2018.jy@cn.fujitsu.com
State New
Headers show
Series [v3,1/2] syscalls/ioctl_loop06: Using LOOP_CONFIGURE to test invalid block size | expand

Commit Message

Yang Xu July 24, 2020, 2:05 a.m. UTC
Since kernel commit 3448914e8cc5("loop: Add LOOP_CONFIGURE ioctl"),
it can explicitly request direct I/O mode by setting LO_FLAGS_DIRECT_IO
in loop_config.info.lo_flags.

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
v2->v3:
1.Use tst_detach_device_by_fd api
 .../kernel/syscalls/ioctl/ioctl_loop05.c      | 151 +++++++++++++-----
 1 file changed, 114 insertions(+), 37 deletions(-)

Comments

Cyril Hrubis July 29, 2020, 11:39 a.m. UTC | #1
Hi!
I started look at this patch however the first thing I've found out is that
our mountinfo parser is wrong. If you look at man 5 proc it says:

36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue
(1)(2)(3)   (4)   (5)      (6)      (7)   (8) (9)   (10)         (11)


(7)  optional fields: zero or more fields of the form
     "tag[:value]"; see below.

So we cannot really parse the information with a static scanf() string,
since the number of elements in the line is not constant.

And it does fail on some of the machines I do have here since there is
no optional fields present.

So I guess that we will have to write a parser that reads that
information line by line after all.
Cyril Hrubis July 29, 2020, 12:58 p.m. UTC | #2
Hi!
> +	/*
> +	 * When we call loop_configure ioctl successfully and detach it,
> +	 * the subquent loop_configure without non-zero lo_offset or
> +	 * sizelimit may trigger the blk_update_request I/O error.
> +	 * To avoid this, sleep 1s to ensure last blk_update_request has
> +	 * completed.
> +	 */
> +	sleep(1);

This sounds to me like a kernel bug. Have you tried asking kernel
developers if this is expected behavior before we attempt work around
the problem in tests?
Yang Xu July 30, 2020, 8:49 a.m. UTC | #3
HI Cyril

> Hi!
> I started look at this patch however the first thing I've found out is that
> our mountinfo parser is wrong. If you look at man 5 proc it says:
> 
> 36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue
> (1)(2)(3)   (4)   (5)      (6)      (7)   (8) (9)   (10)         (11)
> 
> 
> (7)  optional fields: zero or more fields of the form
>       "tag[:value]"; see below.
> 
> So we cannot really parse the information with a static scanf() string,
> since the number of elements in the line is not constant.
> 
> And it does fail on some of the machines I do have here since there is
> no optional fields present.
> 
> So I guess that we will have to write a parser that reads that
> information line by line after all.
I doubt how machies will have more or zero fields in (7). But I think 
you are right,
How about using the (3) field and second to last field. Then we can 
avoid zero or more filed in (7). the code as below:

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 8d8bc5b40..36d6666fe 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -497,16 +497,31 @@ unsigned long tst_dev_bytes_written(const char *dev)

  void tst_find_backing_dev(const char *path, char *dev)
  {
-       char fmt[1024];
+       char fmt[20];
         struct stat buf;
+       FILE *file;
+       char line[PATH_MAX];
+       char *pre = NULL;
+       char *next = NULL;

         if (stat(path, &buf) < 0)
                  tst_brkm(TWARN | TERRNO, NULL, "stat() failed");

-       snprintf(fmt, sizeof(fmt), "%%*i %%*i %u:%u %%*s %%*s %%*s %%*s 
%%*s %%*s %%s %%*s",
-                       major(buf.st_dev), minor(buf.st_dev));
+       snprintf(fmt, sizeof(fmt), "%u:%u", major(buf.st_dev), 
minor(buf.st_dev));
+       file = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r");

-       SAFE_FILE_LINES_SCANF(NULL, "/proc/self/mountinfo", fmt, dev);
+       while (fgets(line, sizeof(line), file)) {
+               if (strstr(line, fmt) != NULL) {
+                       pre = strtok_r(line, " ", &next);
+                       while(pre != NULL) {
+                               strcpy(dev, pre);
+                               pre = strtok_r(NULL, " ", &next);
+                               if (strlen(next) == 0)
+                                       break;
+                       }
+                       break;
+               }
+       }

         if (stat(dev, &buf) < 0)
                  tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);

>
Cyril Hrubis July 30, 2020, 9:28 a.m. UTC | #4
Hi!
> > So I guess that we will have to write a parser that reads that
> > information line by line after all.
> I doubt how machies will have more or zero fields in (7). But I think 
> you are right,

Well that's what I do have here.

> How about using the (3) field and second to last field. Then we can 
> avoid zero or more filed in (7). the code as below??

Actually looking into util-linux code it says that th the optional
fields are terminated with " - ", see:

https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/libmount/src/tab_parse.c#n177

So I guess the safest option would be:

* Match the line by major:minor as you do
* Then strstr() for " - " should land us directly to field (8)
Yang Xu July 30, 2020, 10:08 a.m. UTC | #5
HI Cyril
> Hi!
>>> So I guess that we will have to write a parser that reads that
>>> information line by line after all.
>> I doubt how machies will have more or zero fields in (7). But I think
>> you are right,
> 
> Well that's what I do have here.
> 
>> How about using the (3) field and second to last field. Then we can
>> avoid zero or more filed in (7). the code as below??
> 
> Actually looking into util-linux code it says that th the optional
> fields are terminated with " - ", see:
> 
> https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tree/libmount/src/tab_parse.c#n177
> 
> So I guess the safest option would be:
> 
> * Match the line by major:minor as you do
> * Then strstr() for " - " should land us directly to field (8)
Yes, using " - " more easily and faster. code as below:

index 8d8bc5b40..bdd93f19e 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -497,17 +497,30 @@ unsigned long tst_dev_bytes_written(const char *dev)

  void tst_find_backing_dev(const char *path, char *dev)
  {
-       char fmt[1024];
+       char fmt[20];
         struct stat buf;
+       FILE *file;
+       char line[PATH_MAX];
+       char *pre = NULL;
+       char *next = NULL;

         if (stat(path, &buf) < 0)
                  tst_brkm(TWARN | TERRNO, NULL, "stat() failed");

-       snprintf(fmt, sizeof(fmt), "%%*i %%*i %u:%u %%*s %%*s %%*s %%*s 
%%*s %%*s %%s %%*s",
-                       major(buf.st_dev), minor(buf.st_dev));
+       snprintf(fmt, sizeof(fmt), "%u:%u", major(buf.st_dev), 
minor(buf.st_dev));
+       file = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r");

-       SAFE_FILE_LINES_SCANF(NULL, "/proc/self/mountinfo", fmt, dev);
+       while (fgets(line, sizeof(line), file)) {
+               if (strstr(line, fmt) != NULL) {
+                       pre = strstr(line, " - ");
+                       pre = strtok_r(pre, " ", &next);
+                       pre = strtok_r(NULL, " ", &next);
+                       pre = strtok_r(NULL, " ", &next);
+                       strcpy(dev, pre);
+               }
+       }

+       SAFE_FCLOSE(NULL, file);
         if (stat(dev, &buf) < 0)
                  tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);


>
Cyril Hrubis July 30, 2020, 10:38 a.m. UTC | #6
Hi!
> index 8d8bc5b40..bdd93f19e 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -497,17 +497,30 @@ unsigned long tst_dev_bytes_written(const char *dev)
> 
>   void tst_find_backing_dev(const char *path, char *dev)
>   {
> -       char fmt[1024];
> +       char fmt[20];
>          struct stat buf;
> +       FILE *file;
> +       char line[PATH_MAX];
> +       char *pre = NULL;
> +       char *next = NULL;
> 
>          if (stat(path, &buf) < 0)
>                   tst_brkm(TWARN | TERRNO, NULL, "stat() failed");
> 
> -       snprintf(fmt, sizeof(fmt), "%%*i %%*i %u:%u %%*s %%*s %%*s %%*s 
> %%*s %%*s %%s %%*s",
> -                       major(buf.st_dev), minor(buf.st_dev));
> +       snprintf(fmt, sizeof(fmt), "%u:%u", major(buf.st_dev), 
> minor(buf.st_dev));
> +       file = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r");
> 
> -       SAFE_FILE_LINES_SCANF(NULL, "/proc/self/mountinfo", fmt, dev);
> +       while (fgets(line, sizeof(line), file)) {
> +               if (strstr(line, fmt) != NULL) {
> +                       pre = strstr(line, " - ");
> +                       pre = strtok_r(pre, " ", &next);
> +                       pre = strtok_r(NULL, " ", &next);
> +                       pre = strtok_r(NULL, " ", &next);
> +                       strcpy(dev, pre);

We should break; here as well, since we already found the result.

> +               }
> +       }
> 
> +       SAFE_FCLOSE(NULL, file);
>          if (stat(dev, &buf) < 0)
>                   tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);

Otherwise it looks good.
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
index e3c14faab..7491e62fa 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c
@@ -19,6 +19,9 @@ 
  * enabled but falls back to buffered I/O later on. This is the case at least
  * for Btrfs. Because of that the test passes both with failure as well as
  * success with non-zero offset.
+ *
+ * Also use LOOP_CONFIGURE to test this by setting LO_FLAGS_DIRECT_IO
+ * in loop_config.info.lo_flags.
  */
 
 #include <stdio.h>
@@ -32,8 +35,36 @@ 
 #define DIO_MESSAGE "In dio mode"
 #define NON_DIO_MESSAGE "In non dio mode"
 
-static char dev_path[1024], sys_loop_diopath[1024], backing_file_path[1024];;
+static char dev_path[1024], sys_loop_diopath[1024], backing_file_path[1024];
 static int dev_num, dev_fd, block_devfd, attach_flag, logical_block_size;
+static int file_fd, loop_configure_sup = 1;
+static struct loop_config loopconfig;
+static struct loop_info loopinfo;
+
+static struct tcase {
+	int multi; /*logical_block_size / 2 as unit */
+	int dio_value;
+	int ioctl_flag;
+	char *message;
+} tcases[] = {
+	{0, 1, LOOP_SET_DIRECT_IO,
+	"Using LOOP_SET_DIRET_IO without setting lo_offset or sizelimit"},
+
+	{2, 1, LOOP_SET_DIRECT_IO,
+	"Using LOOP_SET_DIRECT_IO With offset equal to logical_block_size"},
+
+	{1, 0, LOOP_SET_DIRECT_IO,
+	"Using LOOP_SET_DIRECT_IO with offset less than logical_block_size"},
+
+	{0, 1, LOOP_CONFIGURE,
+	"Using LOOP_CONFIGURE without setting lo_offset or sizelimit"},
+
+	{2, 1, LOOP_CONFIGURE,
+	"Using LOOP_CONFIGURE With offset equal to logical_block_size"},
+
+	{1, 0, LOOP_CONFIGURE,
+	"Using LOOP_CONFIGURE witg offset less than logical_block_size"},
+};
 
 static void check_dio_value(int flag)
 {
@@ -42,61 +73,91 @@  static void check_dio_value(int flag)
 	memset(&loopinfoget, 0, sizeof(loopinfoget));
 
 	SAFE_IOCTL(dev_fd, LOOP_GET_STATUS, &loopinfoget);
-	tst_res(TINFO, "%s", flag ? DIO_MESSAGE : NON_DIO_MESSAGE);
 
 	if (loopinfoget.lo_flags & LO_FLAGS_DIRECT_IO)
-		tst_res(flag ? TPASS : TFAIL, "lo_flags has LO_FLAGS_DIRECT_IO flag");
+		tst_res(flag ? TPASS : TFAIL,
+			"%s, lo_flags has LO_FLAGS_DIRECT_IO flag",
+			flag ? DIO_MESSAGE : NON_DIO_MESSAGE);
 	else
-		tst_res(flag ? TFAIL : TPASS, "lo_flags doesn't have LO_FLAGS_DIRECT_IO flag");
+		tst_res(flag ? TFAIL : TPASS,
+			"%s, lo_flags doesn't have LO_FLAGS_DIRECT_IO flag",
+			flag ? DIO_MESSAGE : NON_DIO_MESSAGE);
 
 	TST_ASSERT_INT(sys_loop_diopath, flag);
 }
 
-static void verify_ioctl_loop(void)
+static void verify_ioctl_loop(unsigned int n)
 {
-	struct loop_info loopinfo;
-
-	memset(&loopinfo, 0, sizeof(loopinfo));
-	TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), TST_RETVAL_EQ0);
+	if (tcases[n].ioctl_flag == LOOP_SET_DIRECT_IO) {
+		TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), TST_RETVAL_EQ0);
+
+		TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
+		if (TST_RET == 0) {
+			if (tcases[n].dio_value)
+				tst_res(TPASS, "set direct io succeeded");
+			else
+				tst_res(TPASS, "set direct io succeeded, offset is ignored");
+			check_dio_value(1);
+			SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
+			return;
+		}
+		if (TST_ERR == EINVAL && !tcases[n].dio_value)
+			tst_res(TPASS | TTERRNO,
+				"set direct io failed as expected");
+		else
+			tst_res(TFAIL | TTERRNO, "set direct io failed");
+		return;
+	}
+	/*
+	 * When we call loop_configure ioctl successfully and detach it,
+	 * the subquent loop_configure without non-zero lo_offset or
+	 * sizelimit may trigger the blk_update_request I/O error.
+	 * To avoid this, sleep 1s to ensure last blk_update_request has
+	 * completed.
+	 */
+	sleep(1);
+	/*
+	 * loop_cofigure calls loop_update_dio() function, it will ignore
+	 * the result of setting dio. It is different from loop_set_dio.
+	 */
+	TST_RETRY_FUNC(ioctl(dev_fd, LOOP_CONFIGURE, &loopconfig), TST_RETVAL_EQ0);
+	check_dio_value(tcases[n].dio_value);
+	TST_RETRY_FUNC(ioctl(dev_fd, LOOP_CLR_FD, 0), TST_RETVAL_EQ0);
+}
 
-	tst_res(TINFO, "Without setting lo_offset or sizelimit");
-	SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 1);
-	check_dio_value(1);
+static void run(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
 
-	SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
-	check_dio_value(0);
+	tst_res(TINFO, "%s", tc->message);
 
-	tst_res(TINFO, "With offset equal to logical_block_size");
-	loopinfo.lo_offset = logical_block_size;
-	TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), TST_RETVAL_EQ0);
-	TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
-	if (TST_RET == 0) {
-		tst_res(TPASS, "LOOP_SET_DIRECT_IO succeeded");
-		check_dio_value(1);
+	if (tc->ioctl_flag == LOOP_SET_DIRECT_IO) {
+		if (!attach_flag) {
+			tst_attach_device(dev_path, "test.img");
+			attach_flag = 1;
+		}
 		SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
-	} else {
-		tst_res(TFAIL | TTERRNO, "LOOP_SET_DIRECT_IO failed");
+		check_dio_value(0);
+		loopinfo.lo_offset = logical_block_size * tc->multi / 2;
+		verify_ioctl_loop(n);
+		return;
 	}
-
-	tst_res(TINFO, "With nonzero offset less than logical_block_size");
-	loopinfo.lo_offset = logical_block_size / 2;
-	TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_STATUS, &loopinfo), TST_RETVAL_EQ0);
-
-	TEST(ioctl(dev_fd, LOOP_SET_DIRECT_IO, 1));
-	if (TST_RET == 0) {
-		tst_res(TPASS, "LOOP_SET_DIRECT_IO succeeded, offset is ignored");
-		SAFE_IOCTL(dev_fd, LOOP_SET_DIRECT_IO, 0);
+	if (tc->ioctl_flag == LOOP_CONFIGURE && !loop_configure_sup) {
+		tst_res(TCONF, "LOOP_CONFIGURE ioctl not supported");
 		return;
 	}
-	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");
+	if (attach_flag) {
+		tst_detach_device_by_fd(dev_path, dev_fd);
+		attach_flag = 0;
+	}
+	loopconfig.info.lo_offset = logical_block_size * tc->multi / 2;
+	verify_ioctl_loop(n);
 }
 
 static void setup(void)
 {
 	char bd_path[100];
+	int ret;
 
 	if (tst_fs_type(".") == TST_TMPFS_MAGIC)
 		tst_brk(TCONF, "tmpfd doesn't support O_DIRECT flag");
@@ -128,8 +189,21 @@  static void setup(void)
 	SAFE_IOCTL(block_devfd, BLKSSZGET, &logical_block_size);
 	tst_res(TINFO, "backing dev(%s) logical_block_size is %d", bd_path, logical_block_size);
 	SAFE_CLOSE(block_devfd);
+
 	if (logical_block_size > 512)
 		TST_RETRY_FUNC(ioctl(dev_fd, LOOP_SET_BLOCK_SIZE, logical_block_size), TST_RETVAL_EQ0);
+
+	file_fd = SAFE_OPEN("test.img", O_RDWR);
+	loopconfig.fd = -1;
+	ret = ioctl(dev_fd, LOOP_CONFIGURE, &loopconfig);
+	if (ret && errno != EBADF) {
+		tst_res(TINFO | TERRNO, "LOOP_CONFIGURE is not supported");
+		loop_configure_sup = 0;
+		return;
+	}
+	loopconfig.block_size = logical_block_size;
+	loopconfig.fd = file_fd;
+	loopconfig.info.lo_flags = LO_FLAGS_DIRECT_IO;
 }
 
 static void cleanup(void)
@@ -138,6 +212,8 @@  static void cleanup(void)
 		SAFE_CLOSE(dev_fd);
 	if (block_devfd > 0)
 		SAFE_CLOSE(block_devfd);
+	if (file_fd > 0)
+		SAFE_CLOSE(file_fd);
 	if (attach_flag)
 		tst_detach_device(dev_path);
 }
@@ -145,7 +221,8 @@  static void cleanup(void)
 static struct tst_test test = {
 	.setup = setup,
 	.cleanup = cleanup,
-	.test_all = verify_ioctl_loop,
+	.test = run,
+	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
 	.needs_tmpdir = 1,
 	.needs_drivers = (const char *const []) {