[PATCH 0/2] losetup: support direct-IO to backing file

[PATCH 0/2] losetup: support direct-IO to backing file

[Ming Lei]

Hi,

The loop direct I/O patches[1] have been merged to linus kernel tree
and will be released in v4.4.

So the two patches provide interfaces for using direct IO/AIO
in losetup.

 include/loopdev.h   |  4 ++++
 lib/loopdev.c       | 40 ++++++++++++++++++++++++++++++++++++++++
 sys-utils/losetup.c | 30 +++++++++++++++++++++++++++---
 3 files changed, 71 insertions(+), 3 deletions(-)

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bc07c10a3603a5ab3ef01ba42b3d41f9ac63d1b6

Thanks,
Ming

[PATCH 1/2] losetup: support ioctl cmd of LOOP_SET_DIRECT_IO

[Ming Lei]

>From v4.4, linux kernel starts to support direct I/O and
AIO to backing file for loop driver, so allow losetup to
enable the feature by using LOOP_SET_DIRECT_IO ioctl cmd.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/loopdev.h   |  2 ++
 lib/loopdev.c       | 18 ++++++++++++++++++
 sys-utils/losetup.c | 24 +++++++++++++++++++++---
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/include/loopdev.h b/include/loopdev.h
index 573a569..9a7f6ba 100644
--- a/include/loopdev.h
+++ b/include/loopdev.h
@@ -23,6 +23,7 @@
 #define LOOP_GET_STATUS64	0x4C05
 /* #define LOOP_CHANGE_FD	0x4C06 */
 #define LOOP_SET_CAPACITY	0x4C07
+#define LOOP_SET_DIRECT_IO	0x4C08
 
 /* /dev/loop-control interface */
 #ifndef LOOP_CTL_ADD
@@ -164,6 +165,7 @@ extern int loopcxt_next(struct loopdev_cxt *lc);
 extern int loopcxt_setup_device(struct loopdev_cxt *lc);
 extern int loopcxt_delete_device(struct loopdev_cxt *lc);
 extern int loopcxt_set_capacity(struct loopdev_cxt *lc);
+extern int loopcxt_set_dio(struct loopdev_cxt *lc, unsigned long use_dio);
 
 int loopcxt_set_offset(struct loopdev_cxt *lc, uint64_t offset);
 int loopcxt_set_sizelimit(struct loopdev_cxt *lc, uint64_t sizelimit);
diff --git a/lib/loopdev.c b/lib/loopdev.c
index fe047cd..ff99dd4 100644
--- a/lib/loopdev.c
+++ b/lib/loopdev.c
@@ -1315,6 +1315,24 @@ int loopcxt_set_capacity(struct loopdev_cxt *lc)
 	return 0;
 }
 
+int loopcxt_set_dio(struct loopdev_cxt *lc, unsigned long use_dio)
+{
+	int fd = loopcxt_get_fd(lc);
+
+	if (fd < 0)
+		return -EINVAL;
+
+	/* Kernels prior to v4.4 don't support this ioctl */
+	if (ioctl(fd, LOOP_SET_DIRECT_IO, use_dio) < 0) {
+		int rc = -errno;
+		DBG(CXT, ul_debugobj(lc, "LOOP_SET_DIRECT_IO failed: %m"));
+		return rc;
+	}
+
+	DBG(CXT, ul_debugobj(lc, "direct io set"));
+	return 0;
+}
+
 int loopcxt_delete_device(struct loopdev_cxt *lc)
 {
 	int fd = loopcxt_get_fd(lc);
diff --git a/sys-utils/losetup.c b/sys-utils/losetup.c
index a68b222..8403621 100644
--- a/sys-utils/losetup.c
+++ b/sys-utils/losetup.c
@@ -35,6 +35,7 @@ enum {
 	A_SHOW_ONE,		/* print info about one device */
 	A_FIND_FREE,		/* find first unused */
 	A_SET_CAPACITY,		/* set device capacity */
+	A_SET_DIRECT_IO,	/* set accessing backing file by direct io */
 };
 
 enum {
@@ -393,6 +394,7 @@ static void usage(FILE *out)
 	fputs(_("     --sizelimit <num>         device is limited to <num> bytes of the file\n"), out);
 	fputs(_(" -P, --partscan                create a partitioned loop device\n"), out);
 	fputs(_(" -r, --read-only               set up a read-only loop device\n"), out);
+	fputs(_("     --direct-io               open backing file with O_DIRECT\n"), out);
 	fputs(_("     --show                    print device name after setup (with -f)\n"), out);
 	fputs(_(" -v, --verbose                 verbose mode\n"), out);
 
@@ -446,11 +448,13 @@ int main(int argc, char **argv)
 	int res = 0, showdev = 0, lo_flags = 0;
 	char *outarg = NULL;
 	int list = 0;
+	unsigned long use_dio = 0;
 
 	enum {
 		OPT_SIZELIMIT = CHAR_MAX + 1,
 		OPT_SHOW,
-		OPT_RAW
+		OPT_RAW,
+		OPT_DIO
 	};
 	static const struct option longopts[] = {
 		{ "all", 0, 0, 'a' },
@@ -468,6 +472,7 @@ int main(int argc, char **argv)
 		{ "sizelimit", 1, 0, OPT_SIZELIMIT },
 		{ "partscan", 0, 0, 'P' },
 		{ "read-only", 0, 0, 'r' },
+		{ "direct-io", 1, 0, OPT_DIO },
 		{ "raw", 0, 0, OPT_RAW },
 		{ "show", 0, 0, OPT_SHOW },
 		{ "verbose", 0, 0, 'v' },
@@ -557,6 +562,10 @@ int main(int argc, char **argv)
 		case OPT_SHOW:
 			showdev = 1;
 			break;
+		case OPT_DIO:
+			act = A_SET_DIRECT_IO;
+			use_dio = strtoul_or_err(optarg, _("failed to parse dio"));
+			break;
 		case 'v':
 			break;
 		case 'V':
@@ -606,11 +615,14 @@ int main(int argc, char **argv)
 		 */
 		act = A_SHOW;
 
-	if (!act && optind + 1 == argc) {
+	if ((!act || act == A_SET_DIRECT_IO) && optind + 1 == argc) {
 		/*
 		 * losetup [--list] <device>
+		 * OR
+		 * losetup --direct-io DIO <device>
 		 */
-		act = A_SHOW_ONE;
+		if (!act)
+			act = A_SHOW_ONE;
 		if (!is_loopdev(argv[optind]) ||
 		    loopcxt_set_device(&lc, argv[optind]))
 			err(EXIT_FAILURE, _("%s: failed to use device"),
@@ -747,6 +759,12 @@ int main(int argc, char **argv)
 			warn(_("%s: set capacity failed"),
 			        loopcxt_get_device(&lc));
 		break;
+	case A_SET_DIRECT_IO:
+		res = loopcxt_set_dio(&lc, use_dio);
+		if (res)
+			warn(_("%s: set direct io failed"),
+			        loopcxt_get_device(&lc));
+		break;
 	default:
 		usage(stderr);
 		break;
-- 
1.9.1

[PATCH 2/2] losetup: support list direct io

[Ming Lei]

So that user can see if DIO is set for current loop device.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/loopdev.h   |  2 ++
 lib/loopdev.c       | 22 ++++++++++++++++++++++
 sys-utils/losetup.c |  6 ++++++
 3 files changed, 30 insertions(+)

diff --git a/include/loopdev.h b/include/loopdev.h
index 9a7f6ba..18ca41f 100644
--- a/include/loopdev.h
+++ b/include/loopdev.h
@@ -40,6 +40,7 @@ enum {
 	LO_FLAGS_USE_AOPS   = 2,
 	LO_FLAGS_AUTOCLEAR  = 4,	/* kernel >= 2.6.25 */
 	LO_FLAGS_PARTSCAN   = 8,	/* kernel >= 3.2 */
+	LO_FLAGS_DIRECT_IO  = 16,	/* kernel >= 4.2 */
 };
 
 #define LO_NAME_SIZE	64
@@ -181,6 +182,7 @@ extern int loopcxt_get_encrypt_type(struct loopdev_cxt *lc, uint32_t *type);
 extern const char *loopcxt_get_crypt_name(struct loopdev_cxt *lc);
 extern int loopcxt_is_autoclear(struct loopdev_cxt *lc);
 extern int loopcxt_is_readonly(struct loopdev_cxt *lc);
+extern int loopcxt_is_dio(struct loopdev_cxt *lc);
 extern int loopcxt_is_partscan(struct loopdev_cxt *lc);
 extern int loopcxt_find_by_backing_file(struct loopdev_cxt *lc,
 				const char *filename,
diff --git a/lib/loopdev.c b/lib/loopdev.c
index ff99dd4..54c6200 100644
--- a/lib/loopdev.c
+++ b/lib/loopdev.c
@@ -955,6 +955,28 @@ int loopcxt_is_readonly(struct loopdev_cxt *lc)
 
 /*
  * @lc: context
+ *
+ * Returns: 1 if the dio flags is set.
+ */
+int loopcxt_is_dio(struct loopdev_cxt *lc)
+{
+	struct sysfs_cxt *sysfs = loopcxt_get_sysfs(lc);
+
+	if (sysfs) {
+		int fl;
+		if (sysfs_read_int(sysfs, "loop/dio", &fl) == 0)
+			return fl;
+	}
+	if (loopcxt_ioctl_enabled(lc)) {
+		struct loop_info64 *lo = loopcxt_get_info(lc);
+		if (lo)
+			return lo->lo_flags & LO_FLAGS_DIRECT_IO;
+	}
+	return 0;
+}
+
+/*
+ * @lc: context
  * @st: backing file stat or NULL
  * @backing_file: filename
  * @offset: offset
diff --git a/sys-utils/losetup.c b/sys-utils/losetup.c
index 8403621..f4c9213 100644
--- a/sys-utils/losetup.c
+++ b/sys-utils/losetup.c
@@ -49,6 +49,7 @@ enum {
 	COL_PARTSCAN,
 	COL_RO,
 	COL_SIZELIMIT,
+	COL_DIO,
 };
 
 /* basic output flags */
@@ -74,6 +75,7 @@ static struct colinfo infos[] = {
 	[COL_RO]          = { "RO",           1, SCOLS_FL_RIGHT, N_("read-only device")},
 	[COL_SIZELIMIT]   = { "SIZELIMIT",    5, SCOLS_FL_RIGHT, N_("size limit of the file in bytes")},
 	[COL_MAJMIN]      = { "MAJ:MIN",      3, 0, N_("loop device major:minor number")},
+	[COL_DIO]         = { "DIO",          1, SCOLS_FL_RIGHT, N_("access backing file with direct-io")},
 };
 
 static int columns[ARRAY_SIZE(infos) * 2] = {-1};
@@ -271,6 +273,9 @@ static int set_scols_data(struct loopdev_cxt *lc, struct libscols_line *ln)
 		case COL_RO:
 			p = loopcxt_is_readonly(lc) ? "1" : "0";
 			break;
+		case COL_DIO:
+			p = loopcxt_is_dio(lc) ? "1" : "0";
+			break;
 		case COL_PARTSCAN:
 			p = loopcxt_is_partscan(lc) ? "1" : "0";
 			break;
@@ -599,6 +604,7 @@ int main(int argc, char **argv)
 		columns[ncolumns++] = COL_AUTOCLR;
 		columns[ncolumns++] = COL_RO;
 		columns[ncolumns++] = COL_BACK_FILE;
+		columns[ncolumns++] = COL_DIO;
 	}
 
 	if (act == A_FIND_FREE && optind < argc) {
-- 
1.9.1

Re: [PATCH 0/2] losetup: support direct-IO to backing file

[Karel Zak]

On Sat, Nov 07, 2015 at 08:40:16AM +0800, Ming Lei wrote:
> The loop direct I/O patches[1] have been merged to linus kernel tree
> and will be released in v4.4.
> 
> So the two patches provide interfaces for using direct IO/AIO
> in losetup.

Thanks!

>  include/loopdev.h   |  4 ++++
>  lib/loopdev.c       | 40 ++++++++++++++++++++++++++++++++++++++++
>  sys-utils/losetup.c | 30 +++++++++++++++++++++++++++---
>  3 files changed, 71 insertions(+), 3 deletions(-)
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bc07c10a3603a5ab3ef01ba42b3d41f9ac63d1b6

If I read correctly the kernel code than LO_FLAGS_DIRECT_IO is not
possible to use when initialize loop device by LOOP_SET_FD ioctl. I
need extra LOOP_SET_DIRECT_IO ioctl. Why?

Why kernel function loop_set_fd() does not initialize lo->use_dio
according to "lo_flags & LO_FLAGS_DIRECT_IO" as we use for another
loopdev stuff? See for example LO_FLAGS_PARTSCAN. Would be possible 
to improve kernel a little bit?


Now you have to use two commands (ioctls):

 # losetup /dev/loop0 file.img
 # losetup --direct-io 1 /dev/loop0

It's fine to use --direct-io as toggle, but it would be nice to
support it also when initialize the device.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/2] losetup: support direct-IO to backing file

[Ming Lei]

On Mon, Nov 9, 2015 at 6:29 PM, Karel Zak <kzak@redhat.com> wrote:
> On Sat, Nov 07, 2015 at 08:40:16AM +0800, Ming Lei wrote:
>> The loop direct I/O patches[1] have been merged to linus kernel tree
>> and will be released in v4.4.
>>
>> So the two patches provide interfaces for using direct IO/AIO
>> in losetup.
>
> Thanks!
>
>>  include/loopdev.h   |  4 ++++
>>  lib/loopdev.c       | 40 ++++++++++++++++++++++++++++++++++++++++
>>  sys-utils/losetup.c | 30 +++++++++++++++++++++++++++---
>>  3 files changed, 71 insertions(+), 3 deletions(-)
>>
>> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bc07c10a3603a5ab3ef01ba42b3d41f9ac63d1b6
>
> If I read correctly the kernel code than LO_FLAGS_DIRECT_IO is not
> possible to use when initialize loop device by LOOP_SET_FD ioctl. I
> need extra LOOP_SET_DIRECT_IO ioctl. Why?
>
> Why kernel function loop_set_fd() does not initialize lo->use_dio
> according to "lo_flags & LO_FLAGS_DIRECT_IO" as we use for another
> loopdev stuff? See for example LO_FLAGS_PARTSCAN. Would be possible
> to improve kernel a little bit?

One case we need to support is 'mount -o loop', which is a bit different
with PARTSCAN.

>
>
> Now you have to use two commands (ioctls):
>
>  # losetup /dev/loop0 file.img
>  # losetup --direct-io 1 /dev/loop0
>
> It's fine to use --direct-io as toggle, but it would be nice to
> support it also when initialize the device.

OK, looks a good idea, but we still need to support the standalone
command for direct-io for the case of 'mount -o loop'.

>
>     Karel
>
>
> --
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com



-- 
Ming Lei

Re: [PATCH 0/2] losetup: support direct-IO to backing file

[Karel Zak]

On Mon, Nov 09, 2015 at 07:29:21PM +0800, Ming Lei wrote:
> > It's fine to use --direct-io as toggle, but it would be nice to
> > support it also when initialize the device.
> 
> OK, looks a good idea, but we still need to support the standalone
> command for direct-io for the case of 'mount -o loop'.

Sure, I understand this use-case. I have talked bout loop_set_fd(),
maybe all we need is:


diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 423f4ca..22642a0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -925,7 +925,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 
 	set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
 
-	lo->use_dio = false;
+	lo->use_dio = (lo_flags & LO_FLAGS_DIRECT_IO);
 	lo->lo_blocksize = lo_blocksize;
 	lo->lo_device = bdev;
 	lo->lo_flags = lo_flags;


then we can a little bit modify your losetup.c patches and support
--direct-io also for A_CREATE.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/2] losetup: support direct-IO to backing file

[Ming Lei]

On Mon, Nov 9, 2015 at 7:46 PM, Karel Zak <kzak@redhat.com> wrote:
> On Mon, Nov 09, 2015 at 07:29:21PM +0800, Ming Lei wrote:
>> > It's fine to use --direct-io as toggle, but it would be nice to
>> > support it also when initialize the device.
>>
>> OK, looks a good idea, but we still need to support the standalone
>> command for direct-io for the case of 'mount -o loop'.
>
> Sure, I understand this use-case. I have talked bout loop_set_fd(),
> maybe all we need is:
>
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 423f4ca..22642a0 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -925,7 +925,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>
>         set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
>
> -       lo->use_dio = false;
> +       lo->use_dio = (lo_flags & LO_FLAGS_DIRECT_IO);
>         lo->lo_blocksize = lo_blocksize;
>         lo->lo_device = bdev;
>         lo->lo_flags = lo_flags;

I don't think this change is doable because lo_flags is always started
as zero from loop_set_fd().

>
>
> then we can a little bit modify your losetup.c patches and support
> --direct-io also for A_CREATE.

That makes sense, and I will submit V1 to support creating one device
with direct io.

thanks,
Ming

>
>     Karel
>
> --
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Ming Lei

Re: [PATCH 0/2] losetup: support direct-IO to backing file

[Karel Zak]

On Tue, Nov 10, 2015 at 02:05:39PM +0800, Ming Lei wrote:
> On Mon, Nov 9, 2015 at 7:46 PM, Karel Zak <kzak@redhat.com> wrote:
> > On Mon, Nov 09, 2015 at 07:29:21PM +0800, Ming Lei wrote:
> >> > It's fine to use --direct-io as toggle, but it would be nice to
> >> > support it also when initialize the device.
> >>
> >> OK, looks a good idea, but we still need to support the standalone
> >> command for direct-io for the case of 'mount -o loop'.
> >
> > Sure, I understand this use-case. I have talked bout loop_set_fd(),
> > maybe all we need is:
> >
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 423f4ca..22642a0 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -925,7 +925,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
> >
> >         set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
> >
> > -       lo->use_dio = false;
> > +       lo->use_dio = (lo_flags & LO_FLAGS_DIRECT_IO);
> >         lo->lo_blocksize = lo_blocksize;
> >         lo->lo_device = bdev;
> >         lo->lo_flags = lo_flags;
> 
> I don't think this change is doable because lo_flags is always started
> as zero from loop_set_fd().

 Sorry, wrong function -- should be loop_set_status() where we can
 modify lo->lo_flags by info->lo_flags. It's already used for
 LO_FLAGS_AUTOCLEAR and LO_FLAGS_PARTSCAN.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/2] losetup: support direct-IO to backing file

[Ming Lei]

On Tue, Nov 10, 2015 at 7:40 PM, Karel Zak <kzak@redhat.com> wrote:
> On Tue, Nov 10, 2015 at 02:05:39PM +0800, Ming Lei wrote:
>> On Mon, Nov 9, 2015 at 7:46 PM, Karel Zak <kzak@redhat.com> wrote:
>> > On Mon, Nov 09, 2015 at 07:29:21PM +0800, Ming Lei wrote:
>> >> > It's fine to use --direct-io as toggle, but it would be nice to
>> >> > support it also when initialize the device.
>> >>
>> >> OK, looks a good idea, but we still need to support the standalone
>> >> command for direct-io for the case of 'mount -o loop'.
>> >
>> > Sure, I understand this use-case. I have talked bout loop_set_fd(),
>> > maybe all we need is:
>> >
>> >
>> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> > index 423f4ca..22642a0 100644
>> > --- a/drivers/block/loop.c
>> > +++ b/drivers/block/loop.c
>> > @@ -925,7 +925,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>> >
>> >         set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
>> >
>> > -       lo->use_dio = false;
>> > +       lo->use_dio = (lo_flags & LO_FLAGS_DIRECT_IO);
>> >         lo->lo_blocksize = lo_blocksize;
>> >         lo->lo_device = bdev;
>> >         lo->lo_flags = lo_flags;
>>
>> I don't think this change is doable because lo_flags is always started
>> as zero from loop_set_fd().
>
>  Sorry, wrong function -- should be loop_set_status() where we can
>  modify lo->lo_flags by info->lo_flags. It's already used for
>  LO_FLAGS_AUTOCLEAR and LO_FLAGS_PARTSCAN.

Yes, that might work, but I am opt to use LOOP_SET_DIRECT_IO cmd
because set_status is already too fat and it is always safe to use one
specific cmd for chaning direct-io.

Also there have been bugs in set_status() already and it is always difficult to
change things together/atomaticaly especially, for example:

     - changing lo->transfer may cause oops
     - partial change in case of failure from figuring size
     - for direct-io, it may fail too, so it may introduce more
trouble to set_status
     for handling the failure.

Thanks,
Ming Lei