Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] loop: change queue block size to match when using DIO.
@ 2019-08-28 10:32 Martijn Coenen
  2019-08-30 15:50 ` Christoph Hellwig
  2019-09-04 19:49 ` [PATCH v2] " Martijn Coenen
  0 siblings, 2 replies; 5+ messages in thread
From: Martijn Coenen @ 2019-08-28 10:32 UTC (permalink / raw)
  To: axboe
  Cc: linux-block, linux-kernel, gregkh, kernel-team, narayan,
	dariofreni, ioffe, jiyong, maco, Martijn Coenen

The loop driver assumes that if the passed in fd is opened with
O_DIRECT, the caller wants to use direct I/O on the loop device.
However, if the underlying filesystem has a different block size than
the loop block queue, direct I/O can't be enabled. Instead of requiring
userspace to manually change the blocksize and re-enable direct I/O,
just change the queue block size to match.

Signed-off-by: Martijn Coenen <maco@android.com>
---
 drivers/block/loop.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ab7ca5989097a..1138162ff6c4d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -994,6 +994,12 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op->fsync)
 		blk_queue_write_cache(lo->lo_queue, true, false);
 
+	if (io_is_direct(lo->lo_backing_file) && inode->i_sb->s_bdev) {
+		/* In case of direct I/O, match underlying block size */
+		blk_queue_logical_block_size(lo->lo_queue,
+				bdev_logical_block_size(inode->i_sb->s_bdev));
+	}
+
 	loop_update_rotational(lo);
 	loop_update_dio(lo);
 	set_capacity(lo->lo_disk, size);
-- 
2.23.0.187.g17f5b7556c-goog


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] loop: change queue block size to match when using DIO.
  2019-08-28 10:32 [PATCH] loop: change queue block size to match when using DIO Martijn Coenen
@ 2019-08-30 15:50 ` Christoph Hellwig
  2019-09-02 19:37   ` Martijn Coenen
  2019-09-04 19:49 ` [PATCH v2] " Martijn Coenen
  1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2019-08-30 15:50 UTC (permalink / raw)
  To: Martijn Coenen
  Cc: axboe, linux-block, linux-kernel, gregkh, kernel-team, narayan,
	dariofreni, ioffe, jiyong, maco

On Wed, Aug 28, 2019 at 12:32:29PM +0200, Martijn Coenen wrote:
> The loop driver assumes that if the passed in fd is opened with
> O_DIRECT, the caller wants to use direct I/O on the loop device.
> However, if the underlying filesystem has a different block size than
> the loop block queue, direct I/O can't be enabled. Instead of requiring
> userspace to manually change the blocksize and re-enable direct I/O,
> just change the queue block size to match.

Why can't we enable the block device in that case?  All the usual
block filesystems support 512 byte aligned direct I/O with a 4k
file system block size (as long as the underlying block device
sector size is also 512 bytes).

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] loop: change queue block size to match when using DIO.
  2019-08-30 15:50 ` Christoph Hellwig
@ 2019-09-02 19:37   ` Martijn Coenen
  0 siblings, 0 replies; 5+ messages in thread
From: Martijn Coenen @ 2019-09-02 19:37 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, linux-block, LKML, kernel-team, Narayan Kamath,
	Dario Freni, Nikita Ioffe, Jiyong Park, Martijn Coenen, Greg KH

On Fri, Aug 30, 2019 at 5:50 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Aug 28, 2019 at 12:32:29PM +0200, Martijn Coenen wrote:
> > The loop driver assumes that if the passed in fd is opened with
> > O_DIRECT, the caller wants to use direct I/O on the loop device.
> > However, if the underlying filesystem has a different block size than
> > the loop block queue, direct I/O can't be enabled. Instead of requiring
> > userspace to manually change the blocksize and re-enable direct I/O,
> > just change the queue block size to match.
>
> Why can't we enable the block device in that case?  All the usual
> block filesystems support 512 byte aligned direct I/O with a 4k
> file system block size (as long as the underlying block device
> sector size is also 512 bytes).

Sorry, I didn't word that correctly: it's not the logical block size
of the filesystem, but the logical block size of the underlying block
device that loop's queue must match (or exceed). With the current loop
code, if the backing file is opened with O_DIRECT and resides on a
block device with a 512 bytes logical block size, the loop device will
correctly use direct I/O. If instead the backing file happened to
reside on a block device with a 4k logical block size, the loop device
would silently fall back to cached mode. I think there's a benefit in
the behavior being consistent independent of the block size of the
backing device, and I don't see a good reason for not automatically
switching loop's logical/physical queue sizes to match the backing
device in this specific case.

Will send a v2.

Thanks,
Martijn

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] loop: change queue block size to match when using DIO.
  2019-08-28 10:32 [PATCH] loop: change queue block size to match when using DIO Martijn Coenen
  2019-08-30 15:50 ` Christoph Hellwig
@ 2019-09-04 19:49 ` " Martijn Coenen
  2019-09-10 10:12   ` Martijn Coenen
  1 sibling, 1 reply; 5+ messages in thread
From: Martijn Coenen @ 2019-09-04 19:49 UTC (permalink / raw)
  To: axboe, hch, ming.lei
  Cc: linux-block, linux-kernel, gregkh, kernel-team, narayan,
	dariofreni, ioffe, jiyong, maco, Martijn Coenen

The loop driver assumes that if the passed in fd is opened with
O_DIRECT, the caller wants to use direct I/O on the loop device.
However, if the underlying block device has a different block size than
the loop block queue, direct I/O can't be enabled. Instead of requiring
userspace to manually change the blocksize and re-enable direct I/O,
just change the queue block sizes to match, as well as the io_min size.

Signed-off-by: Martijn Coenen <maco@android.com>
---
v2 changes:
- Fixed commit message to say the block size must match the underlying
  block device, not the underlying filesystem.
- Also set physical blocksize and minimal io size correspondingly.

 drivers/block/loop.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ab7ca5989097a..b547182037af2 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -994,6 +994,16 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op->fsync)
 		blk_queue_write_cache(lo->lo_queue, true, false);
 
+	if (io_is_direct(lo->lo_backing_file) && inode->i_sb->s_bdev) {
+		/* In case of direct I/O, match underlying block size */
+		unsigned short bsize = bdev_logical_block_size(
+			inode->i_sb->s_bdev);
+
+		blk_queue_logical_block_size(lo->lo_queue, bsize);
+		blk_queue_physical_block_size(lo->lo_queue, bsize);
+		blk_queue_io_min(lo->lo_queue, bsize);
+	}
+
 	loop_update_rotational(lo);
 	loop_update_dio(lo);
 	set_capacity(lo->lo_disk, size);
-- 
2.23.0.187.g17f5b7556c-goog


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] loop: change queue block size to match when using DIO.
  2019-09-04 19:49 ` [PATCH v2] " Martijn Coenen
@ 2019-09-10 10:12   ` Martijn Coenen
  0 siblings, 0 replies; 5+ messages in thread
From: Martijn Coenen @ 2019-09-10 10:12 UTC (permalink / raw)
  To: Jens Axboe, Christoph Hellwig, Ming Lei
  Cc: linux-block, LKML, Greg KH, kernel-team, Narayan Kamath,
	Dario Freni, Nikita Ioffe, Jiyong Park, Martijn Coenen

Hi Jens, Ming,

Do you have any thoughts about this patch?

Thanks,
Martijn

On Wed, Sep 4, 2019 at 9:49 PM Martijn Coenen <maco@android.com> wrote:
>
> The loop driver assumes that if the passed in fd is opened with
> O_DIRECT, the caller wants to use direct I/O on the loop device.
> However, if the underlying block device has a different block size than
> the loop block queue, direct I/O can't be enabled. Instead of requiring
> userspace to manually change the blocksize and re-enable direct I/O,
> just change the queue block sizes to match, as well as the io_min size.
>
> Signed-off-by: Martijn Coenen <maco@android.com>
> ---
> v2 changes:
> - Fixed commit message to say the block size must match the underlying
>   block device, not the underlying filesystem.
> - Also set physical blocksize and minimal io size correspondingly.
>
>  drivers/block/loop.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ab7ca5989097a..b547182037af2 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -994,6 +994,16 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>         if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op->fsync)
>                 blk_queue_write_cache(lo->lo_queue, true, false);
>
> +       if (io_is_direct(lo->lo_backing_file) && inode->i_sb->s_bdev) {
> +               /* In case of direct I/O, match underlying block size */
> +               unsigned short bsize = bdev_logical_block_size(
> +                       inode->i_sb->s_bdev);
> +
> +               blk_queue_logical_block_size(lo->lo_queue, bsize);
> +               blk_queue_physical_block_size(lo->lo_queue, bsize);
> +               blk_queue_io_min(lo->lo_queue, bsize);
> +       }
> +
>         loop_update_rotational(lo);
>         loop_update_dio(lo);
>         set_capacity(lo->lo_disk, size);
> --
> 2.23.0.187.g17f5b7556c-goog
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-28 10:32 [PATCH] loop: change queue block size to match when using DIO Martijn Coenen
2019-08-30 15:50 ` Christoph Hellwig
2019-09-02 19:37   ` Martijn Coenen
2019-09-04 19:49 ` [PATCH v2] " Martijn Coenen
2019-09-10 10:12   ` Martijn Coenen

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox