Add a new LOOPSET_FD_AND_STATUS ioctl
v2 cover Add a new LOOPSET_FD_AND_STATUS ioctl
This series introduces a new ioctl that makes it possible to atomically configure a loop device. Previously, if you wanted to set parameters such as the offset on a loop device, this required calling LOOP_SET_FD to set the backing file, and then LOOP_SET_STATUS to set the offset. However, in between these two calls, the loop device is available and would accept requests, which is generally not desirable.
There are also performance benefits with combining these two ioctls into one, which are described in more detail in the last change in the series.
Note that this series depends on
"loop: Call loop_config_discard() only after new config is applied." [0], which I sent as a separate patch as it fixes an unrelated bug.
[0]: https://lkml.org/lkml/2020/3/31/755
---
v2:
- Addressed review comments from Bart van Assche:
-- Use SECTOR_SHIFT constant
-- Renamed loop_set_from_status() to loop_set_status_from_info() -- Added kerneldoc for loop_set_status_from_info()
-- Removed dots in patch subject lines
- Addressed review comments from Christoph Hellwig:
-- Added missing padding in struct loop_fd_and_status -- Cleaned up some __user pointer handling in lo_ioctl -- Pass in a stack-initialized loop_info64 for the legacy
LOOP_SET_FD case
Martijn Coenen (5):
loop: Refactor size calculation
loop: Factor out configuring loop from status
loop: Move loop_set_status_from_info() and friends up
loop: rework lo_ioctl() __user argument casting
loop: Add LOOP_SET_FD_AND_STATUS ioctl
drivers/block/loop.c | 327 ++++++++++++++++++++++---------------- include/uapi/linux/loop.h | 7 + 2 files changed, 201 insertions(+), 133 deletions(-)
Signature · 1 line
2.26.2.303.gf8c07b1a785-goog
PATCH v2 1/5 loop: Refactor size calculation
figure_loop_size() calculates the loop size based on the passed in parameters, but at the same time it updates the offset and sizelimit parameters in the loop device configuration. That is a somewhat unexpected side effect of a function with this name, and it is only only needed by one of the two callers of this function - loop_set_status().
Move the lo_offset and lo_sizelimit assignment back into loop_set_status(), and factor out a new function (loop_set_size()) to apply a newly calculated size that has been validated to fit in sector_t, and notify user-space.
loop_update_size() is now solely used by loop_set_capacity to compute an updated size, and loop_set_status() uses a combination of functions to validate the loop size, and only apply it later.
Signed-off-by Martijn Coenen <[email protected]>
--- drivers/block/loop.c | 88 ++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 32 deletions(-)
drivers/block/loop.c +56 -32
@@ -228,23 +228,42 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)blk_mq_unfreeze_queue (lo->lo_queue); } +/** +* loop_set_size - sets device size and notifies userspace +* @lo: struct loop_device to set the size for +* @size: new size of the loop device +* +* Callers must validate that the size passed into this function fits into +* a sector_t. +*/ +static void loop_set_size (struct loop_device *lo,loff_t size) +{ +struct block_device *bdev = lo->lo_device; + +set_capacity (lo->lo_disk, size); +bd_set_size (bdev, size <<SECTOR_SHIFT ); +/* let user-space know about the new size */ +kobject_uevent (&disk_to_dev (bdev->bd_disk)->kobj,KOBJ_CHANGE ); +} + +/** +* loop_update_size - updates device size from the backing file +* @lo: struct loop_device to update the size for +* +* Recomputes the device size from the backing file, and updates the device +* with the new size. +*/ static int -figure_loop_size (struct loop_device *lo,loff_t offset,loff_t sizelimit) +loop_update_size (struct loop_device *lo) { -loff_t size =get_size (offset, sizelimit, lo->lo_backing_file); +loff_t size =get_loop_size (lo, lo->lo_backing_file);sector_t x = (sector_t )size; -struct block_device *bdev = lo->lo_device;if (unlikely ((loff_t )x != size))return -EFBIG ; -if (lo->lo_offset != offset) - lo->lo_offset = offset; -if (lo->lo_sizelimit != sizelimit) - lo->lo_sizelimit = sizelimit; -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 */ -kobject_uevent (&disk_to_dev (bdev->bd_disk)->kobj,KOBJ_CHANGE ); + +loop_set_size (lo, size); +return 0 ; }
@@ -1040,11 +1059,8 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,loop_update_rotational (lo);loop_update_dio (lo); -set_capacity (lo->lo_disk, size); -bd_set_size (bdev, size <<9 );loop_sysfs_init (lo); -/* let user-space know about the new size */ -kobject_uevent (&disk_to_dev (bdev->bd_disk)->kobj,KOBJ_CHANGE ); +loop_set_size (lo, size);set_blocksize (bdev,S_ISBLK (inode->i_mode) ?block_size (inode->i_bdev) :PAGE_SIZE );
@@ -1267,6 +1283,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)kuid_t uid =current_uid ();struct block_device *bdev;bool partscan = false; +bool size_changed = false; +loff_t validated_size; err =mutex_lock_killable (&loop_ctl_mutex);if (err)
@@ -1288,6 +1306,14 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)if (lo->lo_offset != info->lo_offset || lo->lo_sizelimit != info->lo_sizelimit) { +size_t size =get_size (info->lo_offset, info->lo_sizelimit, + lo->lo_backing_file); +if ((loff_t )(sector_t )size != size) { + err = -EFBIG ; + goto out_unlock; + } + size_changed = true; + validated_size = size;sync_blockdev (lo->lo_device);kill_bdev (lo->lo_device); }
@@ -1295,6 +1321,15 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)/* I/O need to be drained during transfer transition */ blk_mq_freeze_queue (lo->lo_queue); +if (size_changed && lo->lo_device->bd_inode->i_mapping->nrpages) { +/* If any pages were dirtied after kill_bdev(), try again */ + err = -EAGAIN ; +pr_warn ("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n" , + __func__, lo->lo_number, lo->lo_file_name, + lo->lo_device->bd_inode->i_mapping->nrpages); + goto out_unfreeze; + } + err =loop_release_xfer (lo);if (err) goto out_unfreeze;
@@ -1318,22 +1353,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)if (err) goto out_unfreeze; -if (lo->lo_offset != info->lo_offset || - lo->lo_sizelimit != info->lo_sizelimit) { -/* kill_bdev should have truncated all the pages */ -if (lo->lo_device->bd_inode->i_mapping->nrpages) { - err = -EAGAIN ; -pr_warn ("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n" , - __func__, lo->lo_number, lo->lo_file_name, - lo->lo_device->bd_inode->i_mapping->nrpages); - goto out_unfreeze; - } -if (figure_loop_size (lo, info->lo_offset, info->lo_sizelimit)) { - err = -EFBIG ; - goto out_unfreeze; - } - } - + lo->lo_offset = info->lo_offset; + lo->lo_sizelimit = info->lo_sizelimit;memcpy (lo->lo_file_name, info->lo_file_name,LO_NAME_SIZE );memcpy (lo->lo_crypt_name, info->lo_crypt_name,LO_NAME_SIZE ); lo->lo_file_name[LO_NAME_SIZE -1 ] =0 ;
@@ -1357,6 +1378,9 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) lo->lo_key_owner = uid; } +if (size_changed) +loop_set_size (lo, validated_size); +loop_config_discard (lo);/* update dio if lo_offset or transfer is changed */
@@ -1534,7 +1558,7 @@ 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 loop_update_size (lo); }static int loop_set_dio (struct loop_device *lo,unsigned long arg)
Signature · 1 line
2.26.2.303.gf8c07b1a785-goog
Re: [PATCH v2 1/5] loop: Refactor size calculation
Quoted 14 lines · Martijn Coenen
drivers/block/loop.c +6
-0@@ -228,23 +228,42 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)blk_mq_unfreeze_queue (lo->lo_queue); } +/** +* loop_set_size - sets device size and notifies userspace +* @lo: struct loop_device to set the size for +* @size: new size of the loop device +* +* Callers must validate that the size passed into this function fits into
drivers/block/loop.c static void __loop_update_dio(struct loop_device *lo, bool dio) · line 246
Quoted 10 lines
drivers/block/loop.c +10
-0+* a sector_t. +*/ +static void loop_set_size (struct loop_device *lo,loff_t size) +{ +struct block_device *bdev = lo->lo_device; + +set_capacity (lo->lo_disk, size); +bd_set_size (bdev, size <<SECTOR_SHIFT ); +/* let user-space know about the new size */ +kobject_uevent (&disk_to_dev (bdev->bd_disk)->kobj,KOBJ_CHANGE );
I think this should use set_capacity_revalidate_and_notify, although that probably should be a separate patch.
Also I'm having a bit of a hard time following all the different changes (even if the all look good). Maybe at least the loop_set_size factoring should be split into another separate prep patch.
Quoted 11 lines
drivers/block/loop.c +8
-1+/** +* loop_update_size - updates device size from the backing file +* @lo: struct loop_device to update the size for +* +* Recomputes the device size from the backing file, and updates the device +* with the new size. +*/ static int -figure_loop_size (struct loop_device *lo,loff_t offset,loff_t sizelimit) +loop_update_size (struct loop_device *lo) {
drivers/block/loop.c static void __loop_update_dio(struct loop_device *lo, bool dio) · line 268
Quoted 10 lines
drivers/block/loop.c +4
-0+loff_t size =get_loop_size (lo, lo->lo_backing_file);sector_t x = (sector_t )size;if (unlikely ((loff_t )x != size))return -EFBIG ; + +loop_set_size (lo, size); +return 0 ; }
With all the other nice helper, I think this can actually be open coded in the caller. Another useful helper would be one that checks for the size truncation, as that is duplicated in a few places.
PATCH v2 2/5 loop: Factor out configuring loop from status
In reply to Martijn CoenenFactor out this code into a separate function, so it can be reused by other code more easily.
Signed-off-by Martijn Coenen <[email protected]>
--- drivers/block/loop.c | 117 +++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 50 deletions(-)
drivers/block/loop.c +67 -50
@@ -1275,13 +1275,78 @@ static int loop_clr_fd(struct loop_device *lo)return __loop_clr_fd (lo, false); } +/** +* loop_set_status_from_info - configure device from loop_info +* @lo: struct loop_device to configure +* @info: struct loop_info64 to configure the device with +* +* Configures the loop device parameters according to the passed +* in loop_info64 configuration. +*/ static int -loop_set_status (struct loop_device *lo,const struct loop_info64 *info) +loop_set_status_from_info (struct loop_device *lo, +const struct loop_info64 *info) {int err;struct loop_func_table *xfer;kuid_t uid =current_uid (); + +if ((unsigned int ) info->lo_encrypt_key_size >LO_KEY_SIZE ) +return -EINVAL ; + + err =loop_release_xfer (lo); +if (err) +return err; + +if (info->lo_encrypt_type) { +unsigned int type = info->lo_encrypt_type; + +if (type >=MAX_LO_CRYPT ) +return -EINVAL ; + xfer = xfer_funcs[type]; +if (xfer ==NULL ) +return -EINVAL ; + }else + xfer =NULL ; + + err =loop_init_xfer (lo, xfer, info); +if (err) +return err; + + lo->lo_offset = info->lo_offset; + lo->lo_sizelimit = info->lo_sizelimit; +memcpy (lo->lo_file_name, info->lo_file_name,LO_NAME_SIZE ); +memcpy (lo->lo_crypt_name, info->lo_crypt_name,LO_NAME_SIZE ); + lo->lo_file_name[LO_NAME_SIZE -1 ] =0 ; + lo->lo_crypt_name[LO_NAME_SIZE -1 ] =0 ; + +if (!xfer) + xfer = &none_funcs; + lo->transfer = xfer->transfer; + lo->ioctl = xfer->ioctl; + +if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) != + (info->lo_flags &LO_FLAGS_AUTOCLEAR )) + lo->lo_flags ^=LO_FLAGS_AUTOCLEAR ; + + lo->lo_encrypt_key_size = info->lo_encrypt_key_size; + lo->lo_init[0 ] = info->lo_init[0 ]; + lo->lo_init[1 ] = info->lo_init[1 ]; +if (info->lo_encrypt_key_size) { +memcpy (lo->lo_encrypt_key, info->lo_encrypt_key, + info->lo_encrypt_key_size); + lo->lo_key_owner = uid; + } + +return 0 ; +} + +static int +loop_set_status (struct loop_device *lo,const struct loop_info64 *info) +{ +int err;struct block_device *bdev; +kuid_t uid =current_uid ();bool partscan = false;bool size_changed = false;loff_t validated_size;
@@ -1299,10 +1364,6 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) err = -ENXIO ; goto out_unlock; } -if ((unsigned int ) info->lo_encrypt_key_size >LO_KEY_SIZE ) { - err = -EINVAL ; - goto out_unlock; - }if (lo->lo_offset != info->lo_offset || lo->lo_sizelimit != info->lo_sizelimit) {
@@ -1330,54 +1391,10 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) goto out_unfreeze; } - err =loop_release_xfer (lo); + err =loop_set_status_from_info (lo, info);if (err) goto out_unfreeze; -if (info->lo_encrypt_type) { -unsigned int type = info->lo_encrypt_type; - -if (type >=MAX_LO_CRYPT ) { - err = -EINVAL ; - goto out_unfreeze; - } - xfer = xfer_funcs[type]; -if (xfer ==NULL ) { - err = -EINVAL ; - goto out_unfreeze; - } - }else - xfer =NULL ; - - err =loop_init_xfer (lo, xfer, info); -if (err) - goto out_unfreeze; - - lo->lo_offset = info->lo_offset; - lo->lo_sizelimit = info->lo_sizelimit; -memcpy (lo->lo_file_name, info->lo_file_name,LO_NAME_SIZE ); -memcpy (lo->lo_crypt_name, info->lo_crypt_name,LO_NAME_SIZE ); - lo->lo_file_name[LO_NAME_SIZE -1 ] =0 ; - lo->lo_crypt_name[LO_NAME_SIZE -1 ] =0 ; - -if (!xfer) - xfer = &none_funcs; - lo->transfer = xfer->transfer; - lo->ioctl = xfer->ioctl; - -if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) != - (info->lo_flags &LO_FLAGS_AUTOCLEAR )) - lo->lo_flags ^=LO_FLAGS_AUTOCLEAR ; - - lo->lo_encrypt_key_size = info->lo_encrypt_key_size; - lo->lo_init[0 ] = info->lo_init[0 ]; - lo->lo_init[1 ] = info->lo_init[1 ]; -if (info->lo_encrypt_key_size) { -memcpy (lo->lo_encrypt_key, info->lo_encrypt_key, - info->lo_encrypt_key_size); - lo->lo_key_owner = uid; - } -if (size_changed)loop_set_size (lo, validated_size);
Signature · 1 line
2.26.2.303.gf8c07b1a785-goog
Re: [PATCH v2 2/5] loop: Factor out configuring loop from status
Quoted 4 lines · Martijn Coenen
Factor out this code into a separate function, so it can be reused by other code more easily.
Signed-off-by Martijn Coenen <[email protected]>
Looks good,
Reviewed-by Christoph Hellwig <[email protected]>
PATCH v2 3/5 loop: Move loop_set_status_from_info() and friends up
In reply to Martijn CoenenSo we can use it without forward declaration. This is a separate commit to make it easier to verify that this is just a move, without functional modifications.
Signed-off-by Martijn Coenen <[email protected]>
--- drivers/block/loop.c | 206 +++++++++++++++++++++---------------------- 1 file changed, 103 insertions(+), 103 deletions(-)
drivers/block/loop.c +103 -103
@@ -971,6 +971,109 @@ static void loop_update_rotational(struct loop_device *lo)blk_queue_flag_clear (QUEUE_FLAG_NONROT , q); } +static int +loop_release_xfer (struct loop_device *lo) +{ +int err =0 ; +struct loop_func_table *xfer = lo->lo_encryption; + +if (xfer) { +if (xfer->release) + err = xfer->release (lo); + lo->transfer =NULL ; + lo->lo_encryption =NULL ; +module_put (xfer->owner); + } +return err; +} + +static int +loop_init_xfer (struct loop_device *lo,struct loop_func_table *xfer, +const struct loop_info64 *i) +{ +int err =0 ; + +if (xfer) { +struct module *owner = xfer->owner; + +if (!try_module_get (owner)) +return -EINVAL ; +if (xfer->init) + err = xfer->init (lo, i); +if (err) +module_put (owner); +else + lo->lo_encryption = xfer; + } +return err; +} + +/** +* loop_set_status_from_info - configure device from loop_info +* @lo: struct loop_device to configure +* @info: struct loop_info64 to configure the device with +* +* Configures the loop device parameters according to the passed +* in loop_info64 configuration. +*/ +static int +loop_set_status_from_info (struct loop_device *lo, +const struct loop_info64 *info) +{ +int err; +struct loop_func_table *xfer; +kuid_t uid =current_uid (); + +if ((unsigned int ) info->lo_encrypt_key_size >LO_KEY_SIZE ) +return -EINVAL ; + + err =loop_release_xfer (lo); +if (err) +return err; + +if (info->lo_encrypt_type) { +unsigned int type = info->lo_encrypt_type; + +if (type >=MAX_LO_CRYPT ) +return -EINVAL ; + xfer = xfer_funcs[type]; +if (xfer ==NULL ) +return -EINVAL ; + }else + xfer =NULL ; + + err =loop_init_xfer (lo, xfer, info); +if (err) +return err; + + lo->lo_offset = info->lo_offset; + lo->lo_sizelimit = info->lo_sizelimit; +memcpy (lo->lo_file_name, info->lo_file_name,LO_NAME_SIZE ); +memcpy (lo->lo_crypt_name, info->lo_crypt_name,LO_NAME_SIZE ); + lo->lo_file_name[LO_NAME_SIZE -1 ] =0 ; + lo->lo_crypt_name[LO_NAME_SIZE -1 ] =0 ; + +if (!xfer) + xfer = &none_funcs; + lo->transfer = xfer->transfer; + lo->ioctl = xfer->ioctl; + +if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) != + (info->lo_flags &LO_FLAGS_AUTOCLEAR )) + lo->lo_flags ^=LO_FLAGS_AUTOCLEAR ; + + lo->lo_encrypt_key_size = info->lo_encrypt_key_size; + lo->lo_init[0 ] = info->lo_init[0 ]; + lo->lo_init[1 ] = info->lo_init[1 ]; +if (info->lo_encrypt_key_size) { +memcpy (lo->lo_encrypt_key, info->lo_encrypt_key, + info->lo_encrypt_key_size); + lo->lo_key_owner = uid; + } + +return 0 ; +} +static int loop_set_fd (struct loop_device *lo,fmode_t mode,struct block_device *bdev,unsigned int arg) {
@@ -1094,43 +1197,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,return error; } -static int -loop_release_xfer (struct loop_device *lo) -{ -int err =0 ; -struct loop_func_table *xfer = lo->lo_encryption; - -if (xfer) { -if (xfer->release) - err = xfer->release (lo); - lo->transfer =NULL ; - lo->lo_encryption =NULL ; -module_put (xfer->owner); - } -return err; -} - -static int -loop_init_xfer (struct loop_device *lo,struct loop_func_table *xfer, -const struct loop_info64 *i) -{ -int err =0 ; - -if (xfer) { -struct module *owner = xfer->owner; - -if (!try_module_get (owner)) -return -EINVAL ; -if (xfer->init) - err = xfer->init (lo, i); -if (err) -module_put (owner); -else - lo->lo_encryption = xfer; - } -return err; -} -static int __loop_clr_fd (struct loop_device *lo,bool release) {struct file *filp =NULL ;
@@ -1275,72 +1341,6 @@ static int loop_clr_fd(struct loop_device *lo)return __loop_clr_fd (lo, false); } -/** -* loop_set_status_from_info - configure device from loop_info -* @lo: struct loop_device to configure -* @info: struct loop_info64 to configure the device with -* -* Configures the loop device parameters according to the passed -* in loop_info64 configuration. -*/ -static int -loop_set_status_from_info (struct loop_device *lo, -const struct loop_info64 *info) -{ -int err; -struct loop_func_table *xfer; -kuid_t uid =current_uid (); - -if ((unsigned int ) info->lo_encrypt_key_size >LO_KEY_SIZE ) -return -EINVAL ; - - err =loop_release_xfer (lo); -if (err) -return err; - -if (info->lo_encrypt_type) { -unsigned int type = info->lo_encrypt_type; - -if (type >=MAX_LO_CRYPT ) -return -EINVAL ; - xfer = xfer_funcs[type]; -if (xfer ==NULL ) -return -EINVAL ; - }else - xfer =NULL ; - - err =loop_init_xfer (lo, xfer, info); -if (err) -return err; - - lo->lo_offset = info->lo_offset; - lo->lo_sizelimit = info->lo_sizelimit; -memcpy (lo->lo_file_name, info->lo_file_name,LO_NAME_SIZE ); -memcpy (lo->lo_crypt_name, info->lo_crypt_name,LO_NAME_SIZE ); - lo->lo_file_name[LO_NAME_SIZE -1 ] =0 ; - lo->lo_crypt_name[LO_NAME_SIZE -1 ] =0 ; - -if (!xfer) - xfer = &none_funcs; - lo->transfer = xfer->transfer; - lo->ioctl = xfer->ioctl; - -if ((lo->lo_flags &LO_FLAGS_AUTOCLEAR ) != - (info->lo_flags &LO_FLAGS_AUTOCLEAR )) - lo->lo_flags ^=LO_FLAGS_AUTOCLEAR ; - - lo->lo_encrypt_key_size = info->lo_encrypt_key_size; - lo->lo_init[0 ] = info->lo_init[0 ]; - lo->lo_init[1 ] = info->lo_init[1 ]; -if (info->lo_encrypt_key_size) { -memcpy (lo->lo_encrypt_key, info->lo_encrypt_key, - info->lo_encrypt_key_size); - lo->lo_key_owner = uid; - } - -return 0 ; -} -static int loop_set_status (struct loop_device *lo,const struct loop_info64 *info) {
Signature · 1 line
2.26.2.303.gf8c07b1a785-goog
Re: [PATCH v2 3/5] loop: Move loop_set_status_from_info() and friends up
Looks good,
Reviewed-by Christoph Hellwig <[email protected]>
PATCH v2 4/5 loop: rework lo_ioctl() __user argument casting
In reply to Martijn CoenenIn preparation for a new ioctl that needs to copy_from_user(); makes the code easier to read as well.
Signed-off-by Martijn Coenen <[email protected]>
--- drivers/block/loop.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/block/loop.c +5 -6
@@ -1658,6 +1658,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, unsigned long arg) {struct loop_device *lo = bdev->bd_disk->private_data; +void __user *argp = (void __user *) arg;int err;switch (cmd) {
@@ -1670,21 +1671,19 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,case LOOP_SET_STATUS : err = -EPERM ;if ((mode &FMODE_WRITE ) ||capable (CAP_SYS_ADMIN )) { - err =loop_set_status_old (lo, - (struct loop_info __user *)arg); + err =loop_set_status_old (lo, argp); }break ;case LOOP_GET_STATUS : -return loop_get_status_old (lo, (struct loop_info __user *) arg); +return loop_get_status_old (lo, argp);case LOOP_SET_STATUS64 : err = -EPERM ;if ((mode &FMODE_WRITE ) ||capable (CAP_SYS_ADMIN )) { - err =loop_set_status64 (lo, - (struct loop_info64 __user *) arg); + err =loop_set_status64 (lo, argp); }break ;case LOOP_GET_STATUS64 : -return loop_get_status64 (lo, (struct loop_info64 __user *) arg); +return loop_get_status64 (lo, argp);case LOOP_SET_CAPACITY :case LOOP_SET_DIRECT_IO :case LOOP_SET_BLOCK_SIZE :
Signature · 1 line
2.26.2.303.gf8c07b1a785-goog
Re: [PATCH v2 4/5] loop: rework lo_ioctl() __user argument casting
Quoted 4 lines · Martijn Coenen
In preparation for a new ioctl that needs to
copy_from_user(); makes the code easier to read as well.Signed-off-by Martijn Coenen <[email protected]>
Looks good,
Reviewed-by Christoph Hellwig <[email protected]>
PATCH v2 5/5 loop: Add LOOP_SET_FD_AND_STATUS ioctl
In reply to Martijn CoenenThis allows userspace to completely setup a loop device with a single ioctl, removing the in-between state where the device can be partially configured - eg the loop device has a backing file associated with it, but is reading from the wrong offset.
Besides removing the intermediate state, another big benefit of this ioctl is that LOOP_SET_STATUS can be slow; the main reason for this slowness is that LOOP_SET_STATUS(64) calls blk_mq_freeze_queue() to freeze the associated queue; this requires waiting for RCU synchronization, which I've measured can take about 15-20ms on this device on average.
Here's setting up ~70 regular loop devices with an offset on an x86 Android device, using LOOP_SET_FD and LOOP_SET_STATUS:
vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done
0m03.40s real 0m00.02s user 0m00.03s system
Here's configuring ~70 devices in the same way, but using a modified losetup that uses the new LOOP_SET_FD_AND_STATUS ioctl:
vsoc_x86:/system/apex # time for i in seq 30 100;
do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done
0m01.94s real 0m00.01s user 0m00.01s system
Signed-off-by Martijn Coenen <[email protected]>
--- drivers/block/loop.c | 45 ++++++++++++++++++++++++++++----------- include/uapi/linux/loop.h | 7 ++++++ 2 files changed, 40 insertions(+), 12 deletions(-)
drivers/block/loop.c +33 -12
@@ -1074,8 +1074,9 @@ loop_set_status_from_info(struct loop_device *lo,return 0 ; } -static int loop_set_fd (struct loop_device *lo,fmode_t mode, -struct block_device *bdev,unsigned int arg) +static int loop_set_fd_and_status (struct loop_device *lo,fmode_t mode, +struct block_device *bdev,unsigned int fd, +const struct loop_info64 *info) {struct file *file;struct inode *inode;
@@ -1090,7 +1091,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,__module_get (THIS_MODULE ); error = -EBADF ; - file =fget (arg); + file =fget (fd);if (!file) goto out;
@@ -1099,7 +1100,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,* here to avoid changing device under exclusive owner. */ if (!(mode &FMODE_EXCL )) { - claimed_bdev =bd_start_claiming (bdev, loop_set_fd); + claimed_bdev =bd_start_claiming (bdev, loop_set_fd_and_status);if (IS_ERR (claimed_bdev)) { error =PTR_ERR (claimed_bdev); goto out_putf;
@@ -1126,9 +1127,14 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode, lo_flags |=LO_FLAGS_READ_ONLY ; error = -EFBIG ; - size =get_loop_size (lo, file); + size =get_size (info->lo_offset, info->lo_sizelimit, file);if ((loff_t )(sector_t )size != size) goto out_unlock; + + error =loop_set_status_from_info (lo, info); +if (error) + goto out_unlock; + error =loop_prepare_queue (lo);if (error) goto out_unlock;
@@ -1141,9 +1147,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode, lo->lo_device = bdev; lo->lo_flags = lo_flags; lo->lo_backing_file = file; - lo->transfer =NULL ; - lo->ioctl =NULL ; - lo->lo_sizelimit =0 ; lo->old_gfp_mask =mapping_gfp_mask (mapping);mapping_set_gfp_mask (mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
@@ -1181,14 +1184,14 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,if (partscan)loop_reread_partitions (lo, bdev);if (claimed_bdev) -bd_abort_claiming (bdev, claimed_bdev, loop_set_fd); +bd_abort_claiming (bdev, claimed_bdev, loop_set_fd_and_status);return 0 ; out_unlock:mutex_unlock (&loop_ctl_mutex); out_bdev:if (claimed_bdev) -bd_abort_claiming (bdev, claimed_bdev, loop_set_fd); +bd_abort_claiming (bdev, claimed_bdev, loop_set_fd_and_status); out_putf:fput (file); out:
@@ -1662,8 +1665,25 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,int err;switch (cmd) { -case LOOP_SET_FD : -return loop_set_fd (lo, mode, bdev, arg); +case LOOP_SET_FD : { +/* legacy case - pass in a zeroed out loop_info64, which +* corresponds with the default parameters we'd have used +* otherwise. +*/ +struct loop_info64 info; + +memset (&info,0 ,sizeof (info)); +return loop_set_fd_and_status (lo, mode, bdev, arg, &info); + } +case LOOP_SET_FD_AND_STATUS : { +struct loop_fd_and_status fds; + +if (copy_from_user (&fds, argp,sizeof (fds))) +return -EFAULT ; + +return loop_set_fd_and_status (lo, mode, bdev, fds.fd, + &fds.info); + }case LOOP_CHANGE_FD :return loop_change_fd (lo, bdev, arg);case LOOP_CLR_FD :
@@ -1835,6 +1855,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,case LOOP_CLR_FD :case LOOP_GET_STATUS64 :case LOOP_SET_STATUS64 : +case LOOP_SET_FD_AND_STATUS : arg = (unsigned long )compat_ptr (arg);/* fall through */ case LOOP_SET_FD :
include/uapi/linux/loop.h +7 -0
@@ -60,6 +60,12 @@ struct loop_info64 {__u64 lo_init[2 ]; }; +struct loop_fd_and_status { +struct loop_info64 info; +__u32 fd; +__u32 __pad; +}; +/* * Loop filter types */
@@ -90,6 +96,7 @@ struct loop_info64 {#define LOOP_SET_CAPACITY 0x4C07#define LOOP_SET_DIRECT_IO 0x4C08#define LOOP_SET_BLOCK_SIZE 0x4C09 +#define LOOP_SET_FD_AND_STATUS 0x4C0A/* /dev/loop-control interface */ #define LOOP_CTL_ADD 0x4C80
Signature · 1 line
2.26.2.303.gf8c07b1a785-goog
Re: [PATCH v2 5/5] loop: Add LOOP_SET_FD_AND_STATUS ioctl
drivers/block/loop.c static int lo_ioctl(struct block_device *bdev, fmode_t mode, · line 1672
Quoted 5 lines
drivers/block/loop.c +5
-0+case LOOP_SET_FD : { +/* legacy case - pass in a zeroed out loop_info64, which +* corresponds with the default parameters we'd have used +* otherwise. +*/
Nitpick: kernel coding style always has the /* on a line of its own. Also please capitalize the first word in a multi-line comment.
drivers/block/loop.c static int lo_ioctl(struct block_device *bdev, fmode_t mode, · line 1685
Quoted 13 lines
drivers/block/loop.c +13
-0+struct loop_info64 info; + +memset (&info,0 ,sizeof (info)); +return loop_set_fd_and_status (lo, mode, bdev, arg, &info); + } +case LOOP_SET_FD_AND_STATUS : { +struct loop_fd_and_status fds; + +if (copy_from_user (&fds, argp,sizeof (fds))) +return -EFAULT ; + +return loop_set_fd_and_status (lo, mode, bdev, fds.fd, + &fds.info);
What about actually passing the whole loop_fd_and_status structure?
Otherwise this looks good:
Reviewed-by Christoph Hellwig <[email protected]>