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.
