Re: [PATCH] loop: stop using vfs_iter_{read,write} for buffered I/O

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Apr 10, 2025 at 09:34:39AM +0200, Christoph Hellwig wrote:
> On Wed, Apr 09, 2025 at 09:44:41PM +0800, Ming Lei wrote:
> > On Wed, Apr 09, 2025 at 03:09:40PM +0200, Christoph Hellwig wrote:
> > > vfs_iter_{read,write} always perform direct I/O when the file has the
> > > O_DIRECT flag set, which breaks disabling direct I/O using the
> > > LOOP_SET_STATUS / LOOP_SET_STATUS64 ioctls.
> > 
> > So dio is disabled automatically because lo_offset is changed in
> > LOOP_SET_STATUS, but backing file is still opened with O_DIRECT,
> > then dio fails?
> > 
> > But Darrick reports it is caused by changing sector size, instead of
> > LOOP_SET_STATUS.
> 
> LOOP_SET_STATUS changes the direct I/O flag.
> 
> This is the minimal reproducer, dev needs to be a 4k lba size device:
> 
> dev=/dev/nvme0n1
> 
> mkfs.xfs -f $dev
> mount $dev /mnt
> 
> truncate -s 30g /mnt/a
> losetup --direct-io=on -f --show /mnt/a
> losetup --direct-io=off /dev/loop0
> losetup --sector-size 2048 /dev/loop0
> mkfs.xfs /dev/loop0
> 
> mkfs then fails with an I/O error.
> 
> (I plan to wire up something like this for blktests)

Please cc me so I can pick through the test. :)

> > > This was recenly reported as a regression, but as far as I can tell
> > > was only uncovered by better checking for block sizes and has been
> > > around since the direct I/O support was added.
> > 
> > What is the 1st real bad commit for this regression? I think it is useful
> > for backporting. Or it is new test case?
> 
> Not entirely sure, maybe Darrick can fill in.

It was a surprisingly hard struggle to get losetup on RHEL8 and 9 to
cooperate.  It doesn't look like 5.4 or 5.15 are affected.  Regrettably
it's going to be harder to test 6.1 and 6.6 because I don't have kernels
at the ready, and grubby is a PITA to deal with.

--D

> > 
> > > 
> > > Fix this by using the existing aio code that calls the raw read/write
> > > iter methods instead.  Note that despite the comments there is no need
> > > for block drivers to ever call flush_dcache_page themselves, and the
> > > call is a left-over from prehistoric times.
> > > 
> > > Fixes: ab1cb278bc70 ("block: loop: introduce ioctl command of LOOP_SET_DIRECT_IO")
> > 
> > Why is the issue related with ioctl(LOOP_SET_DIRECT_IO)?
> > 
> > 
> > Thanks, 
> > Ming
> ---end quoted text---