| Message ID | 20210105135419.68715-1-mfo@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] loop: fix I/O error on fsync() in detached loop devices | expand |
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
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 >
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
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 >
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);
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(+)