Re: [PATCH v2] loop: fix I/O error on fsync() in detached loop devices

[Ming Lei <ming.lei@redhat.com>]

On Wed, Jan 06, 2021 at 08:33:50PM -0300, Mauricio Faria de Oliveira wrote:
> On Wed, Jan 6, 2021 at 6:08 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Tue, Jan 05, 2021 at 10:54:19AM -0300, Mauricio Faria de Oliveira wrote:
> > > There's an I/O error on fsync() in a detached loop device
> > > if it has been previously attached.
> > >
> > > The issue is write cache is enabled in the attach path in
> > > loop_configure() but it isn't disabled in the detach path;
> > > thus it remains enabled in the block device regardless of
> > > whether it is attached or not.
> > >
> > > Now fsync() can get an I/O request that will just be failed
> > > later in loop_queue_rq() as device's state is not 'Lo_bound'.
> > >
> > > So, disable write cache in the detach path.
> > >
> > > Test-case:
> > >
> > >     # DEV=/dev/loop7
> > >
> > >     # IMG=/tmp/image
> > >     # truncate --size 1M $IMG
> > >
> > >     # losetup $DEV $IMG
> > >     # losetup -d $DEV
> > >
> > > Before:
> > >
> > >     # strace -e fsync parted -s $DEV print 2>&1 | grep fsync
> > >     fsync(3)                                = -1 EIO (Input/output error)
> > >     Warning: Error fsyncing/closing /dev/loop7: Input/output error
> > >     [  982.529929] blk_update_request: I/O error, dev loop7, sector 0 op 0x1:(WRITE) flags 0x800 phys_seg 0 prio class 0
> > >
> > > After:
> > >
> > >     # strace -e fsync parted -s $DEV print 2>&1 | grep fsync
> > >     fsync(3)                                = 0
> >
> > But IO on detached loop should have been failed, right? The magic is
> > that submit_bio_checks() filters FLUSH request for queues which doesn't
> > support writeback cache, and always fake a normal completion.
> >
> 
> Hey Ming, thanks for taking a look at this.
> 
> Well, it depends -- currently read() works (without I/O errors) and
> write() fails (ENOSPC).
> Example tests are provided below.

read() actually returns 0 because of the following code in blkdev_read_iter():

        if (pos >= size)
                return 0;

> 
> And that's consistent before and after attach/detach; so, I thought
> fsync() should follow.
> 
> > I understand that the issue is that user becomes confused with this observation
> > because no such failure if they run 'parted -s /dev/loop0 print' on one detached
> > loop disk if it is never attached.
> >
> 
> That is indeed one of the issues. There's also a monitoring/alerting
> perspective that
> would benefit; e.g., sosreport runs parted, it's run on data
> collection for support cases.
> Now, that I/O error message is thrown in the logs, and some mon/alert
> tools might not
> yet have filters to ignore (detached) loop devices, and alert. It'd be
> nice to deflect that.

IMO, if loop is detached, any IO should have been failed. However,
read/flush is just a bit special:

- blkdev_read_iter() always return 0 if the read is beyond the device
size(0)

- submit_bio(FLUSH) return successfully if the queue doesn't support
writeback cache.

> 
> It's not a common issue, to be honest; but the consistency point
> seemed fair to me,
> as essentially the current code doesn't deinitialize something it
> previously initialized,
> and the block device is left running with that enabled regardless.

OK, looks it is fine to disable writeback cache in __loop_clr_fd().

BTW, just wondering why don't you disable WC unconditionally in
__loop_clr_fd() or clear it in the following way because WC can be
changed via sysfs?

	if (test_bit(QUEUE_FLAG_WC, &q->queue_flags))
		blk_queue_write_cache(q, false, false);

Thanks, 
Ming