| Message ID | cda9076854b30eb4c05092b90fae747e7f1f6542.1503422996.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 22, 2017 at 10:33:00AM -0700, Omar Sandoval wrote: > @@ -946,6 +938,9 @@ 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); > + blk_queue_logical_block_size(lo->lo_queue, 512); > + blk_queue_physical_block_size(lo->lo_queue, 512); > + blk_queue_io_min(lo->lo_queue, 512); ... > @@ -1133,14 +1128,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) ... > + if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { > + blk_queue_logical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); > + blk_queue_physical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); > + blk_queue_io_min(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); > + } > + I don't understand this. * it seems the default is 512, but what about if the backing file is not a regular file? For example "losetup -f /dev/sda" where sda sector size is > 512. * why the attributes in the /sys are affected by LO_FLAGS_BLOCKSIZE? Would be better to have a real sizes there (independently on the flag)? * I think kernel lies in /sys about I/O size now. The phy sector size has never been 512, but PAGE_SIZE or real phy sector if backing file is a block device. Your patch improves it for LO_FLAGS_BLOCKSIZE, but it's still lies about internally used I/O sizes. Why we cannot use blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize); blk_queue_logical_block_size(lo->lo_queue, lo->lo_logical_blocksize); (as suggested by Hannes' original patch), but without if (info->lo_flags & LO_FLAGS_BLOCKSIZE) condition? Yes, result will be backwardly incompatible, but the current situation (all is 512) does not describe reality. It's legacy from time where all in kernel was 512... Karel
On Wed, Aug 23, 2017 at 11:23:55AM +0200, Karel Zak wrote: > On Tue, Aug 22, 2017 at 10:33:00AM -0700, Omar Sandoval wrote: > > @@ -946,6 +938,9 @@ 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); > > + blk_queue_logical_block_size(lo->lo_queue, 512); > > + blk_queue_physical_block_size(lo->lo_queue, 512); > > + blk_queue_io_min(lo->lo_queue, 512); > > ... > > > @@ -1133,14 +1128,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) > ... > > + if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { > > + blk_queue_logical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); > > + blk_queue_physical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); > > + blk_queue_io_min(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); > > + } > > + > > I don't understand this. > > * it seems the default is 512, but what about if the backing file > is not a regular file? For example "losetup -f /dev/sda" where > sda sector size is > 512. I didn't change the legacy behavior here, i.e., it's always 512 by default. > * why the attributes in the /sys are affected by LO_FLAGS_BLOCKSIZE? > Would be better to have a real sizes there (independently on the flag)? What do you mean? LO_FLAGS_BLOCKSIZE means I want to change the logical blocksize of the loop device, so why shouldn't it be reflected in sysfs? > * I think kernel lies in /sys about I/O size now. The phy sector size > has never been 512, but PAGE_SIZE or real phy sector if backing file > is a block device. > > Your patch improves it for LO_FLAGS_BLOCKSIZE, but it's still lies > about internally used I/O sizes. > > Why we cannot use > > blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize); > blk_queue_logical_block_size(lo->lo_queue, lo->lo_logical_blocksize); > > (as suggested by Hannes' original patch), but without > > if (info->lo_flags & LO_FLAGS_BLOCKSIZE) > > condition? > > Yes, result will be backwardly incompatible, but the current > situation (all is 512) does not describe reality. It's legacy from > time where all in kernel was 512... I went back and forth on this. Yeah, the kernel is lying, and using the backing block size makes more sense, but backwards compatability is exactly why I didn't change this. Then again, the physical block size is more of a hint, so it might be safe to change this.
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index a444dc2d5977..28026f0abaa9 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -221,8 +221,7 @@ 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, - loff_t logical_blocksize) +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit) { loff_t size = get_size(offset, sizelimit, lo->lo_backing_file); sector_t x = (sector_t)size; @@ -234,12 +233,6 @@ 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 */ @@ -934,7 +927,6 @@ 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; @@ -946,6 +938,9 @@ 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); + blk_queue_logical_block_size(lo->lo_queue, 512); + blk_queue_physical_block_size(lo->lo_queue, 512); + blk_queue_io_min(lo->lo_queue, 512); loop_update_dio(lo); set_capacity(lo->lo_disk, size); @@ -1133,14 +1128,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) } else xfer = NULL; - err = loop_init_xfer(lo, xfer, info); - if (err) - goto exit; - if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { - if (!(lo->lo_flags & LO_FLAGS_BLOCKSIZE)) - lo->lo_logical_blocksize = 512; - lo->lo_flags |= LO_FLAGS_BLOCKSIZE; if (LO_INFO_BLOCKSIZE(info) != 512 && LO_INFO_BLOCKSIZE(info) != 1024 && LO_INFO_BLOCKSIZE(info) != 2048 && @@ -1154,18 +1142,25 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) } } + err = loop_init_xfer(lo, xfer, info); + if (err) + goto exit; + if (lo->lo_offset != info->lo_offset || lo->lo_sizelimit != info->lo_sizelimit || - lo->lo_flags != lo_flags || - ((lo->lo_flags & LO_FLAGS_BLOCKSIZE) && - lo->lo_logical_blocksize != LO_INFO_BLOCKSIZE(info))) { - if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit, - LO_INFO_BLOCKSIZE(info))) { + lo->lo_flags != lo_flags) { + if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) { err = -EFBIG; goto exit; } } + if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { + blk_queue_logical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); + blk_queue_physical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); + blk_queue_io_min(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); + } + loop_config_discard(lo); memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE); @@ -1352,8 +1347,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, - lo->lo_logical_blocksize); + return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit); } 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 2c096b9a17b8..fecd3f97ef8c 100644 --- a/drivers/block/loop.h +++ b/drivers/block/loop.h @@ -49,7 +49,6 @@ 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;