[PATCHv4 0/2] loop: enable different logical blocksizes

[PATCHv4 0/2] loop: enable different logical blocksizes

[Hannes Reinecke]

Currently the loop driver just simulates 512-byte blocks. When
creating bootable images for virtual machines it might be required
to use a different physical blocksize (eg 4k for S/390 DASD), as
the some bootloaders (like lilo or zipl for S/390) need to know
the physical block addresses of the kernel and initrd.

With this patchset the loop driver will export the logical and
physical blocksize and the current LOOP_SET_STATUS64 ioctl is
extended to set the logical blocksize by re-using the existing
'init' fields, which are currently unused.

As usual, comments and reviews are welcome.

Changes to v1:
- Move LO_FLAGS_BLOCKSIZE definition
- Reshuffle patches
Changes to v2:
- Include reviews from Ming Lei
Changes to v3:
- Include reviews from Christoph
- Merge patches

Hannes Reinecke (2):
  loop: Remove unused 'bdev' argument from loop_set_capacity
  loop: support 4k physical blocksize

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

-- 
2.6.6

[PATCHv3 1/2] loop: Remove unused 'bdev' argument from loop_set_capacity

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/block/loop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index fa1b7a9..3008dec 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1298,7 +1298,7 @@ loop_get_status64(struct loop_device *lo, struct loop_info64 __user *arg) {
 	return err;
 }
 
-static int loop_set_capacity(struct loop_device *lo, struct block_device *bdev)
+static int loop_set_capacity(struct loop_device *lo)
 {
 	if (unlikely(lo->lo_state != Lo_bound))
 		return -ENXIO;
@@ -1361,7 +1361,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	case LOOP_SET_CAPACITY:
 		err = -EPERM;
 		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
-			err = loop_set_capacity(lo, bdev);
+			err = loop_set_capacity(lo);
 		break;
 	case LOOP_SET_DIRECT_IO:
 		err = -EPERM;
-- 
2.6.6

[PATCH 2/2] loop: support 4k physical blocksize

[Hannes Reinecke]

When generating bootable VM images certain systems (most notably
s390x) require devices with 4k blocksize. This patch implements
a new flag 'LO_FLAGS_BLOCKSIZE' which will set the physical
blocksize to that of the underlying device, and allow to change
the logical blocksize for up to the physical blocksize.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/block/loop.c      | 36 +++++++++++++++++++++++++++++++-----
 drivers/block/loop.h      |  1 +
 include/uapi/linux/loop.h |  1 +
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3008dec..9ad56bf 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -221,7 +221,8 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
 }
 
 static int
-figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
+figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit,
+		 loff_t logical_blocksize)
 {
 	loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
 	sector_t x = (sector_t)size;
@@ -233,6 +234,12 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
 		lo->lo_offset = offset;
 	if (lo->lo_sizelimit != sizelimit)
 		lo->lo_sizelimit = sizelimit;
+	if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) {
+		lo->lo_logical_blocksize = logical_blocksize;
+		blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
+		blk_queue_logical_block_size(lo->lo_queue,
+					     lo->lo_logical_blocksize);
+	}
 	set_capacity(lo->lo_disk, x);
 	bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9);
 	/* let user-space know about the new size */
@@ -814,6 +821,7 @@ static void loop_config_discard(struct loop_device *lo)
 	struct file *file = lo->lo_backing_file;
 	struct inode *inode = file->f_mapping->host;
 	struct request_queue *q = lo->lo_queue;
+	int lo_bits = blksize_bits(lo->lo_logical_blocksize);
 
 	/*
 	 * We use punch hole to reclaim the free space used by the
@@ -833,7 +841,7 @@ static void loop_config_discard(struct loop_device *lo)
 
 	q->limits.discard_granularity = inode->i_sb->s_blocksize;
 	q->limits.discard_alignment = 0;
-	blk_queue_max_discard_sectors(q, UINT_MAX >> 9);
+	blk_queue_max_discard_sectors(q, UINT_MAX >> lo_bits);
 	q->limits.discard_zeroes_data = 1;
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 }
@@ -922,6 +930,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 
 	lo->use_dio = false;
 	lo->lo_blocksize = lo_blocksize;
+	lo->lo_logical_blocksize = 512;
 	lo->lo_device = bdev;
 	lo->lo_flags = lo_flags;
 	lo->lo_backing_file = file;
@@ -1087,6 +1096,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	int err;
 	struct loop_func_table *xfer;
 	kuid_t uid = current_uid();
+	int lo_flags = lo->lo_flags;
 
 	if (lo->lo_encrypt_key_size &&
 	    !uid_eq(lo->lo_key_owner, uid) &&
@@ -1116,9 +1126,24 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	if (err)
 		return err;
 
+	if (info->lo_flags & LO_FLAGS_BLOCKSIZE) {
+		lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
+		if ((info->lo_init[0] != 512) &&
+		    (info->lo_init[0] != 1024) &&
+		    (info->lo_init[0] != 2048) &&
+		    (info->lo_init[0] != 4096))
+			return -EINVAL;
+		if (info->lo_init[0] > lo->lo_blocksize)
+			return -EINVAL;
+	}
+
 	if (lo->lo_offset != info->lo_offset ||
-	    lo->lo_sizelimit != info->lo_sizelimit)
-		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit))
+	    lo->lo_sizelimit != info->lo_sizelimit ||
+	    lo->lo_flags != lo_flags ||
+	    ((lo->lo_flags & LO_FLAGS_BLOCKSIZE) &&
+	     (lo->lo_logical_blocksize != info->lo_init[0])))
+		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
+				     info->lo_init[0]))
 			return -EFBIG;
 
 	loop_config_discard(lo);
@@ -1303,7 +1328,8 @@ static int loop_set_capacity(struct loop_device *lo)
 	if (unlikely(lo->lo_state != Lo_bound))
 		return -ENXIO;
 
-	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
+	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit,
+				lo->lo_logical_blocksize);
 }
 
 static int loop_set_dio(struct loop_device *lo, unsigned long arg)
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index fb2237c..579f2f7 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -49,6 +49,7 @@ struct loop_device {
 	struct file *	lo_backing_file;
 	struct block_device *lo_device;
 	unsigned	lo_blocksize;
+	unsigned	lo_logical_blocksize;
 	void		*key_data; 
 
 	gfp_t		old_gfp_mask;
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index c8125ec..2691c1c 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -22,6 +22,7 @@ enum {
 	LO_FLAGS_AUTOCLEAR	= 4,
 	LO_FLAGS_PARTSCAN	= 8,
 	LO_FLAGS_DIRECT_IO	= 16,
+	LO_FLAGS_BLOCKSIZE	= 32,
 };
 
 #include <asm/posix_types.h>	/* for __kernel_old_dev_t */
-- 
2.6.6

Re: [PATCH 2/2] loop: support 4k physical blocksize

[Christoph Hellwig]

> -figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
> +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit,
> +		 loff_t logical_blocksize)
>  {
>  	loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
>  	sector_t x = (sector_t)size;
> @@ -233,6 +234,12 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
>  		lo->lo_offset = offset;
>  	if (lo->lo_sizelimit != sizelimit)
>  		lo->lo_sizelimit = sizelimit;
> +	if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) {
> +		lo->lo_logical_blocksize = logical_blocksize;
> +		blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
> +		blk_queue_logical_block_size(lo->lo_queue,
> +					     lo->lo_logical_blocksize);
> +	}

I've looked how the existing code uses lo_blocksize and this whole thing
confuses me to no end.  Can we have all the blocksize assignments (i.e.
including the existing lo_blocksize assignments) in one single place and
documented?  Especialy as it seems so far lo_blocksize seems to be
the "fs" blocksize as set by set_blocksize, which seems totally wrong
to be set by loop at all.

> +	if (info->lo_flags & LO_FLAGS_BLOCKSIZE) {
> +		lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
> +		if ((info->lo_init[0] != 512) &&
> +		    (info->lo_init[0] != 1024) &&
> +		    (info->lo_init[0] != 2048) &&
> +		    (info->lo_init[0] != 4096))
> +			return -EINVAL;

No need for the inner braces.  Also please find a way to have a
descriptive name for the field, be that an anonymous union, or a #define.

> +	     (lo->lo_logical_blocksize != info->lo_init[0])))

Same comment about the inner braces here.

> +		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
> +				     info->lo_init[0]))
>  			return -EFBIG;
>  
>  	loop_config_discard(lo);
> @@ -1303,7 +1328,8 @@ static int loop_set_capacity(struct loop_device *lo)
>  	if (unlikely(lo->lo_state != Lo_bound))
>  		return -ENXIO;
>  
> -	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
> +	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit,
> +				lo->lo_logical_blocksize);

I'd say drop all the arguments but lo here (maybe in a prep patch) as
passing them all seems pointless and confusing.

Re: [PATCH 2/2] loop: support 4k physical blocksize

[Ming Lei]

On Thu, Nov 3, 2016 at 10:26 PM, Christoph Hellwig <hch@lst.de> wrote:
>> -figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
>> +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit,
>> +              loff_t logical_blocksize)
>>  {
>>       loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
>>       sector_t x = (sector_t)size;
>> @@ -233,6 +234,12 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
>>               lo->lo_offset = offset;
>>       if (lo->lo_sizelimit != sizelimit)
>>               lo->lo_sizelimit = sizelimit;
>> +     if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) {
>> +             lo->lo_logical_blocksize = logical_blocksize;
>> +             blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
>> +             blk_queue_logical_block_size(lo->lo_queue,
>> +                                          lo->lo_logical_blocksize);
>> +     }
>
> I've looked how the existing code uses lo_blocksize and this whole thing
> confuses me to no end.  Can we have all the blocksize assignments (i.e.
> including the existing lo_blocksize assignments) in one single place and
> documented?  Especialy as it seems so far lo_blocksize seems to be
> the "fs" blocksize as set by set_blocksize, which seems totally wrong
> to be set by loop at all.

This patch uses backing fs block size to emulate physical block size
of loop block device, and the logical block size of loop is set from user
space, seems the idea is correct.

>
>> +     if (info->lo_flags & LO_FLAGS_BLOCKSIZE) {
>> +             lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
>> +             if ((info->lo_init[0] != 512) &&
>> +                 (info->lo_init[0] != 1024) &&
>> +                 (info->lo_init[0] != 2048) &&
>> +                 (info->lo_init[0] != 4096))
>> +                     return -EINVAL;
>
> No need for the inner braces.  Also please find a way to have a
> descriptive name for the field, be that an anonymous union, or a #define.
>
>> +          (lo->lo_logical_blocksize != info->lo_init[0])))
>
> Same comment about the inner braces here.
>
>> +             if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
>> +                                  info->lo_init[0]))
>>                       return -EFBIG;
>>
>>       loop_config_discard(lo);
>> @@ -1303,7 +1328,8 @@ static int loop_set_capacity(struct loop_device *lo)
>>       if (unlikely(lo->lo_state != Lo_bound))
>>               return -ENXIO;
>>
>> -     return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
>> +     return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit,
>> +                             lo->lo_logical_blocksize);
>
> I'd say drop all the arguments but lo here (maybe in a prep patch) as
> passing them all seems pointless and confusing.



-- 
Ming Lei