linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Theodore Y. Ts'o" <tytso@mit.edu>
To: Jeff Layton <jlayton@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	linux-fsdevel@vger.kernel.org, fstests@vger.kernel.org,
	xfs <linux-xfs@vger.kernel.org>,
	linux-block <linux-block@vger.kernel.org>
Subject: Re: [PATCH] fs: clear writeback errors in inode_init_always
Date: Mon, 21 May 2018 10:43:33 -0400	[thread overview]
Message-ID: <20180521144333.GA23868@thunk.org> (raw)
In-Reply-To: <a11668bbe2050c8b5fb8f27b8b301ba8a78e6a97.camel@kernel.org>

On Mon, May 21, 2018 at 07:20:05AM -0400, Jeff Layton wrote:
> 
> As a side note, it looks like __loop_update_dio will discard an error
> from vfs_fsync, so certain ioctls against a loop device can cause errors
> to be lost. It seems like those ought to get propagated to userland or
> to the blockdev's mapping somehow.

I'm not sure it's worth it.  All of the ioctls that call
loop_update_dio are used just to configure the loop device.  In
practice they are never called once the loop device is actually
running.  It might be a better choice is to just define that errors
get reset if there is any attempt to reconfigure the loop device.

One of these ioctls change the offset that a loop device maps onto the
backing file.  So for example, while offset 0 of the loop device might
have corresponds beginning of the backing file, after executing the
ioctl, offset 0 of the loop device no corresponds to offset 2MB of the
backing store.  This might happen if we are resetting the loop device
to point at the first partition of a disk image, for example.  I
really don't think that preserving the error status when we are doing
that kind of radical configuration makes sense.

Or for example, when we change the block size of the loop device; if
the underlying backing store is an IBM Mainframe DASD with a 2k block
size, the reason why the error was signalled was because there was an
attempt to write a 1k block onto a 2k block device.  So again,
resetting the error status of the loop device is the right thing to
do.

The final thing that's worth perhaps exploring is whether or not we
need all of these knobs and ioctls.  In particular, LOOP_CHANGE_FD is
used by losetup.  It exists because back in prehistory, some
distribution installer needed it for some obscure corner case.  If I
remember correctly, it had to do with switching out the initramfs
floppy disk with the root file system floppy disk.  We've wasted
developer bandwidth trying to deal with syzbot complaints about that
ioctl, and it's not clear it was worth the effort, because it's not
clear any real code actually _needs_ that ioctl.  It might have been
easier to comment out the ioctl, and see if anyone screamed.

Given that loop device is maintianed mostly on the margins, and it's
not clear that lots of complicated error handling during setup is
really necessary, this is basically a please to keep things simple, or
at least no more complicated than it has to be.

Cheers,

						- Ted

  reply	other threads:[~2018-05-21 14:43 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-18 22:50 commit b4678df184b causing xfstests regressions Theodore Y. Ts'o
2018-05-19  2:17 ` Matthew Wilcox
2018-05-19 13:09 ` Jeff Layton
2018-05-19 15:25   ` Darrick J. Wong
2018-05-19 15:27   ` [PATCH] fs: clear writeback errors in inode_init_always Darrick J. Wong
2018-05-19 15:36     ` Matthew Wilcox
2018-05-21 17:54       ` Darrick J. Wong
2018-05-22 10:30         ` Jeff Layton
2018-05-22 22:09           ` Dave Chinner
2018-05-23 10:56             ` Jeff Layton
2018-05-24  3:59               ` Dave Chinner
2018-05-19 23:19     ` Theodore Y. Ts'o
2018-05-20 11:45       ` Jeff Layton
2018-05-20 12:58         ` Matthew Wilcox
2018-05-20 13:18           ` Jeff Layton
2018-05-20 16:29           ` Theodore Y. Ts'o
2018-05-20 19:20             ` Matthew Wilcox
2018-05-20 19:41               ` Theodore Y. Ts'o
2018-05-21 11:20                 ` Jeff Layton
2018-05-21 14:43                   ` Theodore Y. Ts'o [this message]
2018-05-20 17:57         ` Theodore Y. Ts'o
2018-05-22  4:06     ` [PATCH v2] " Darrick J. Wong
2018-05-22 10:14       ` Jeff Layton
2018-05-22 12:14       ` Brian Foster
2018-05-22 14:37         ` Darrick J. Wong
2018-05-22 16:43     ` [PATCH v3] " Darrick J. Wong
2018-05-22 18:40       ` Brian Foster
2018-05-22 18:47         ` Darrick J. Wong
2018-05-22 22:05       ` Dave Chinner
2018-05-23  3:00         ` Darrick J. Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180521144333.GA23868@thunk.org \
    --to=tytso@mit.edu \
    --cc=darrick.wong@oracle.com \
    --cc=fstests@vger.kernel.org \
    --cc=jlayton@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH] fs: clear writeback errors in inode_init_always' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).