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

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

[Mauricio Faria de Oliveira]

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

Co-developed-by: Eric Desrochers <eric.desrochers@canonical.com>
Signed-off-by: Eric Desrochers <eric.desrochers@canonical.com>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Tested-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---

v2:
- Fix ordering of Co-developed-by:/Signed-off-by: tags. (thanks, Krisman)
- Add Tested-by: tag. (likewise.)

 drivers/block/loop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e5ff328f0917..49517482e061 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1212,6 +1212,9 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 		goto out_unlock;
 	}
 
+	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY) && filp->f_op->fsync)
+		blk_queue_write_cache(lo->lo_queue, false, false);
+
 	/* freeze request queue during the transition */
 	blk_mq_freeze_queue(lo->lo_queue);
 
-- 
2.27.0

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

[Ming Lei]

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.

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.


Thanks, 
Ming

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

[Mauricio Faria de Oliveira]

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.

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.

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.

cheers,

Setup:

    # DEV=/dev/loop7
    # IMG=/tmp/image
    # truncate --size 1M $IMG

Before attach/detach:

    # dd if=$DEV of=/dev/null
    0+0 records in
    0+0 records out
    0 bytes copied, 0.00011206 s, 0.0 kB/s

    # echo $?
    0

    # dd if=/dev/zero of=$DEV
    dd: writing to '/dev/loop7': No space left on device
    1+0 records in
    0+0 records out
    0 bytes copied, 0.000229225 s, 0.0 kB/s

    # echo $?
    1

After attach/detach: (same)

    # losetup $DEV $IMG
    # losetup -d $DEV

    # dd if=$DEV of=/dev/null
    0+0 records in
    0+0 records out
    0 bytes copied, 0.000102131 s, 0.0 kB/s

    # echo $?
    0

    # dd if=/dev/zero of=$DEV
    dd: writing to '/dev/loop7': No space left on device
    1+0 records in
    0+0 records out
    0 bytes copied, 0.000259658 s, 0.0 kB/s

    # echo $?
    1


>
> Thanks,
> Ming
>


-- 
Mauricio Faria de Oliveira

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

[Ming Lei]

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

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

[Mauricio Faria de Oliveira]

On Thu, Jan 7, 2021 at 4:11 AM Ming Lei <ming.lei@redhat.com> wrote:
[snip]
> 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);
>

Nice catch. Fixed that in v3.

That was actually a bug, where a read-only loop device (losetup -r)
that had write cache enabled via sysfs while attached, would not
disable it, since it was checking only for the read-only bits, thus
still suffering from the original issue.

Thanks,

> Thanks,
> Ming
>


-- 
Mauricio Faria de Oliveira