On Thu, Jan 30, 2025 at 02:49:38PM +0900, Damien Le Moal wrote:
> > +static void loop_query_min_dio_size(struct loop_device *lo)
> > +{
> > + struct file *file = lo->lo_backing_file;
> > + struct block_device *sb_bdev = file->f_mapping->host->i_sb->s_bdev;
> > + struct kstat st;
> > +
> > + if (!vfs_getattr(&file->f_path, &st, STATX_DIOALIGN, 0) &&
> > + (st.result_mask & STATX_DIOALIGN))
> > + lo->lo_min_dio_size = st.dio_offset_align;
>
> Nit: Maybe return here to avoid the else below and the comment block in the
> middle of a if-else-if ?
That looks kinda weird. But maybe I can just return the value instead
and let the caller assign it, which would allow an early return without
multi-line blocks.
> > -static unsigned int loop_default_blocksize(struct loop_device *lo,
> > - struct block_device *backing_bdev)
> > +static unsigned int loop_default_blocksize(struct loop_device *lo)
> > {
> > - /* In case of direct I/O, match underlying block size */
> > - if ((lo->lo_backing_file->f_flags & O_DIRECT) && backing_bdev)
> > - return bdev_logical_block_size(backing_bdev);
> > + /* In case of direct I/O, match underlying minimum I/O size */
> > + if (lo->lo_backing_file->f_flags & O_DIRECT)
> > + return lo->lo_min_dio_size;
>
> I wonder if conditionally returning lo_min_dio_size only for O_DIRECT is
> correct. What if the loop dev is first started without direct IO and gets a
> block size of 512B, and then later restarted with direct IO and now gets a block
> size of say 4K ? That could break a file system that is already on the device
> and formatted using the initial smaller block size. So shouldn't we simply
> always return lo_min_dio_size here, regardless of if O_DIRECT is used or not ?
Yes, that's not handled correctly here or in the existing code. I'll
add a patch before this one to fix that up.
