[PATCH 0/4] loop: support different logical block sizes

[PATCH 0/4] loop: support different logical block sizes

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hi, everyone,

Here's another attempt at supporting different logical block sizes for
loop devices. First, some terminology:

 * Logical block size: the lowest possible block size that the storage
   device can address.
 * Physical block size: the lowest possible sector size that the
   hardware can operate on without reverting to read-modify-write
   operations. Always greater than or equal to the logical block size.

These are characteristics of the device itself tracked in the
request_queue. For loop devices, both are currently always 512 bytes.

struct block_device also has another block size, basically a "soft"
block size which is the preferred I/O size and can be changed from
userspace. For loop devices, this is PAGE_SIZE if the backing file is a
regular file or otherwise the soft block size of the backing block
device.

Patch 1 simplifies how the soft block size is set. I'm tempted to not
set it at all and use the default of PAGE_SIZE, but maybe someone is
depending on inheriting the soft block size from the backing block
device...

Patch 2 sets the physical block size of loop devices to a more
meaningful value for loop, PAGE_SIZE.

Patch 3 allows us to change the logical block size. It adds a new ioctl
instead of the previous approach (which extends LOOP_{GET,SET}_STATUS).
Hannes, I assume you had a specific reason for doing it the way you did,
care to explain?

Patch 4 is a cleanup.

This is based on my patch reverting the original block size support,
targeting 4.14. Thanks!

Omar Sandoval (4):
  loop: get rid of lo_blocksize
  loop: set physical block size to PAGE_SIZE
  loop: add ioctl for changing logical block size
  loop: fold loop_switch() into callers

 drivers/block/loop.c      | 112 ++++++++++++++++------------------------------
 drivers/block/loop.h      |   1 -
 include/uapi/linux/loop.h |   1 +
 3 files changed, 40 insertions(+), 74 deletions(-)

-- 
2.14.1

[PATCH 1/4] loop: get rid of lo_blocksize

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

This is only used for setting the soft block size on the struct
block_device once and then never used again.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 drivers/block/loop.c | 10 ++--------
 drivers/block/loop.h |  1 -
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f321b96405f5..54e091887199 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -573,8 +573,6 @@ static void do_loop_switch(struct loop_device *lo, struct switch_request *p)
 	mapping = file->f_mapping;
 	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
 	lo->lo_backing_file = file;
-	lo->lo_blocksize = S_ISBLK(mapping->host->i_mode) ?
-		mapping->host->i_bdev->bd_block_size : PAGE_SIZE;
 	lo->old_gfp_mask = mapping_gfp_mask(mapping);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
 	loop_update_dio(lo);
@@ -867,7 +865,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	struct file	*file, *f;
 	struct inode	*inode;
 	struct address_space *mapping;
-	unsigned lo_blocksize;
 	int		lo_flags = 0;
 	int		error;
 	loff_t		size;
@@ -911,9 +908,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	    !file->f_op->write_iter)
 		lo_flags |= LO_FLAGS_READ_ONLY;
 
-	lo_blocksize = S_ISBLK(inode->i_mode) ?
-		inode->i_bdev->bd_block_size : PAGE_SIZE;
-
 	error = -EFBIG;
 	size = get_loop_size(lo, file);
 	if ((loff_t)(sector_t)size != size)
@@ -927,7 +921,6 @@ 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->lo_blocksize = lo_blocksize;
 	lo->lo_device = bdev;
 	lo->lo_flags = lo_flags;
 	lo->lo_backing_file = file;
@@ -947,7 +940,8 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	/* let user-space know about the new size */
 	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 
-	set_blocksize(bdev, lo_blocksize);
+	set_blocksize(bdev, S_ISBLK(inode->i_mode) ?
+		      block_size(inode->i_bdev) : PAGE_SIZE);
 
 	lo->lo_state = Lo_bound;
 	if (part_shift)
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index fecd3f97ef8c..efe57189d01e 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -48,7 +48,6 @@ struct loop_device {
 
 	struct file *	lo_backing_file;
 	struct block_device *lo_device;
-	unsigned	lo_blocksize;
 	void		*key_data; 
 
 	gfp_t		old_gfp_mask;
-- 
2.14.1

[PATCH 2/4] loop: set physical block size to PAGE_SIZE

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The physical block size is "the lowest possible sector size that the
hardware can operate on without reverting to read-modify-write
operations" (from the comment on blk_queue_physical_block_size()). Since
loop does buffered I/O on the backing file by default, the RMW unit is a
page. This isn't the case for direct I/O mode, but let's keep it simple.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 drivers/block/loop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 54e091887199..1a5b4ecf54ec 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1764,6 +1764,8 @@ static int loop_add(struct loop_device **l, int i)
 	}
 	lo->lo_queue->queuedata = lo;
 
+	blk_queue_physical_block_size(lo->lo_queue, PAGE_SIZE);
+
 	/*
 	 * It doesn't make sense to enable merge because the I/O
 	 * submitted to backing file is handled page by page.
-- 
2.14.1

[PATCH 3/4] loop: add ioctl for changing logical block size

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

This is a different approach from the first attempt in f2c6df7dbf9a
("loop: support 4k physical blocksize"). Rather than extending
LOOP_{GET,SET}_STATUS, add a separate ioctl just for setting the block
size.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 drivers/block/loop.c      | 24 ++++++++++++++++++++++++
 include/uapi/linux/loop.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1a5b4ecf54ec..6b1d932028e3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1047,6 +1047,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
+	blk_queue_logical_block_size(lo->lo_queue, 512);
 	if (bdev) {
 		bdput(bdev);
 		invalidate_bdev(bdev);
@@ -1330,6 +1331,24 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
 	return error;
 }
 
+static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
+{
+	if (lo->lo_state != Lo_bound)
+		return -ENXIO;
+
+	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
+		return -EINVAL;
+
+	blk_mq_freeze_queue(lo->lo_queue);
+
+	blk_queue_logical_block_size(lo->lo_queue, arg);
+	loop_update_dio(lo);
+
+	blk_mq_unfreeze_queue(lo->lo_queue);
+
+	return 0;
+}
+
 static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	unsigned int cmd, unsigned long arg)
 {
@@ -1378,6 +1397,11 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
 			err = loop_set_dio(lo, arg);
 		break;
+	case LOOP_SET_BLOCK_SIZE:
+		err = -EPERM;
+		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
+			err = loop_set_block_size(lo, arg);
+		break;
 	default:
 		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
 	}
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index c8125ec1f4f2..23158dbe2424 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -88,6 +88,7 @@ struct loop_info64 {
 #define LOOP_CHANGE_FD		0x4C06
 #define LOOP_SET_CAPACITY	0x4C07
 #define LOOP_SET_DIRECT_IO	0x4C08
+#define LOOP_SET_BLOCK_SIZE	0x4C09
 
 /* /dev/loop-control interface */
 #define LOOP_CTL_ADD		0x4C80
-- 
2.14.1

[PATCH 4/4] loop: fold loop_switch() into callers

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The comments here are really outdated, and blk-mq made flushing much
simpler, so just fold the two cases into the callers.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 drivers/block/loop.c | 76 ++++++++--------------------------------------------
 1 file changed, 11 insertions(+), 65 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b1d932028e3..e60b4ac77577 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -546,72 +546,12 @@ static int do_req_filebacked(struct loop_device *lo, struct request *rq)
 	}
 }
 
-struct switch_request {
-	struct file *file;
-	struct completion wait;
-};
-
 static inline void loop_update_dio(struct loop_device *lo)
 {
 	__loop_update_dio(lo, io_is_direct(lo->lo_backing_file) |
 			lo->use_dio);
 }
 
-/*
- * Do the actual switch; called from the BIO completion routine
- */
-static void do_loop_switch(struct loop_device *lo, struct switch_request *p)
-{
-	struct file *file = p->file;
-	struct file *old_file = lo->lo_backing_file;
-	struct address_space *mapping;
-
-	/* if no new file, only flush of queued bios requested */
-	if (!file)
-		return;
-
-	mapping = file->f_mapping;
-	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
-	lo->lo_backing_file = file;
-	lo->old_gfp_mask = mapping_gfp_mask(mapping);
-	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
-	loop_update_dio(lo);
-}
-
-/*
- * loop_switch performs the hard work of switching a backing store.
- * First it needs to flush existing IO, it does this by sending a magic
- * BIO down the pipe. The completion of this BIO does the actual switch.
- */
-static int loop_switch(struct loop_device *lo, struct file *file)
-{
-	struct switch_request w;
-
-	w.file = file;
-
-	/* freeze queue and wait for completion of scheduled requests */
-	blk_mq_freeze_queue(lo->lo_queue);
-
-	/* do the switch action */
-	do_loop_switch(lo, &w);
-
-	/* unfreeze */
-	blk_mq_unfreeze_queue(lo->lo_queue);
-
-	return 0;
-}
-
-/*
- * Helper to flush the IOs in loop, but keeping loop thread running
- */
-static int loop_flush(struct loop_device *lo)
-{
-	/* loop not yet configured, no running thread, nothing to flush */
-	if (lo->lo_state != Lo_bound)
-		return 0;
-	return loop_switch(lo, NULL);
-}
-
 static void loop_reread_partitions(struct loop_device *lo,
 				   struct block_device *bdev)
 {
@@ -676,9 +616,14 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 		goto out_putf;
 
 	/* and ... switch */
-	error = loop_switch(lo, file);
-	if (error)
-		goto out_putf;
+	blk_mq_freeze_queue(lo->lo_queue);
+	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
+	lo->lo_backing_file = file;
+	lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
+	mapping_set_gfp_mask(file->f_mapping,
+			     lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
+	loop_update_dio(lo);
+	blk_mq_unfreeze_queue(lo->lo_queue);
 
 	fput(old_file);
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
@@ -1601,12 +1546,13 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 		err = loop_clr_fd(lo);
 		if (!err)
 			return;
-	} else {
+	} else if (lo->lo_state == Lo_bound) {
 		/*
 		 * Otherwise keep thread (if running) and config,
 		 * but flush possible ongoing bios in thread.
 		 */
-		loop_flush(lo);
+		blk_mq_freeze_queue(lo->lo_queue);
+		blk_mq_unfreeze_queue(lo->lo_queue);
 	}
 
 	mutex_unlock(&lo->lo_ctl_mutex);
-- 
2.14.1

Re: [PATCH 3/4] loop: add ioctl for changing logical block size

[Karel Zak]

On Thu, Aug 24, 2017 at 12:03:43AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This is a different approach from the first attempt in f2c6df7dbf9a
> ("loop: support 4k physical blocksize"). Rather than extending
> LOOP_{GET,SET}_STATUS, add a separate ioctl just for setting the block
> size.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c      | 24 ++++++++++++++++++++++++
>  include/uapi/linux/loop.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1a5b4ecf54ec..6b1d932028e3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1047,6 +1047,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
>  	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
>  	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
> +	blk_queue_logical_block_size(lo->lo_queue, 512);
>  	if (bdev) {
>  		bdput(bdev);
>  		invalidate_bdev(bdev);
> @@ -1330,6 +1331,24 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
>  	return error;
>  }
>  
> +static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
> +{
> +	if (lo->lo_state != Lo_bound)
> +		return -ENXIO;
> +
> +	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
> +		return -EINVAL;

I'm not sure if the PAGE_SIZE is the right limit for the use-case
where backing file is a block device ...but it's nothing important.

> +	blk_mq_freeze_queue(lo->lo_queue);
> +
> +	blk_queue_logical_block_size(lo->lo_queue, arg);

Just note that this function also updates physical_block_size if the
'arg' is greater than the current setting. That's good thing :-)

> +	loop_update_dio(lo);
> +
> +	blk_mq_unfreeze_queue(lo->lo_queue);
> +
> +	return 0;
> +}
> +
>  static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  	unsigned int cmd, unsigned long arg)
>  {
> @@ -1378,6 +1397,11 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
>  			err = loop_set_dio(lo, arg);
>  		break;
> +	case LOOP_SET_BLOCK_SIZE:
> +		err = -EPERM;
> +		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> +			err = loop_set_block_size(lo, arg);
> +		break;

I like it this ioctl idea. It makes loop based tests more comfortable
as we can change sector size on the fly without loop device reset.

I did not test it yet, but this implementation seems the best. Thanks.

    Karel

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

Re: [PATCH 1/4] loop: get rid of lo_blocksize

[Hannes Reinecke]

On 08/24/2017 09:03 AM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This is only used for setting the soft block size on the struct
> block_device once and then never used again.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c | 10 ++--------
>  drivers/block/loop.h |  1 -
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/4] loop: set physical block size to PAGE_SIZE

[Hannes Reinecke]

On 08/24/2017 09:03 AM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> The physical block size is "the lowest possible sector size that the
> hardware can operate on without reverting to read-modify-write
> operations" (from the comment on blk_queue_physical_block_size()). Since
> loop does buffered I/O on the backing file by default, the RMW unit is a
> page. This isn't the case for direct I/O mode, but let's keep it simple.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 54e091887199..1a5b4ecf54ec 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1764,6 +1764,8 @@ static int loop_add(struct loop_device **l, int i)
>  	}
>  	lo->lo_queue->queuedata = lo;
>  
> +	blk_queue_physical_block_size(lo->lo_queue, PAGE_SIZE);
> +
>  	/*
>  	 * It doesn't make sense to enable merge because the I/O
>  	 * submitted to backing file is handled page by page.
> 
Let's see it this one goes through ...

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/4] loop: add ioctl for changing logical block size

[Hannes Reinecke]

On 08/24/2017 09:03 AM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This is a different approach from the first attempt in f2c6df7dbf9a
> ("loop: support 4k physical blocksize"). Rather than extending
> LOOP_{GET,SET}_STATUS, add a separate ioctl just for setting the block
> size.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c      | 24 ++++++++++++++++++++++++
>  include/uapi/linux/loop.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1a5b4ecf54ec..6b1d932028e3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1047,6 +1047,7 @@ static int loop_clr_fd(struct loop_device *lo)
>  	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
>  	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
>  	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
> +	blk_queue_logical_block_size(lo->lo_queue, 512);
>  	if (bdev) {
>  		bdput(bdev);
>  		invalidate_bdev(bdev);
> @@ -1330,6 +1331,24 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
>  	return error;
>  }
>  
> +static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
> +{
> +	if (lo->lo_state != Lo_bound)
> +		return -ENXIO;
> +
> +	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
> +		return -EINVAL;
> +
> +	blk_mq_freeze_queue(lo->lo_queue);
> +
> +	blk_queue_logical_block_size(lo->lo_queue, arg);
> +	loop_update_dio(lo);
> +
> +	blk_mq_unfreeze_queue(lo->lo_queue);
> +
> +	return 0;
> +}
> +
>  static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  	unsigned int cmd, unsigned long arg)
>  {
> @@ -1378,6 +1397,11 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
>  			err = loop_set_dio(lo, arg);
>  		break;
> +	case LOOP_SET_BLOCK_SIZE:
> +		err = -EPERM;
> +		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> +			err = loop_set_block_size(lo, arg);
> +		break;
>  	default:
>  		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
>  	}
> diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
> index c8125ec1f4f2..23158dbe2424 100644
> --- a/include/uapi/linux/loop.h
> +++ b/include/uapi/linux/loop.h
> @@ -88,6 +88,7 @@ struct loop_info64 {
>  #define LOOP_CHANGE_FD		0x4C06
>  #define LOOP_SET_CAPACITY	0x4C07
>  #define LOOP_SET_DIRECT_IO	0x4C08
> +#define LOOP_SET_BLOCK_SIZE	0x4C09
>  
>  /* /dev/loop-control interface */
>  #define LOOP_CTL_ADD		0x4C80
> 
Hmm.

If the losetup / util-linux maintainers are on board with this, okay.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 4/4] loop: fold loop_switch() into callers

[Hannes Reinecke]

On 08/24/2017 09:03 AM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> The comments here are really outdated, and blk-mq made flushing much
> simpler, so just fold the two cases into the callers.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c | 76 ++++++++--------------------------------------------
>  1 file changed, 11 insertions(+), 65 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/4] loop: add ioctl for changing logical block size

[Omar Sandoval]

On Thu, Aug 24, 2017 at 10:06:57AM +0200, Karel Zak wrote:
> On Thu, Aug 24, 2017 at 12:03:43AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > This is a different approach from the first attempt in f2c6df7dbf9a
> > ("loop: support 4k physical blocksize"). Rather than extending
> > LOOP_{GET,SET}_STATUS, add a separate ioctl just for setting the block
> > size.
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  drivers/block/loop.c      | 24 ++++++++++++++++++++++++
> >  include/uapi/linux/loop.h |  1 +
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 1a5b4ecf54ec..6b1d932028e3 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1047,6 +1047,7 @@ static int loop_clr_fd(struct loop_device *lo)
> >  	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
> >  	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
> >  	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
> > +	blk_queue_logical_block_size(lo->lo_queue, 512);
> >  	if (bdev) {
> >  		bdput(bdev);
> >  		invalidate_bdev(bdev);
> > @@ -1330,6 +1331,24 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
> >  	return error;
> >  }
> >  
> > +static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
> > +{
> > +	if (lo->lo_state != Lo_bound)
> > +		return -ENXIO;
> > +
> > +	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
> > +		return -EINVAL;
> 
> I'm not sure if the PAGE_SIZE is the right limit for the use-case
> where backing file is a block device ...but it's nothing important.

A bigger logical block size just means we'll send bigger I/Os down to
the backing file, so I think it should be okay.

> > +	blk_mq_freeze_queue(lo->lo_queue);
> > +
> > +	blk_queue_logical_block_size(lo->lo_queue, arg);
> 
> Just note that this function also updates physical_block_size if the
> 'arg' is greater than the current setting. That's good thing :-)

Patch 2 sets the physical block size to PAGE_SIZE, and we cap the
logical block size to PAGE_SIZE, so we won't actually hit that case
anyways :)

> > +	loop_update_dio(lo);
> > +
> > +	blk_mq_unfreeze_queue(lo->lo_queue);
> > +
> > +	return 0;
> > +}
> > +
> >  static int lo_ioctl(struct block_device *bdev, fmode_t mode,
> >  	unsigned int cmd, unsigned long arg)
> >  {
> > @@ -1378,6 +1397,11 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
> >  		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> >  			err = loop_set_dio(lo, arg);
> >  		break;
> > +	case LOOP_SET_BLOCK_SIZE:
> > +		err = -EPERM;
> > +		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> > +			err = loop_set_block_size(lo, arg);
> > +		break;
> 
> I like it this ioctl idea. It makes loop based tests more comfortable
> as we can change sector size on the fly without loop device reset.
> 
> I did not test it yet, but this implementation seems the best. Thanks.

Thanks! Let me know if you spot any issues when you test it.

Re: [PATCH 0/4] loop: support different logical block sizes

[Ming Lei]

On Thu, Aug 24, 2017 at 12:03:40AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hi, everyone,
> 
> Here's another attempt at supporting different logical block sizes for
> loop devices. First, some terminology:
> 
>  * Logical block size: the lowest possible block size that the storage
>    device can address.
>  * Physical block size: the lowest possible sector size that the
>    hardware can operate on without reverting to read-modify-write
>    operations. Always greater than or equal to the logical block size.
> 
> These are characteristics of the device itself tracked in the
> request_queue. For loop devices, both are currently always 512 bytes.
> 
> struct block_device also has another block size, basically a "soft"
> block size which is the preferred I/O size and can be changed from
> userspace. For loop devices, this is PAGE_SIZE if the backing file is a
> regular file or otherwise the soft block size of the backing block
> device.
> 
> Patch 1 simplifies how the soft block size is set. I'm tempted to not
> set it at all and use the default of PAGE_SIZE, but maybe someone is
> depending on inheriting the soft block size from the backing block
> device...
> 
> Patch 2 sets the physical block size of loop devices to a more
> meaningful value for loop, PAGE_SIZE.
> 
> Patch 3 allows us to change the logical block size. It adds a new ioctl
> instead of the previous approach (which extends LOOP_{GET,SET}_STATUS).
> Hannes, I assume you had a specific reason for doing it the way you did,
> care to explain?
> 
> Patch 4 is a cleanup.
> 
> This is based on my patch reverting the original block size support,
> targeting 4.14. Thanks!
> 
> Omar Sandoval (4):
>   loop: get rid of lo_blocksize
>   loop: set physical block size to PAGE_SIZE
>   loop: add ioctl for changing logical block size
>   loop: fold loop_switch() into callers
> 
>  drivers/block/loop.c      | 112 ++++++++++++++++------------------------------
>  drivers/block/loop.h      |   1 -
>  include/uapi/linux/loop.h |   1 +
>  3 files changed, 40 insertions(+), 74 deletions(-)

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 0/4] loop: support different logical block sizes

[Jens Axboe]

On 08/24/2017 01:03 AM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hi, everyone,
> 
> Here's another attempt at supporting different logical block sizes for
> loop devices. First, some terminology:
> 
>  * Logical block size: the lowest possible block size that the storage
>    device can address.
>  * Physical block size: the lowest possible sector size that the
>    hardware can operate on without reverting to read-modify-write
>    operations. Always greater than or equal to the logical block size.
> 
> These are characteristics of the device itself tracked in the
> request_queue. For loop devices, both are currently always 512 bytes.
> 
> struct block_device also has another block size, basically a "soft"
> block size which is the preferred I/O size and can be changed from
> userspace. For loop devices, this is PAGE_SIZE if the backing file is a
> regular file or otherwise the soft block size of the backing block
> device.
> 
> Patch 1 simplifies how the soft block size is set. I'm tempted to not
> set it at all and use the default of PAGE_SIZE, but maybe someone is
> depending on inheriting the soft block size from the backing block
> device...
> 
> Patch 2 sets the physical block size of loop devices to a more
> meaningful value for loop, PAGE_SIZE.
> 
> Patch 3 allows us to change the logical block size. It adds a new ioctl
> instead of the previous approach (which extends LOOP_{GET,SET}_STATUS).
> Hannes, I assume you had a specific reason for doing it the way you did,
> care to explain?
> 
> Patch 4 is a cleanup.
> 
> This is based on my patch reverting the original block size support,
> targeting 4.14. Thanks!

Applied for 4.14, thanks Omar.

-- 
Jens Axboe